diff --git a/ingestion/go.mod b/ingestion/go.mod index 5e9f7b2..ac4eea7 100644 --- a/ingestion/go.mod +++ b/ingestion/go.mod @@ -68,7 +68,7 @@ require ( github.com/segmentio/asm v1.2.0 // indirect golang.org/x/crypto v0.51.0 // indirect golang.org/x/sync v0.21.0 // indirect - golang.org/x/sys v0.45.0 // indirect + golang.org/x/sys v0.45.0 golang.org/x/text v0.39.0 // indirect gopkg.in/yaml.v3 v3.0.1 ) diff --git a/ingestion/internal/api/update_redirect_test.go b/ingestion/internal/api/update_redirect_test.go index 65241ba..6d798bd 100644 --- a/ingestion/internal/api/update_redirect_test.go +++ b/ingestion/internal/api/update_redirect_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mathiasbq/hyperguild/ingestion/internal/api" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" ) // hyperguild#173: a directory symlink inside wiki/ that points elsewhere in @@ -23,7 +24,13 @@ func TestUpdateNoteRefusesANoteBehindASymlinkedDirectory(t *testing.T) { require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "facts"), filepath.Join(brain, "wiki", "w", "alias"))) _, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/alias/v.md", Content: "replaced"}) - assert.Error(t, err) + assert.ErrorIs(t, err, brainfs.ErrNotRegular) got, _ := os.ReadFile(victim) assert.Contains(t, string(got), "untouched") + + real := filepath.Join(brain, "wiki", "w", "facts", "r.md") + require.NoError(t, os.MkdirAll(filepath.Dir(real), 0o755)) + require.NoError(t, os.WriteFile(real, []byte("---\ntitle: r\n---\nold\n"), 0o644)) + _, _, _, err = api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/facts/r.md", Content: "new"}) + require.NoError(t, err, "positive control: a note in a real directory is updated") } diff --git a/ingestion/internal/brain/tunnel_redirect_test.go b/ingestion/internal/brain/tunnel_redirect_test.go index fe57d5b..caf5cdd 100644 --- a/ingestion/internal/brain/tunnel_redirect_test.go +++ b/ingestion/internal/brain/tunnel_redirect_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mathiasbq/hyperguild/ingestion/internal/brain" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" ) // hyperguild#173: brain_tunnel's write followed a directory symlink inside @@ -24,9 +25,14 @@ func TestWriteTunnelRefusesATargetBehindASymlinkedDirectory(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(b, "wiki", "b"), 0o755)) require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "facts"), filepath.Join(b, "wiki", "b", "facts"))) - assert.Error(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/b/facts/v.md")) + assert.ErrorIs(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/b/facts/v.md"), brainfs.ErrNotRegular) got, _ := os.ReadFile(victim) assert.Equal(t, "untouched\n", string(got)) got, _ = os.ReadFile(src) assert.Equal(t, "x\n", string(got), "the source is not rewritten either") + + real := filepath.Join(b, "wiki", "c", "facts", "t.md") + require.NoError(t, os.MkdirAll(filepath.Dir(real), 0o755)) + require.NoError(t, os.WriteFile(real, []byte("t\n"), 0o644)) + require.NoError(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/c/facts/t.md"), "positive control: a tunnel between real directories is written") } diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 92ea8e3..97bf8aa 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -104,6 +104,17 @@ func (b *Brain) ReadDirRegular(relDir string) ([]string, error) { return names, nil } +// cleanRel refuses a path that is not clean and local. realDir checks the +// cleaned parent, so the I/O must use that same path: os.Root resolves ".." +// after following a symlink, and a ".." after a symlinked directory would +// otherwise pass the check and redirect the read or write. +func cleanRel(rel string) error { + if rel == "" || path.Clean(rel) != rel || !filepath.IsLocal(rel) || strings.Contains(rel, "\\") { + return fmt.Errorf("%q: not a clean brain-relative path: %w", rel, ErrNotRegular) + } + return nil +} + // ReadDirDirs lists the real subdirectories directly in a brain-relative // directory, by name. Every component of relDir must be a real directory // (ErrNotRegular otherwise); symlinks to directories are left out. @@ -128,7 +139,10 @@ func (b *Brain) ReadDirDirs(relDir string) ([]string, error) { // ReadRegular reads a brain-relative path, refusing anything that is not a // regular file, both before the open and on the opened handle. func (b *Brain) ReadRegular(rel string) ([]byte, error) { - if err := b.realDir(path.Dir(filepath.ToSlash(rel))); err != nil { + if err := cleanRel(rel); err != nil { + return nil, err + } + if err := b.realDir(path.Dir(rel)); err != nil { return nil, err } fi, err := b.root.Lstat(rel) @@ -157,7 +171,10 @@ func (b *Brain) readOpened(rel string) ([]byte, error) { return b.readOpenedMax( // ReadRegularMax is ReadRegular with a size limit, checked on the opened // handle before reading (max < 0 means no limit). func (b *Brain) ReadRegularMax(rel string, max int64) ([]byte, error) { - if err := b.realDir(path.Dir(filepath.ToSlash(rel))); err != nil { + if err := cleanRel(rel); err != nil { + return nil, err + } + if err := b.realDir(path.Dir(rel)); err != nil { return nil, err } fi, err := b.root.Lstat(rel) @@ -266,7 +283,9 @@ func (b *Brain) WalkRegular(relDir string, fn func(rel string) error, onErr func // (hyperguild#173); a directory swapped in after that check is the #167 // residual. func (b *Brain) WriteRegular(rel string, data []byte) error { - rel = filepath.ToSlash(rel) + if err := cleanRel(rel); err != nil { + return err + } // Every directory above the file must be real: os.Root follows an // in-brain intermediate symlink, which would redirect the write // elsewhere in the brain (hyperguild#173). A swap after this check is diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index b8565b7..8935356 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -351,3 +351,28 @@ func TestReadAndWriteRegularRefuseAFileBehindASymlinkedDirectory(t *testing.T) { require.NoError(t, err, "positive control: the real path still reads") assert.Equal(t, "untouched", string(got)) } + +// realDir checks the cleaned parent, so the I/O must use that same clean +// path: os.Root resolves ".." after following a symlink, and a ".." placed +// after a symlinked directory would otherwise pass the check and redirect. +func TestReadAndWriteRegularRefuseAPathThatIsNotClean(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw", "facts"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "raw", "v.md"), []byte("untouched"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki"), 0o755)) + require.NoError(t, os.Symlink(filepath.Join("..", "raw", "facts"), filepath.Join(dir, "wiki", "alias"))) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + + for _, rel := range []string{"wiki/alias/../v.md", "./raw/v.md", "raw//v.md", "/raw/v.md"} { + got, err := b.ReadRegular(rel) + assert.ErrorIs(t, err, ErrNotRegular, rel) + assert.NotContains(t, string(got), "untouched", rel) + _, err = b.ReadRegularMax(rel, 100) + assert.ErrorIs(t, err, ErrNotRegular, rel) + assert.ErrorIs(t, b.WriteRegular(rel, []byte("OVERWRITTEN")), ErrNotRegular, rel) + } + got, _ := os.ReadFile(filepath.Join(dir, "raw", "v.md")) + assert.Equal(t, "untouched", string(got)) +} diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go index ee86db3..e7aeae8 100644 --- a/ingestion/internal/session/id_test.go +++ b/ingestion/internal/session/id_test.go @@ -2,14 +2,17 @@ package session import ( "errors" + "fmt" "os" "path/filepath" + "sync/atomic" "syscall" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" ) // session_log passed the caller's session_id straight into @@ -102,3 +105,73 @@ func TestAppendRefusesASymlinkedSessionsDir(t *testing.T) { ents, _ := os.ReadDir(elsewhere) assert.Empty(t, ents, "a log was created through the symlinked dir") } + +// The sessions dir is swapped (RENAME_EXCHANGE) with a symlink to a +// directory outside the brain while Append runs. Append opens through an +// os.Root on the sessions dir's parent, so a swap can redirect it only +// inside that parent (the #167 residual), never outside it. +func TestAppendNeverCreatesALogOutsideTheParentUnderADirSwapRace(t *testing.T) { + brain := t.TempDir() + outside := t.TempDir() + dir := filepath.Join(brain, "sessions") + other := filepath.Join(brain, "sessions.link") + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.Symlink(outside, other)) + var stop atomic.Bool + done := make(chan struct{}) + go func() { + defer close(done) + for !stop.Load() { + _ = unix.Renameat2(unix.AT_FDCWD, dir, unix.AT_FDCWD, other, unix.RENAME_EXCHANGE) + } + }() + ok := 0 + for i := 0; i < 20000; i++ { + if Append(dir, fmt.Sprintf("s%d", i), Entry{SessionID: "s"}) == nil { + ok++ + } + } + stop.Store(true) + <-done + ents, _ := os.ReadDir(outside) + assert.Empty(t, ents, "logs were created outside the brain") + assert.Positive(t, ok, "positive control: appends to the real dir succeed") +} + +// The log file is swapped between a regular file and a symlink to another +// file in the brain while Append runs: Append writes only after confirming +// the path is the regular file it opened, so the other file never grows. +func TestAppendNeverWritesThroughALogSwappedForASymlink(t *testing.T) { + brain := t.TempDir() + dir := filepath.Join(brain, "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + victim := filepath.Join(brain, "victim.md") + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + log := filepath.Join(dir, "s.jsonl") + var stop atomic.Bool + done := make(chan struct{}) + go func() { + defer close(done) + tmp := log + ".swap" + for i := 0; !stop.Load(); i++ { + _ = os.Remove(tmp) + if i%2 == 0 { + _ = os.WriteFile(tmp, nil, 0o644) + } else { + _ = os.Symlink(filepath.Join("..", "victim.md"), tmp) + } + _ = os.Rename(tmp, log) + } + }() + ok := 0 + for i := 0; i < 20000; i++ { + if Append(dir, "s", Entry{SessionID: "s"}) == nil { + ok++ + } + } + stop.Store(true) + <-done + got, _ := os.ReadFile(victim) + assert.Equal(t, "untouched\n", string(got), "a log line was written through the swapped-in symlink") + assert.Positive(t, ok, "positive control: appends to the regular log succeed") +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index 656da92..901d555 100644 --- a/ingestion/internal/session/session.go +++ b/ingestion/internal/session/session.go @@ -65,21 +65,42 @@ func Append(sessionsDir, sessionID string, entry Entry) error { if err := checkID(sessionID); err != nil { return err } - if err := os.MkdirAll(sessionsDir, 0o755); err != nil { + // Opened through an os.Root on the sessions dir's parent (the brain dir), + // so a sessions dir swapped for a symlink after the check below can + // redirect the log only inside that parent, never outside it (#173; the + // in-brain swap is the #167 residual). + parentDir, name := filepath.Dir(sessionsDir), filepath.Base(sessionsDir) + if err := os.MkdirAll(parentDir, 0o755); err != nil { + return fmt.Errorf("create sessions parent: %w", err) + } + root, err := os.OpenRoot(parentDir) + if err != nil { + return fmt.Errorf("open sessions parent: %w", err) + } + defer func() { _ = root.Close() }() + if err := root.Mkdir(name, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("create sessions dir: %w", err) } // The sessions dir itself must be a real directory, not a symlink that // redirects the log elsewhere (hyperguild#173). - if fi, err := os.Lstat(sessionsDir); err != nil { + if fi, err := root.Lstat(name); err != nil { return fmt.Errorf("stat sessions dir: %w", err) } else if !fi.IsDir() { return fmt.Errorf("sessions dir %s is not a real directory", sessionsDir) } - path := filepath.Join(sessionsDir, sessionID+".jsonl") - f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644) + logRel := name + "/" + sessionID + ".jsonl" + f, err := root.OpenFile(logRel, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644) if err != nil { return fmt.Errorf("open session log: %w", err) } + // os.Root follows an in-root final symlink even with O_NOFOLLOW, so + // confirm, before writing, that the path is the regular file we opened. + hfi, herr := f.Stat() + lfi, lerr := root.Lstat(logRel) + if herr != nil || lerr != nil || !lfi.Mode().IsRegular() || !os.SameFile(lfi, hfi) { + _ = f.Close() + return fmt.Errorf("session log %s is not a regular file", logRel) + } line, err := json.Marshal(entry) if err != nil {