Skip to content

Commit 1bc64ba

Browse files
authored
fix: file-tool security vulnerabilities (#37)
* fix: file-tool security vulnerabilities - confineToCWD now resolves symlinks to prevent directory-symlink traversal in write_file / patch / batch_patch - glob simple mode uses Lstat and skips symlinks - batch_read wraps content with untrusted_content boundary - read_file / batch_read cap returned content at 1 MiB - write_file preserves original file mode on overwrite Includes 9 regression tests and a verification certificate. * chore: remove local verification certificate from branch
1 parent 133bb1a commit 1bc64ba

2 files changed

Lines changed: 346 additions & 6 deletions

File tree

cmd/odek/file_tool.go

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import (
2121

2222
const maxLines = 2000
2323

24+
// maxReadBytes caps the content returned by read_file / batch_read to prevent
25+
// memory exhaustion from huge files.
26+
const maxReadBytes = 1 << 20 // 1 MiB
27+
2428
type readFileTool struct {
2529
dangerousConfig danger.DangerousConfig
2630
}
@@ -226,6 +230,14 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
226230
}
227231
}
228232

233+
// Preserve the original file's mode when overwriting, so a temp file
234+
// created with default permissions does not change the accessibility
235+
// of an existing file (e.g., making a 0640 file world-readable).
236+
var origMode os.FileMode = 0644
237+
if st, err := os.Stat(args.Path); err == nil {
238+
origMode = st.Mode().Perm()
239+
}
240+
229241
// Atomic write via temp file + rename to prevent TOCTOU symlink races.
230242
// os.CreateTemp creates the file in the same directory (same filesystem),
231243
// and os.Rename atomically replaces the directory entry without following
@@ -241,6 +253,11 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
241253
os.Remove(tmpPath)
242254
return jsonError(fmt.Sprintf("cannot write %q: %v", args.Path, err))
243255
}
256+
if err := tmpFile.Chmod(origMode); err != nil {
257+
tmpFile.Close()
258+
os.Remove(tmpPath)
259+
return jsonError(fmt.Sprintf("cannot set permissions %q: %v", args.Path, err))
260+
}
244261
if err := tmpFile.Close(); err != nil {
245262
os.Remove(tmpPath)
246263
return jsonError(fmt.Sprintf("cannot close temp file: %v", err))
@@ -722,13 +739,16 @@ func isBinary(data []byte) bool {
722739

723740
// readLinesWithCount reads lines from an open file, returning content
724741
// and total line count in a single pass. offset is 1-based, limit caps lines.
742+
// The returned content is capped at maxReadBytes to avoid unbounded memory
743+
// consumption from huge lines or huge limits.
725744
func readLinesWithCount(f *os.File, offset, limit int) (string, int, error) {
726745
var out strings.Builder
727746
scanner := bufio.NewScanner(f)
728747
scanner.Buffer(make([]byte, 1024*1024), 1024*1024)
729748
lineNum := 0
730749
start := offset
731750
end := offset + limit - 1
751+
truncated := false
732752

733753
for scanner.Scan() {
734754
lineNum++
@@ -738,7 +758,17 @@ func readLinesWithCount(f *os.File, offset, limit int) (string, int, error) {
738758
if lineNum > end {
739759
continue // count total even beyond limit
740760
}
741-
out.WriteString(fmt.Sprintf("%d|%s\n", lineNum, scanner.Text()))
761+
line := scanner.Text()
762+
formatted := fmt.Sprintf("%d|%s\n", lineNum, line)
763+
if !truncated && out.Len()+len(formatted) > maxReadBytes {
764+
out.WriteString("... [truncated]\n")
765+
truncated = true
766+
// Continue scanning only to count total lines.
767+
continue
768+
}
769+
if !truncated {
770+
out.WriteString(formatted)
771+
}
742772
}
743773

744774
// If no limit was set (limit=0), continue counting past start
@@ -759,6 +789,10 @@ func confineToCWD(path string) (string, error) {
759789
if err != nil {
760790
return "", fmt.Errorf("cannot determine working directory: %v", err)
761791
}
792+
cwdResolved, err := filepath.EvalSymlinks(cwd)
793+
if err != nil {
794+
return "", fmt.Errorf("cannot resolve working directory: %v", err)
795+
}
762796

763797
// Resolve to absolute path
764798
var abs string
@@ -768,6 +802,34 @@ func confineToCWD(path string) (string, error) {
768802
abs = filepath.Join(cwd, path)
769803
}
770804

805+
// Resolve symlinks so a path that is lexically under CWD but traverses a
806+
// symlink cannot escape (e.g., cwd/link -> /etc, cwd/link/file would
807+
// resolve to /etc/file). If the full path or an intermediate directory
808+
// does not exist yet (common for write_file), walk up to the deepest
809+
// existing ancestor, resolve that, and re-attach the missing suffix.
810+
// Missing directories cannot be symlinks, so they cannot be used to escape.
811+
absResolved := abs
812+
resolved := false
813+
cur := abs
814+
for cur != "/" && cur != "" {
815+
if r, err := filepath.EvalSymlinks(cur); err == nil {
816+
suffix := strings.TrimPrefix(abs, cur)
817+
if suffix == "" {
818+
absResolved = r
819+
} else {
820+
absResolved = r + suffix
821+
}
822+
resolved = true
823+
break
824+
}
825+
cur = filepath.Dir(cur)
826+
}
827+
if !resolved {
828+
// Nothing resolvable along the path (should not happen in practice,
829+
// since / always exists). Fall back to lexical path.
830+
absResolved = abs
831+
}
832+
771833
// Allow paths under ~/.odek/ even when outside CWD — the agent
772834
// frequently writes memory and other state to this directory. The
773835
// carve-out deliberately EXCLUDES odek's trust anchors (config.json,
@@ -779,16 +841,16 @@ func confineToCWD(path string) (string, error) {
779841
home, homeErr := os.UserHomeDir()
780842
if homeErr == nil {
781843
odekPrefix := home + "/.odek/"
782-
if strings.HasPrefix(abs, odekPrefix) {
783-
if isProtectedOdekPath(strings.TrimPrefix(abs, odekPrefix)) {
844+
if strings.HasPrefix(absResolved, odekPrefix) {
845+
if isProtectedOdekPath(strings.TrimPrefix(absResolved, odekPrefix)) {
784846
return "", fmt.Errorf("path %q is a protected odek configuration path and cannot be written by file tools", path)
785847
}
786848
return abs, nil
787849
}
788850
}
789851

790852
// Check that the resolved path is within CWD
791-
if !strings.HasPrefix(abs, cwd+string(filepath.Separator)) && abs != cwd {
853+
if !strings.HasPrefix(absResolved, cwdResolved+string(filepath.Separator)) && absResolved != cwdResolved {
792854
return "", fmt.Errorf("path %q escapes the working directory", path)
793855
}
794856

@@ -986,7 +1048,7 @@ func (t *batchReadTool) readSingle(arg batchReadFileArg) batchReadFileResult {
9861048

9871049
return batchReadFileResult{
9881050
Path: arg.Path,
989-
Content: content,
1051+
Content: wrapUntrusted(arg.Path, content),
9901052
TotalLines: totalLines,
9911053
}
9921054
}
@@ -1169,10 +1231,14 @@ func (t *globTool) Call(argsJSON string) (result string, err error) {
11691231
return jsonError(fmt.Sprintf("invalid glob %q: %v", args.Pattern, err))
11701232
}
11711233
for _, p := range gm {
1172-
info, err := os.Stat(p)
1234+
// Use Lstat so symlinks are not followed to their targets.
1235+
info, err := os.Lstat(p)
11731236
if err != nil {
11741237
continue
11751238
}
1239+
if info.Mode()&os.ModeSymlink != 0 {
1240+
continue
1241+
}
11761242
matches = append(matches, globMatch{
11771243
Path: p,
11781244
Size: info.Size(),

0 commit comments

Comments
 (0)