Skip to content

Commit 56be5d3

Browse files
jkyberneeesclaude
andcommitted
fix: address verification-protocol findings on security branch
Follow-up fixes from running the AI verification protocol over this branch. Each item was verified against the actual code path and covered by a regression test. - sandbox: forbidden mount prefixes (/home, /root, /var, /run) rejected every legitimate in-workdir volume mount on Linux (workdir lives under /home/<user>); skip a forbidden prefix the workdir itself sits under, keeping out-of-workdir rejection intact. - sandbox: resolve the parent symlink chain and re-check containment so an intermediate symlinked directory cannot escape the working directory. - danger: git global options that take a separate value token (-C, -c, --git-dir, ...) were mistaken for the subcommand, misclassifying `git -C <path> push` / `git -c <cfg> fetch` as non-egress; skip those values. - telegram: documents were saved as "chat<id>_*", which the per-chat media quota glob ("*_chat<id>_*") never matched, letting documents bypass the cap; save as "doc_chat<id>_*". - serve: compare session auth tokens with subtle.ConstantTimeCompare. - config: a malicious project ./odek.json could set sandbox=false / sandbox_readonly=false to disable the sandbox; ignore the weakening direction. - session: fail closed (panic) instead of minting a predictable timestamp-based session ID / 256-bit auth token if crypto/rand fails. - audit: aggregate every <untrusted_content_*> blob in a tool message, not just the first, so a later-blob reused-resource injection is not missed. - telegram tests: stop writing fixtures into the package source tree (t.Chdir into a temp dir). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2013bd7 commit 56be5d3

13 files changed

Lines changed: 286 additions & 39 deletions

File tree

