Skip to content

Commit e581503

Browse files
jkyberneeesjkyberneees
committed
fix: address remaining security findings on sandbox confinement and audit extraction
- Run sandbox ForbiddenMountPrefixes check on the symlink-resolved host path so it is composed on the same canonical path as the containment check. - Fold unwrapUntrustedAll + untrustedSourcesAll into a single regex pass via extractUntrustedAll; audit.go now calls the combined helper per tool msg. - Promote duplicated resolveDirSymlinks/withinRoot logic into a new shared internal/pathutil package and reuse it from internal/sandbox and internal/resource. - Add pathutil tests and a single-pass extraction consistency test. All existing tests plus race-detector runs pass.
1 parent 5bdb456 commit e581503

7 files changed

Lines changed: 259 additions & 91 deletions

File tree

‎cmd/odek/audit.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func recordTurnAudit(store *session.AuditStore, sessionID string, turn int, user
2727
}
2828

2929
var toolCalls []string
30-
var actionText strings.Builder // agent actions: tool calls + final response
30+
var actionText strings.Builder // agent actions: tool calls + final response
3131
var untrustedBodies strings.Builder
3232
var untrustedSources []string
3333
ingestedUntrusted := false
@@ -44,12 +44,15 @@ func recordTurnAudit(store *session.AuditStore, sessionID string, turn int, user
4444
ingestedUntrusted = true
4545
// A tool message may carry several untrusted blobs; aggregate
4646
// every body and source, not just the first, so a later blob
47-
// cannot smuggle in a reused-resource injection unseen.
48-
for _, body := range unwrapUntrustedAll(m.Content) {
47+
// cannot smuggle in a reused-resource injection unseen. Extract
48+
// both in a single regex pass rather than scanning the payload
49+
// twice.
50+
bodies, srcs := extractUntrustedAll(m.Content)
51+
for _, body := range bodies {
4952
untrustedBodies.WriteString(body)
5053
untrustedBodies.WriteByte(' ')
5154
}
52-
untrustedSources = append(untrustedSources, untrustedSourcesAll(m.Content)...)
55+
untrustedSources = append(untrustedSources, srcs...)
5356
}
5457
}
5558
if m.Role == "assistant" && m.Content != "" {

‎cmd/odek/untrusted.go‎

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -190,45 +190,48 @@ func unwrapUntrusted(s string) string {
190190
return body
191191
}
192192

193-
// unwrapUntrustedAll returns the trimmed body of every <untrusted_content_*>
194-
// wrapper in s. A single tool message may concatenate several blobs (e.g. a
195-
// multi-fetch tool), and the audit divergence check must inspect all of them —
196-
// using only the first match would let an injection arriving in a later blob
197-
// escape detection.
198-
func unwrapUntrustedAll(s string) []string {
193+
// extractUntrustedAll extracts every <untrusted_content_*> wrapper from s in a
194+
// single regex pass, returning the trimmed bodies and the desanitised source
195+
// attributes separately. A single tool message may concatenate several blobs
196+
// (e.g. a multi-fetch tool), and the audit divergence check must inspect all of
197+
// them — using only the first match would let an injection arriving in a later
198+
// blob escape detection.
199+
func extractUntrustedAll(s string) (bodies, sources []string) {
199200
matches := reWrapper.FindAllStringSubmatch(s, -1)
200201
if len(matches) == 0 {
201-
return nil
202+
return nil, nil
202203
}
203-
bodies := make([]string, 0, len(matches))
204+
rep := strings.NewReplacer("'", `"`, "‹", "<", "›", ">")
205+
bodies = make([]string, 0, len(matches))
206+
sources = make([]string, 0, len(matches))
204207
for _, m := range matches {
205208
body := strings.TrimPrefix(m[2], "\n")
206209
body = strings.TrimSuffix(body, "\n")
207210
bodies = append(bodies, body)
208-
}
209-
return bodies
210-
}
211211

212-
// untrustedSourcesAll extracts the (desanitised) source attribute from every
213-
// <untrusted_content_*> wrapper in s.
214-
func untrustedSourcesAll(s string) []string {
215-
matches := reWrapper.FindAllStringSubmatch(s, -1)
216-
if len(matches) == 0 {
217-
return nil
218-
}
219-
rep := strings.NewReplacer("'", `"`, "‹", "<", "›", ">")
220-
srcs := make([]string, 0, len(matches))
221-
for _, m := range matches {
222212
src := rep.Replace(m[1])
223213
// Skip empty sources. An empty source would match every resource as a
224214
// prefix in the audit divergence check (strings.HasPrefix(r, "")), which
225215
// would blind the reused-resource injection heuristic for the whole turn.
226-
if src == "" {
227-
continue
216+
if src != "" {
217+
sources = append(sources, src)
228218
}
229-
srcs = append(srcs, src)
230219
}
231-
return srcs
220+
return bodies, sources
221+
}
222+
223+
// unwrapUntrustedAll returns the trimmed body of every <untrusted_content_*>
224+
// wrapper in s.
225+
func unwrapUntrustedAll(s string) []string {
226+
bodies, _ := extractUntrustedAll(s)
227+
return bodies
228+
}
229+
230+
// untrustedSourcesAll extracts the (desanitised) source attribute from every
231+
// <untrusted_content_*> wrapper in s.
232+
func untrustedSourcesAll(s string) []string {
233+
_, sources := extractUntrustedAll(s)
234+
return sources
232235
}
233236

234237
// hasUntrustedWrapper reports whether s contains a complete nonce'd

‎cmd/odek/untrusted_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,3 +114,35 @@ func TestUntrustedSourcesAll_SkipsEmptySource(t *testing.T) {
114114
t.Fatalf("unwrapUntrustedAll returned %d bodies, want 2: %#v", len(bodies), bodies)
115115
}
116116
}
117+
118+
// TestExtractUntrustedAll_SinglePass verifies that extractUntrustedAll returns
119+
// the same bodies and sources as the separate unwrapUntrustedAll and
120+
// untrustedSourcesAll helpers, proving the single-pass refactoring is correct.
121+
func TestExtractUntrustedAll_SinglePass(t *testing.T) {
122+
combined := wrapUntrusted("https://a.example", "body one") +
123+
wrapUntrusted("", "body two") +
124+
wrapUntrusted("https://b.example", "body three")
125+
126+
wantBodies := unwrapUntrustedAll(combined)
127+
wantSources := untrustedSourcesAll(combined)
128+
129+
gotBodies, gotSources := extractUntrustedAll(combined)
130+
131+
if len(gotBodies) != len(wantBodies) {
132+
t.Fatalf("bodies length mismatch: got %d, want %d", len(gotBodies), len(wantBodies))
133+
}
134+
for i := range wantBodies {
135+
if gotBodies[i] != wantBodies[i] {
136+
t.Errorf("body[%d] = %q, want %q", i, gotBodies[i], wantBodies[i])
137+
}
138+
}
139+
140+
if len(gotSources) != len(wantSources) {
141+
t.Fatalf("sources length mismatch: got %d, want %d", len(gotSources), len(wantSources))
142+
}
143+
for i := range wantSources {
144+
if gotSources[i] != wantSources[i] {
145+
t.Errorf("source[%d] = %q, want %q", i, gotSources[i], wantSources[i])
146+
}
147+
}
148+
}

‎internal/pathutil/pathutil.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// Package pathutil provides small, security-critical helpers for path
2+
// confinement and symlink-aware resolution. These primitives are used by
3+
// multiple packages (resource resolver, sandbox volume validation, file tools,
4+
// etc.) so they are promoted here to avoid drifting near-identical copies.
5+
package pathutil
6+
7+
import (
8+
"os"
9+
"path/filepath"
10+
"strings"
11+
)
12+
13+
// CleanAbs returns the absolute, cleaned form of path. If the absolute path
14+
// cannot be determined, it returns the error.
15+
func CleanAbs(path string) (string, error) {
16+
abs, err := filepath.Abs(path)
17+
if err != nil {
18+
return "", err
19+
}
20+
return filepath.Clean(abs), nil
21+
}
22+
23+
// ResolveDirSymlinks returns the absolute, cleaned path with all directory
24+
// symlinks resolved. The final path component is left untouched so callers can
25+
// still enforce O_NOFOLLOW on it. If a directory component does not exist,
26+
// the original absolute path is returned so the caller can produce a sensible
27+
// "not found" error.
28+
func ResolveDirSymlinks(path string) string {
29+
abs, err := CleanAbs(path)
30+
if err != nil {
31+
return path
32+
}
33+
34+
dir := filepath.Dir(abs)
35+
base := filepath.Base(abs)
36+
37+
resolvedDir, err := filepath.EvalSymlinks(dir)
38+
if err != nil {
39+
return abs
40+
}
41+
return filepath.Join(resolvedDir, base)
42+
}
43+
44+
// WithinRoot reports whether candidate resolves to a path inside root.
45+
// Directory symlinks in candidate are resolved before comparison so a symlinked
46+
// directory outside the workspace cannot bypass confinement; the final
47+
// component is kept unresolved so symlinks to files inside the workspace are
48+
// still visible to callers that reject symlink final components separately.
49+
// The check is separator-aware so "/foo" does not match "/foobar".
50+
//
51+
// If root cannot be symlink-resolved (e.g. it does not exist yet in a test or
52+
// for a not-yet-created working directory), the comparison falls back to the
53+
// lexical absolute path, preserving the original sandbox semantics where the
54+
// resolved re-check was optional.
55+
func WithinRoot(root, candidate string) bool {
56+
absRoot, err := CleanAbs(root)
57+
if err != nil {
58+
return false
59+
}
60+
resolvedRoot := absRoot
61+
if r, err := filepath.EvalSymlinks(absRoot); err == nil {
62+
resolvedRoot = r
63+
}
64+
65+
resolved := ResolveDirSymlinks(candidate)
66+
if resolved == resolvedRoot {
67+
return true
68+
}
69+
return strings.HasPrefix(resolved, resolvedRoot+string(os.PathSeparator))
70+
}

‎internal/pathutil/pathutil_test.go‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package pathutil
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"runtime"
7+
"testing"
8+
)
9+
10+
func TestCleanAbs(t *testing.T) {
11+
got, err := CleanAbs("/foo/bar/../baz")
12+
if err != nil {
13+
t.Fatalf("CleanAbs error: %v", err)
14+
}
15+
if want := filepath.Clean("/foo/baz"); got != want {
16+
t.Errorf("CleanAbs = %q, want %q", got, want)
17+
}
18+
}
19+
20+
func TestResolveDirSymlinks_LeavesFinalComponent(t *testing.T) {
21+
if runtime.GOOS == "windows" {
22+
t.Skip("symlink tests skipped on windows")
23+
}
24+
dir := t.TempDir()
25+
target := filepath.Join(dir, "target")
26+
if err := os.Mkdir(target, 0755); err != nil {
27+
t.Fatal(err)
28+
}
29+
link := filepath.Join(dir, "link")
30+
if err := os.Symlink(target, link); err != nil {
31+
t.Fatal(err)
32+
}
33+
34+
// link/file: directory symlink resolved, final component untouched.
35+
resolved := ResolveDirSymlinks(filepath.Join(link, "file"))
36+
want := filepath.Join(target, "file")
37+
// On macOS the temp dir itself may be reached through a symlink (/var ->
38+
// /private/var), so resolve the target directory before appending the final
39+
// component.
40+
if r, err := filepath.EvalSymlinks(target); err == nil {
41+
want = filepath.Join(r, "file")
42+
}
43+
if resolved != want {
44+
t.Errorf("ResolveDirSymlinks = %q, want %q", resolved, want)
45+
}
46+
}
47+
48+
func TestResolveDirSymlinks_FallsBackWhenParentMissing(t *testing.T) {
49+
dir := t.TempDir()
50+
missing := filepath.Join(dir, "missing", "file")
51+
got := ResolveDirSymlinks(missing)
52+
want, err := CleanAbs(missing)
53+
if err != nil {
54+
t.Fatal(err)
55+
}
56+
if got != want {
57+
t.Errorf("ResolveDirSymlinks missing-parent = %q, want %q", got, want)
58+
}
59+
}
60+
61+
func TestWithinRoot_Lexical(t *testing.T) {
62+
if !WithinRoot("/workspace", "/workspace/extra") {
63+
t.Error("expected /workspace/extra to be under /workspace")
64+
}
65+
if WithinRoot("/workspace", "/tmp") {
66+
t.Error("expected /tmp not to be under /workspace")
67+
}
68+
// Separator-aware: /workspacefoo must not match /workspace prefix.
69+
if WithinRoot("/workspace", "/workspacefoo") {
70+
t.Error("expected /workspacefoo not to be under /workspace")
71+
}
72+
}
73+
74+
func TestWithinRoot_SymlinkEscape(t *testing.T) {
75+
if runtime.GOOS == "windows" {
76+
t.Skip("symlink tests skipped on windows")
77+
}
78+
workdir := t.TempDir()
79+
outside := t.TempDir()
80+
if err := os.Symlink(outside, filepath.Join(workdir, "link")); err != nil {
81+
t.Fatal(err)
82+
}
83+
84+
// workdir/link/secret resolves outside workdir.
85+
if WithinRoot(workdir, filepath.Join(workdir, "link", "secret")) {
86+
t.Error("expected symlinked directory escape to be rejected")
87+
}
88+
}
89+
90+
func TestWithinRoot_MissingRootFallsBack(t *testing.T) {
91+
// The root does not exist. The comparison should fall back to lexical
92+
// paths so legitimate under-root candidates are still accepted.
93+
if !WithinRoot("/home/alice/project", "/home/alice/project/data") {
94+
t.Error("expected under-root candidate to be accepted when root does not exist")
95+
}
96+
if WithinRoot("/home/alice/project", "/tmp") {
97+
t.Error("expected outside candidate to be rejected when root does not exist")
98+
}
99+
}
100+
101+
func TestWithinRoot_Equality(t *testing.T) {
102+
if !WithinRoot("/workspace", "/workspace") {
103+
t.Error("expected root to be considered inside itself")
104+
}
105+
}

‎internal/resource/resource.go‎

Lines changed: 4 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"syscall"
1919
"time"
2020

21+
"github.com/BackendStack21/odek/internal/pathutil"
2122
"github.com/BackendStack21/odek/internal/session"
2223
)
2324

