From 06d7800895372d6ebcbd557cae5c227b7ece083e Mon Sep 17 00:00:00 2001 From: Virgil Date: Mon, 23 Mar 2026 14:30:08 +0000 Subject: [PATCH] fix(security): close audit #4 gaps in workspace, local, s3, datanode Co-Authored-By: Virgil --- datanode/client.go | 60 +++++++++++++++++++++--------------- datanode/client_test.go | 29 ++++++++++++++++- local/client.go | 11 +++++++ local/client_test.go | 15 +++++++++ s3/s3.go | 65 +++++++++++++++++++++++---------------- s3/s3_test.go | 12 ++++++-- workspace/service.go | 46 +++++++++++++++++---------- workspace/service_test.go | 8 +++++ 8 files changed, 176 insertions(+), 70 deletions(-) diff --git a/datanode/client.go b/datanode/client.go index c4f09ad..e76e65f 100644 --- a/datanode/client.go +++ b/datanode/client.go @@ -233,7 +233,7 @@ func (m *Medium) Delete(p string) error { } // Remove the file by creating a new DataNode without it - if err := m.removeFileLocked(p); err != nil { + if err := m.removeFilesLocked(map[string]struct{}{p: {}}); err != nil { return coreerr.E("datanode.Delete", "failed to delete file: "+p, err) } return nil @@ -253,10 +253,9 @@ func (m *Medium) DeleteAll(p string) error { // Check if p itself is a file info, err := m.dn.Stat(p) + toDelete := make(map[string]struct{}) if err == nil && !info.IsDir() { - if err := m.removeFileLocked(p); err != nil { - return coreerr.E("datanode.DeleteAll", "failed to delete file: "+p, err) - } + toDelete[p] = struct{}{} found = true } @@ -267,13 +266,17 @@ func (m *Medium) DeleteAll(p string) error { } for _, name := range entries { if name == p || strings.HasPrefix(name, prefix) { - if err := m.removeFileLocked(name); err != nil { - return coreerr.E("datanode.DeleteAll", "failed to delete file: "+name, err) - } + toDelete[name] = struct{}{} found = true } } + if found { + if err := m.removeFilesLocked(toDelete); err != nil { + return coreerr.E("datanode.DeleteAll", "failed to delete files", err) + } + } + // Remove explicit dirs under prefix for d := range m.dirs { if d == p || strings.HasPrefix(d, prefix) { @@ -303,15 +306,10 @@ func (m *Medium) Rename(oldPath, newPath string) error { if !info.IsDir() { // Read old, write new, delete old - data, err := m.readFileLocked(oldPath) - if err != nil { + if err := m.rewriteDataNodeLocked(map[string]string{oldPath: newPath}); err != nil { return coreerr.E("datanode.Rename", "failed to read source file: "+oldPath, err) } - m.dn.AddData(newPath, data) m.ensureDirsLocked(path.Dir(newPath)) - if err := m.removeFileLocked(oldPath); err != nil { - return coreerr.E("datanode.Rename", "failed to remove source file: "+oldPath, err) - } return nil } @@ -323,19 +321,15 @@ func (m *Medium) Rename(oldPath, newPath string) error { if err != nil { return coreerr.E("datanode.Rename", "failed to inspect tree: "+oldPath, err) } + renames := make(map[string]string) for _, name := range entries { if strings.HasPrefix(name, oldPrefix) { - newName := newPrefix + strings.TrimPrefix(name, oldPrefix) - data, err := m.readFileLocked(name) - if err != nil { - return coreerr.E("datanode.Rename", "failed to read source file: "+name, err) - } - m.dn.AddData(newName, data) - if err := m.removeFileLocked(name); err != nil { - return coreerr.E("datanode.Rename", "failed to remove source file: "+name, err) - } + renames[name] = newPrefix + strings.TrimPrefix(name, oldPrefix) } } + if err := m.rewriteDataNodeLocked(renames); err != nil { + return coreerr.E("datanode.Rename", "failed to move source files", err) + } // Move explicit dirs dirsToMove := make(map[string]string) @@ -560,20 +554,38 @@ func (m *Medium) readFileLocked(name string) ([]byte, error) { // This is necessary because Borg's DataNode doesn't expose a Remove method. // Caller must hold m.mu write lock. func (m *Medium) removeFileLocked(target string) error { + exclude := map[string]struct{}{target: {}} + return m.removeFilesLocked(exclude) +} + +func (m *Medium) removeFilesLocked(targets map[string]struct{}) error { + renames := make(map[string]string) + for target := range targets { + renames[target] = "" + } + return m.rewriteDataNodeLocked(renames) +} + +func (m *Medium) rewriteDataNodeLocked(renames map[string]string) error { entries, err := m.collectAllLocked() if err != nil { return err } newDN := borgdatanode.New() for _, name := range entries { - if name == target { + targetName, ok := renames[name] + if ok && targetName == "" { continue } + writeName := name + if ok { + writeName = targetName + } data, err := m.readFileLocked(name) if err != nil { return err } - newDN.AddData(name, data) + newDN.AddData(writeName, data) } m.dn = newDN return nil diff --git a/datanode/client_test.go b/datanode/client_test.go index 8beb6cd..fc9a12a 100644 --- a/datanode/client_test.go +++ b/datanode/client_test.go @@ -215,7 +215,34 @@ func TestRenameDir_Bad_ReadFailure(t *testing.T) { err := m.Rename("src", "dst") require.Error(t, err) - assert.Contains(t, err.Error(), "failed to read source file") + assert.Contains(t, err.Error(), "failed to move source files") +} + +func TestRenameDir_Bad_AtomicFailure(t *testing.T) { + m := New() + require.NoError(t, m.Write("src/a.txt", "one")) + require.NoError(t, m.Write("src/b.txt", "two")) + + readCalls := 0 + originalReadAll := dataNodeReadAll + dataNodeReadAll = func(r io.Reader) ([]byte, error) { + readCalls++ + if readCalls == 2 { + return nil, errors.New("read failed") + } + return originalReadAll(r) + } + t.Cleanup(func() { + dataNodeReadAll = originalReadAll + }) + + err := m.Rename("src", "dst") + require.Error(t, err) + + assert.True(t, m.IsFile("src/a.txt")) + assert.True(t, m.IsFile("src/b.txt")) + assert.False(t, m.IsFile("dst/a.txt")) + assert.False(t, m.IsFile("dst/b.txt")) } func TestList_Good(t *testing.T) { diff --git a/local/client.go b/local/client.go index d4aaafc..044804d 100644 --- a/local/client.go +++ b/local/client.go @@ -154,11 +154,22 @@ func canonicalPath(p string) string { return absolutePath(p) } +func osUserHomeDir() string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return home +} + func isProtectedPath(full string) bool { full = canonicalPath(full) protected := map[string]struct{}{ canonicalPath(dirSeparator()): {}, } + if home := osUserHomeDir(); home != "" { + protected[canonicalPath(home)] = struct{}{} + } for _, home := range []string{core.Env("HOME"), core.Env("DIR_HOME")} { if home == "" { continue diff --git a/local/client_test.go b/local/client_test.go index 120ee0e..a714554 100644 --- a/local/client_test.go +++ b/local/client_test.go @@ -198,6 +198,21 @@ func TestDeleteAll_ProtectedHomeViaEnv(t *testing.T) { assert.DirExists(t, tempHome) } +func TestDelete_ProtectedHomeBypassesEnvHijack(t *testing.T) { + home, err := os.UserHomeDir() + require.NoError(t, err) + require.NotEmpty(t, home) + + t.Setenv("HOME", t.TempDir()) + + m, err := New("/") + require.NoError(t, err) + + err = m.Delete(home) + require.Error(t, err) + assert.DirExists(t, home) +} + func TestRename(t *testing.T) { root := t.TempDir() m, _ := New(root) diff --git a/s3/s3.go b/s3/s3.go index 3ca4ab9..f6c1301 100644 --- a/s3/s3.go +++ b/s3/s3.go @@ -195,6 +195,28 @@ func (m *Medium) FileSet(p, content string) error { return m.Write(p, content) } +func (m *Medium) deleteObjectBatch(prefix string, keys []string) error { + if len(keys) == 0 { + return nil + } + objects := make([]types.ObjectIdentifier, len(keys)) + for i, key := range keys { + objects[i] = types.ObjectIdentifier{Key: aws.String(key)} + } + + deleteOut, err := m.client.DeleteObjects(context.Background(), &s3.DeleteObjectsInput{ + Bucket: aws.String(m.bucket), + Delete: &types.Delete{Objects: objects, Quiet: aws.Bool(true)}, + }) + if err != nil { + return coreerr.E("s3.DeleteAll", "failed to delete objects", err) + } + if err := deleteObjectsError(prefix, deleteOut.Errors); err != nil { + return err + } + return nil +} + // Delete removes a single object. func (m *Medium) Delete(p string) error { key := m.key(p) @@ -219,16 +241,7 @@ func (m *Medium) DeleteAll(p string) error { return coreerr.E("s3.DeleteAll", "path is required", os.ErrInvalid) } - // First, try deleting the exact key - _, err := m.client.DeleteObject(context.Background(), &s3.DeleteObjectInput{ - Bucket: aws.String(m.bucket), - Key: aws.String(key), - }) - if err != nil { - return coreerr.E("s3.DeleteAll", "failed to delete object: "+key, err) - } - - // Then delete all objects under the prefix + deleteKeys := []string{key} prefix := key if !strings.HasSuffix(prefix, "/") { prefix += "/" @@ -247,26 +260,20 @@ func (m *Medium) DeleteAll(p string) error { return coreerr.E("s3.DeleteAll", "failed to list objects: "+prefix, err) } + for _, obj := range listOut.Contents { + deleteKeys = append(deleteKeys, aws.ToString(obj.Key)) + if len(deleteKeys) == 1000 { + if err := m.deleteObjectBatch(prefix, deleteKeys); err != nil { + return err + } + deleteKeys = nil + } + } + if len(listOut.Contents) == 0 { break } - objects := make([]types.ObjectIdentifier, len(listOut.Contents)) - for i, obj := range listOut.Contents { - objects[i] = types.ObjectIdentifier{Key: obj.Key} - } - - deleteOut, err := m.client.DeleteObjects(context.Background(), &s3.DeleteObjectsInput{ - Bucket: aws.String(m.bucket), - Delete: &types.Delete{Objects: objects, Quiet: aws.Bool(true)}, - }) - if err != nil { - return coreerr.E("s3.DeleteAll", "failed to delete objects", err) - } - if err := deleteObjectsError(prefix, deleteOut.Errors); err != nil { - return err - } - if listOut.IsTruncated != nil && *listOut.IsTruncated { continuationToken = listOut.NextContinuationToken } else { @@ -274,6 +281,12 @@ func (m *Medium) DeleteAll(p string) error { } } + if len(deleteKeys) > 0 { + if err := m.deleteObjectBatch(prefix, deleteKeys); err != nil { + return err + } + } + return nil } diff --git a/s3/s3_test.go b/s3/s3_test.go index a81efff..d614576 100644 --- a/s3/s3_test.go +++ b/s3/s3_test.go @@ -3,7 +3,6 @@ package s3 import ( "bytes" "context" - "errors" "fmt" goio "io" "io/fs" @@ -365,11 +364,18 @@ func TestDeleteAll_Bad_EmptyPath(t *testing.T) { func TestDeleteAll_Bad_DeleteObjectError(t *testing.T) { m, mock := newTestMedium(t) - mock.deleteObjectErrors["dir"] = errors.New("boom") + require.NoError(t, m.Write("dir", "metadata")) + mock.deleteObjectsErrs["dir"] = types.Error{ + Key: aws.String("dir"), + Code: aws.String("AccessDenied"), + Message: aws.String("blocked"), + } err := m.DeleteAll("dir") require.Error(t, err) - assert.Contains(t, err.Error(), "failed to delete object: dir") + assert.Contains(t, err.Error(), "partial delete failed") + assert.Contains(t, err.Error(), "dir: AccessDenied blocked") + assert.True(t, m.IsFile("dir")) } func TestDeleteAll_Bad_PartialDelete(t *testing.T) { diff --git a/workspace/service.go b/workspace/service.go index faa443e..e7c6ad9 100644 --- a/workspace/service.go +++ b/workspace/service.go @@ -197,15 +197,6 @@ func workspaceHome() string { return core.Env("DIR_HOME") } -func joinWithinRoot(root string, parts ...string) (string, error) { - candidate := core.Path(append([]string{root}, parts...)...) - sep := core.Env("DS") - if candidate == root || strings.HasPrefix(candidate, root+sep) { - return candidate, nil - } - return "", os.ErrPermission -} - func resolveWorkspacePath(rootPath, workspacePath string) error { resolvedRoot, err := filepath.EvalSymlinks(rootPath) if err != nil { @@ -234,23 +225,46 @@ func resolveWorkspacePath(rootPath, workspacePath string) error { return nil } +func joinWithinRoot(root string, parts ...string) (string, error) { + candidate := filepath.Clean(core.Path(append([]string{root}, parts...)...)) + if candidate == root || strings.HasPrefix(candidate, root+string(os.PathSeparator)) { + return candidate, nil + } + return "", os.ErrPermission +} + func (s *Service) workspacePath(op, name string) (string, error) { if name == "" { return "", coreerr.E(op, "workspace name is required", os.ErrInvalid) } - path, err := joinWithinRoot(s.rootPath, name) - if err != nil { - return "", coreerr.E(op, "workspace path escapes root", err) - } - if core.PathDir(path) != s.rootPath { - return "", coreerr.E(op, "invalid workspace name: "+name, os.ErrPermission) - } + path := filepath.Clean(core.Path(s.rootPath, name)) if err := resolveWorkspacePath(s.rootPath, path); err != nil { if errors.Is(err, os.ErrPermission) { return "", coreerr.E(op, "workspace path escapes root", err) } return "", coreerr.E(op, "failed to resolve workspace path", err) } + + rel, err := filepath.Rel(s.rootPath, path) + if err != nil { + return "", coreerr.E(op, "failed to resolve workspace path", err) + } + if rel == "." { + return "", coreerr.E(op, "invalid workspace name: "+name, os.ErrInvalid) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", coreerr.E(op, "workspace path escapes root", os.ErrPermission) + } + if strings.Contains(rel, string(os.PathSeparator)) { + return "", coreerr.E(op, "invalid workspace name: "+name, os.ErrInvalid) + } + if core.PathDir(path) != s.rootPath { + return "", coreerr.E(op, "invalid workspace name: "+name, os.ErrPermission) + } + if path == s.rootPath { + return "", coreerr.E(op, "invalid workspace name: "+name, os.ErrInvalid) + } + return path, nil } diff --git a/workspace/service_test.go b/workspace/service_test.go index f7148d5..f29ea6c 100644 --- a/workspace/service_test.go +++ b/workspace/service_test.go @@ -67,6 +67,14 @@ func TestSwitchWorkspace_TraversalBlocked(t *testing.T) { assert.Empty(t, s.activeWorkspace) } +func TestSwitchWorkspace_DotNameBlocked(t *testing.T) { + s, _ := newTestService(t) + + err := s.SwitchWorkspace(".") + require.Error(t, err) + assert.Empty(t, s.activeWorkspace) +} + func TestSwitchWorkspace_SymlinkEscapeBlocked(t *testing.T) { s, tempHome := newTestService(t)