cmd/odek/audit.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,14 @@ func recordTurnAudit(store *session.AuditStore, sessionID string, turn int, user
4242
if m.Role == "tool" {
4343
if hasUntrustedWrapper(m.Content) {
4444
ingestedUntrusted = true
45-
untrustedBodies.WriteString(unwrapUntrusted(m.Content))
46-
untrustedBodies.WriteByte(' ')
47-
if src := untrustedSource(m.Content); src != "" {
48-
untrustedSources = append(untrustedSources, src)
45+
// A tool message may carry several untrusted blobs; aggregate
46+
// 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) {
49+
untrustedBodies.WriteString(body)
50+
untrustedBodies.WriteByte(' ')
4951
}
52+
untrustedSources = append(untrustedSources, untrustedSourcesAll(m.Content)...)
5053
}
5154
}
5255
if m.Role == "assistant" && m.Content != "" {

cmd/odek/serve.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"context"
5+
"crypto/subtle"
56
"embed"
67
"encoding/json"
78
"fmt"
@@ -1078,7 +1079,9 @@ func validateSessionToken(store *session.Store, sess *session.Session, token str
10781079
}
10791080
return sess.AuthToken, true
10801081
}
1081-
if token == sess.AuthToken {
1082+
// Constant-time comparison so an attacker cannot recover the token byte by
1083+
// byte via response-timing differences.
1084+
if subtle.ConstantTimeCompare([]byte(token), []byte(sess.AuthToken)) == 1 {
10821085
return sess.AuthToken, true
10831086
}
10841087
return "", false

cmd/odek/untrusted.go

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -190,27 +190,46 @@ 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 {
199+
matches := reWrapper.FindAllStringSubmatch(s, -1)
200+
if len(matches) == 0 {
201+
return nil
202+
}
203+
bodies := make([]string, 0, len(matches))
204+
for _, m := range matches {
205+
body := strings.TrimPrefix(m[2], "\n")
206+
body = strings.TrimSuffix(body, "\n")
207+
bodies = append(bodies, body)
208+
}
209+
return bodies
210+
}
211+
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 {
222+
srcs = append(srcs, rep.Replace(m[1]))
223+
}
224+
return srcs
225+
}
226+
193227
// hasUntrustedWrapper reports whether s contains a complete nonce'd
194228
// untrusted_content wrapper.
195229
func hasUntrustedWrapper(s string) bool {
196230
return reWrapper.MatchString(s)
197231
}
198232

199-
// untrustedSource extracts the source attribute from an
200-
// <untrusted_content_*> wrapper. It returns the empty string if the input
201-
// is not a wrapped blob.
202-
func untrustedSource(s string) string {
203-
m := reWrapper.FindStringSubmatch(s)
204-
if len(m) < 2 {
205-
return ""
206-
}
207-
src := m[1]
208-
// The source attribute is sanitised by wrapUntrusted; reverse the
209-
// substitutions so it compares cleanly against resource strings.
210-
src = strings.NewReplacer("'", `"`, "‹", "<", "›", ">").Replace(src)
211-
return src
212-
}
213-
214233
// mcpDescriptionWithheld replaces an MCP tool description in which
215234
// prompt-injection patterns were detected.
216235
const mcpDescriptionWithheld = "[odek: description withheld — prompt-injection patterns detected in the MCP server's tool description]"

internal/config/loader.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,21 @@ func LoadConfig(cli CLIFlags) ResolvedConfig {
621621
fmt.Fprintf(os.Stderr, "odek: WARNING: ignoring dangerous section from project config (%s); set it via ~/.odek/config.json\n", ProjectConfigPath())
622622
project.Dangerous = nil
623623
}
624+
// A malicious repo must not be able to turn OFF the sandbox or its
625+
// read-only mode via ./odek.json — that would undo the container isolation
626+
// the operator opted into. Only the weakening direction is ignored; a
627+
// project may still enable the sandbox or request read-only. Other sandbox
628+
// knobs (image, user, network, volumes) keep their global/env/CLI
629+
// precedence and project values are confined elsewhere (volumes are bound
630+
// to the working directory in internal/sandbox).
631+
if project.Sandbox != nil && !*project.Sandbox {
632+
fmt.Fprintf(os.Stderr, "odek: WARNING: ignoring sandbox=false from project config (%s); set sandbox policy via ~/.odek/config.json or the CLI\n", ProjectConfigPath())
633+
project.Sandbox = nil
634+
}
635+
if project.SandboxReadonly != nil && !*project.SandboxReadonly {
636+
fmt.Fprintf(os.Stderr, "odek: WARNING: ignoring sandbox_readonly=false from project config (%s); set it via ~/.odek/config.json or CLI\n", ProjectConfigPath())
637+
project.SandboxReadonly = nil
638+
}
624639

625640
// Start with global, overlay project
626641
cfg := overlayFile(FileConfig{}, global)

internal/config/loader_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,62 @@ func TestLoadConfig_ProjectSystemIgnored(t *testing.T) {
382382
}
383383
}
384384