@@ -240,7 +241,7 @@ func (f *FileResolver) Search(ctx context.Context, query string, limit int) ([]R
240241
if len(resources) >= limit {
241242
break
242243
}
243-
if !withinRoot(absRoot, match) {
244+
if !pathutil.WithinRoot(absRoot, match) {
244245
continue
245246
}
246247
rel, _ := filepath.Rel(f.root, match)
@@ -288,8 +289,8 @@ func (f *FileResolver) Load(ctx context.Context, id string) (string, error) {
288289
// workspace/link -> /etc) to read files outside the workspace via
289290
// @link/passwd. The final path component is left unresolved so the
290291
// O_NOFOLLOW open below still rejects symlink final components.
291-
resolvedTarget := resolveDirSymlinks(target)
292-
if !withinRoot(f.root, resolvedTarget) {
292+
resolvedTarget := pathutil.ResolveDirSymlinks(target)
293+
if !pathutil.WithinRoot(f.root, resolvedTarget) {
293294
return "", fmt.Errorf("resource: path %q is outside root", id)
294295
}
295296

@@ -372,51 +373,6 @@ func skipDir(name string) bool {
372373
return false
373374
}
374375

375-
// resolveDirSymlinks returns the absolute, cleaned path with all directory
376-
// symlinks resolved. The final path component is left untouched so callers can
377-
// still enforce O_NOFOLLOW on it. If a directory component does not exist, the
378-
// original absolute path is returned (so the caller can produce a sensible
379-
// "not found" error).
380-
func resolveDirSymlinks(path string) string {
381-
abs, err := filepath.Abs(path)
382-
if err != nil {
383-
return path
384-
}
385-
abs = filepath.Clean(abs)
386-
387-
dir := filepath.Dir(abs)
388-
base := filepath.Base(abs)
389-
390-
resolvedDir, err := filepath.EvalSymlinks(dir)
391-
if err != nil {
392-
return abs
393-
}
394-
return filepath.Join(resolvedDir, base)
395-
}
396-
397-
// withinRoot reports whether candidate resolves to a path inside root.
398-
// Directory symlinks in candidate are resolved before comparison so a symlinked
399-
// directory outside the workspace cannot bypass confinement; the final
400-
// component is kept unresolved so symlinks to files inside the workspace are
401-
// still reported by Search (their content is rejected by Load with O_NOFOLLOW).
402-
// The check is separator-aware so "/foo" does not match "/foobar".
403-
func withinRoot(root, candidate string) bool {
404-
absRoot, err := filepath.Abs(root)
405-
if err != nil {
406-
return false
407-
}
408-
absRoot, err = filepath.EvalSymlinks(absRoot)
409-
if err != nil {
410-
return false
411-
}
412-
413-
resolved := resolveDirSymlinks(candidate)
414-
if resolved == absRoot {
415-
return true
416-
}
417-
return strings.HasPrefix(resolved, absRoot+string(os.PathSeparator))
418-
}
419-
420376
func describeFile(info os.FileInfo) string {
421377
size := info.Size()
422378
switch {

0 commit comments

Comments
 (0)