385+
// TestLoadConfig_ProjectCannotDisableSandbox verifies a malicious repo's
386+
// ./odek.json cannot turn OFF the sandbox or its read-only mode that the
387+
// operator enabled globally.
388+
func TestLoadConfig_ProjectCannotDisableSandbox(t *testing.T) {
389+
dir := t.TempDir()
390+
t.Setenv("HOME", dir)
391+
t.Chdir(dir)
392+
393+
globalDir := filepath.Join(dir, ".odek")
394+
os.MkdirAll(globalDir, 0755)
395+
if err := os.WriteFile(filepath.Join(globalDir, "config.json"), []byte(`{
396+
"sandbox": true,
397+
"sandbox_readonly": true
398+
}`), 0644); err != nil {
399+
t.Fatal(err)
400+
}
401+
402+
if err := os.WriteFile(filepath.Join(dir, "odek.json"), []byte(`{
403+
"sandbox": false,
404+
"sandbox_readonly": false
405+
}`), 0644); err != nil {
406+
t.Fatal(err)
407+
}
408+
409+
cfg := LoadConfig(CLIFlags{})
410+
if !cfg.Sandbox {
411+
t.Error("Sandbox = false, want true (project must not disable the sandbox)")
412+
}
413+
if !cfg.SandboxReadonly {
414+
t.Error("SandboxReadonly = false, want true (project must not disable read-only mode)")
415+
}
416+
}
417+
418+
// TestLoadConfig_ProjectCanEnableSandbox verifies the strip only blocks the
419+
// weakening direction: a project may still turn the sandbox on.
420+
func TestLoadConfig_ProjectCanEnableSandbox(t *testing.T) {
421+
dir := t.TempDir()
422+
t.Setenv("HOME", dir)
423+
t.Chdir(dir)
424+
425+
if err := os.WriteFile(filepath.Join(dir, "odek.json"), []byte(`{
426+
"sandbox": true,
427+
"sandbox_readonly": true
428+
}`), 0644); err != nil {
429+
t.Fatal(err)
430+
}
431+
432+
cfg := LoadConfig(CLIFlags{})
433+
if !cfg.Sandbox {
434+
t.Error("Sandbox = false, want true (project may enable the sandbox)")
435+
}
436+
if !cfg.SandboxReadonly {
437+
t.Error("SandboxReadonly = false, want true (project may enable read-only mode)")
438+
}
439+
}
440+
385441
func TestLoadConfig_ProjectDangerousIgnored(t *testing.T) {
386442
dir := t.TempDir()
387443
t.Setenv("HOME", dir)

internal/danger/classifier.go

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1712,18 +1712,37 @@ func isNetworkEgress(first string, tokens []string) bool {
17121712
// git subcommands that inherently contact a remote.
17131713
if first == "git" {
17141714
// Find the git subcommand, skipping the initial "git" token and any
1715-
// leading path (e.g. /usr/bin/git) or global options.
1715+
// leading path (e.g. /usr/bin/git) or global options. Some global
1716+
// options take a *separate* value token that does not start with "-"
1717+
// (e.g. "git -C <path> push", "git -c <key=val> fetch"); that value
1718+
// must not be mistaken for the subcommand, otherwise a remote-contacting
1719+
// command is misclassified as non-egress and could be auto-allowed.
17161720
sub := ""
17171721
seenGit := false
1722+
skipNext := false
17181723
for _, tok := range tokens {
1719-
if commandName(tok) == "git" {
1724+
if !seenGit && commandName(tok) == "git" {
17201725
seenGit = true
17211726
continue
17221727
}
1723-
if seenGit && !strings.HasPrefix(tok, "-") {
1724-
sub = tok
1725-
break
1728+
if !seenGit {
1729+
continue
1730+
}
1731+
if skipNext {
1732+
skipNext = false
1733+
continue
17261734
}
1735+
if strings.HasPrefix(tok, "-") {
1736+
switch tok {
1737+
case "-C", "-c", "--git-dir", "--work-tree", "--namespace",
1738+
"--exec-path", "--super-prefix", "--config-env":
1739+
// These consume the following token as their value.
1740+
skipNext = true
1741+
}
1742+
continue
1743+
}
1744+
sub = tok
1745+
break
17271746
}
17281747
switch sub {
17291748
case "clone", "fetch", "pull":

internal/danger/classifier_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,12 @@ func TestClassify_NetworkEgress_Commands(t *testing.T) {
164164
{"git clone https://github.com/user/repo", NetworkEgress},
165165
{"git fetch origin", NetworkEgress},
166166
{"git pull origin main", NetworkEgress},
167+
// Global options that take a separate value token must not be mistaken
168+
// for the subcommand (regression: these were misclassified as safe).
169+
{"git -C /repo push origin main", NetworkEgress},
170+
{"git -c http.proxy=http://evil fetch origin", NetworkEgress},
171+
{"git --git-dir /repo/.git push origin", NetworkEgress},
172+
{"git -C /repo -c key=val pull", NetworkEgress},
167173
{"scp file user@remote:/path", NetworkEgress},
168174
{"rsync -avz ./ user@remote:/backup", NetworkEgress},
169175
{"nc example.com 80", NetworkEgress},

internal/sandbox/sandbox.go

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -200,21 +200,49 @@ func sanitizeVolumeMount(vol, workdir string) (string, bool) {
200200
}
201201

202202
// Reject symlinks — they could escape the working directory even if the
203-
// link itself is inside it.
203+
// link itself is inside it. Lstat only inspects the final component.
204204
if info, err := os.Lstat(absHost); err == nil {
205205
if info.Mode()&os.ModeSymlink != 0 {
206206
fmt.Fprintf(os.Stderr, "odek: WARNING: rejecting volume mount %q (symlinks are not allowed)\n", vol)
207207
return "", false
208208
}
209209
}
210210

211-
// Reject forbidden host paths.
211+
// Resolve symlinks in the parent chain and re-check containment so an
212+
// intermediate symlinked directory cannot escape the working directory
213+
// (e.g. workdir/link -> /etc, requested as workdir/link/passwd: the final
214+
// component "passwd" is not itself a symlink, so the Lstat check above
215+
// passes, but the resolved path is outside workdir). Both sides are
216+
// resolved so platforms where the workdir contains symlinks (macOS
217+
// /var -> /private/var) compare canonical paths. When the parent does not
218+
// exist yet, EvalSymlinks fails and the lexical confinement check above
219+
// remains the guarantee.
220+
if parent := filepath.Dir(absHost); parent != absHost {
221+
resolvedParent, perr := filepath.EvalSymlinks(parent)
222+
resolvedWorkdir, werr := filepath.EvalSymlinks(absWorkdir)
223+
if perr == nil && werr == nil {
224+
resolvedHost := filepath.Join(resolvedParent, filepath.Base(absHost))
225+
if !isPathUnder(resolvedHost, resolvedWorkdir) {
226+
fmt.Fprintf(os.Stderr, "odek: WARNING: rejecting volume mount %q (resolved host path %s escapes working directory %s)\n", vol, resolvedHost, resolvedWorkdir)
227+
return "", false
228+
}
229+
}
230+
}
231+
232+
// Reject forbidden host paths. Skip any forbidden prefix that the working
233+
// directory itself sits at or under: the confinement check above already
234+
// bounds the mount to the working directory, and the broad system roots
235+
// (/home, /root, /var, /run) are the normal parents of a project directory.
236+
// Without this exemption every legitimate in-workdir mount on a typical
237+
// Linux host (cwd under /home/<user>) would be rejected, while paths that
238+
// genuinely escape into a forbidden area are still caught — either by the
239+
// confinement check above or by a forbidden prefix the workdir is not under.
240+
sep := string(filepath.Separator)
212241
for _, forbidden := range ForbiddenMountPrefixes {
213-
if absHost == forbidden {
214-
fmt.Fprintf(os.Stderr, "odek: WARNING: rejecting forbidden volume mount %q (host path %s)\n", vol, absHost)
215-
return "", false
242+
if absWorkdir == forbidden || strings.HasPrefix(absWorkdir, forbidden+sep) {
243+
continue
216244
}
217-
if strings.HasPrefix(absHost, forbidden+string(filepath.Separator)) {
245+
if absHost == forbidden || strings.HasPrefix(absHost, forbidden+sep) {
218246
fmt.Fprintf(os.Stderr, "odek: WARNING: rejecting forbidden volume mount %q (host path %s)\n", vol, absHost)
219247
return "", false
220248
}

internal/sandbox/sandbox_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"os"
55
"os/exec"
66
"path/filepath"
7+
"runtime"
78
"strings"
89
"testing"
910
)
@@ -146,6 +147,33 @@ func TestBuildRunArgs_ForbiddenVolumeMountRejected(t *testing.T) {
146147
}
147148
}
148149

150+
// TestBuildRunArgs_InWorkdirMountUnderSystemRootAllowed guards against the
151+
// regression where the broad system-root forbidden prefixes (/home, /root,
152+
// /var, /run) rejected every legitimate in-workdir mount on a typical Linux
153+
// host, where the working directory itself lives under /home/<user>.
154+
func TestBuildRunArgs_InWorkdirMountUnderSystemRootAllowed(t *testing.T) {
155+
for _, workdir := range []string{"/home/alice/project", "/root/project", "/var/lib/app", "/run/app"} {
156+
mount := workdir + "/data:/container/data"
157+
args := BuildRunArgs(Config{Volumes: []string{mount}}, "odek-test", workdir, "alpine:latest")
158+
if !contains(args, mount) {
159+
t.Errorf("in-workdir mount under system root should be allowed for workdir %q\nargs: %v", workdir, args)
160+
}
161+
}
162+
}
163+
164+
// TestBuildRunArgs_SystemRootMountOutsideWorkdirStillRejected confirms the
165+
// system-root protection still fires when the path is NOT inside the workdir.
166+
func TestBuildRunArgs_SystemRootMountOutsideWorkdirStillRejected(t *testing.T) {
167+
// workdir is /, so /etc/secret is lexically "under" the workdir and passes
168+
// confinement, but must still be rejected by the /etc forbidden prefix.
169+
args := BuildRunArgs(Config{Volumes: []string{"/etc/secret:/container/secret"}}, "odek-test", "/", "alpine:latest")
170+
for i, a := range args {
171+
if a == "-v" && i+1 < len(args) && strings.HasPrefix(args[i+1], "/etc/secret:") {
172+
t.Errorf("mount into /etc should be rejected even when workdir is /, found %q", args[i+1])
173+
}
174+
}
175+
}
176+
149177
func TestBuildRunArgs_VolumeOutsideWorkdirRejected(t *testing.T) {
150178
args := BuildRunArgs(Config{
151179
Volumes: []string{"/tmp:/container/tmp"},
@@ -179,6 +207,32 @@ func TestBuildRunArgs_DockerSocketRejected(t *testing.T) {
179207
}
180208
}
181209

210+
// TestBuildRunArgs_IntermediateSymlinkEscapeRejected verifies that a mount
211+
// whose final component is a regular file (passing the Lstat check) but whose
212+
// parent is a symlink pointing outside the working directory is rejected.
213+
func TestBuildRunArgs_IntermediateSymlinkEscapeRejected(t *testing.T) {
214+
if runtime.GOOS == "windows" {
215+
t.Skip("symlink tests skipped on windows")
216+
}
217+
workdir := t.TempDir()
218+
outside := t.TempDir()
219+
if err := os.WriteFile(filepath.Join(outside, "secret"), []byte("x"), 0644); err != nil {
220+
t.Fatal(err)
221+
}
222+
// workdir/link -> outside (a directory symlink that escapes the workdir).
223+
if err := os.Symlink(outside, filepath.Join(workdir, "link")); err != nil {
224+
t.Fatal(err)
225+
}
226+
227+
mount := filepath.Join(workdir, "link", "secret") + ":/container/secret"
228+
args := BuildRunArgs(Config{Volumes: []string{mount}}, "odek-test", workdir, "alpine:latest")
229+
for i, a := range args {
230+
if a == "-v" && i+1 < len(args) && strings.Contains(args[i+1], "secret") {
231+
t.Errorf("mount traversing an intermediate symlink out of workdir should be rejected, found %q", args[i+1])
232+
}
233+
}
234+
}
235+
182236
func TestBuildRunArgs_RelativeVolumeResolvedUnderWorkdir(t *testing.T) {
183237
args := BuildRunArgs(Config{
184238
Volumes: []string{"extra:/container/extra"},

0 commit comments

Comments
 (0)