Skip to content

Commit 83750ae

Browse files
fix(sandbox): drop the unfollowable sandbox override from ACL failures
Both unelevated ACL failures told the reader to re-run with `--sandbox forbid`. There is no such option: SandboxPreferenceForbid is an internal engine state with no flag behind it, so acting on it produced an unknown option and left them stuck on the failure they had just been told how to clear. Advice that does not work costs more than none, because finding that out takes the reader's time. Name the real way out instead, the user config key, which is honored from global config only so a cloned repo cannot set it. The elevated-setup remedy beside it was already correct and stays. Reported by jatmn against the same string on #640. It predates this branch, having arrived with the unelevated fallback tier in #427, and the copy on #886 is fixed separately in 1b304e1. Also covers the secret write with the junction regression it was owed: the caller owns the sandbox home, so they can put a reparse point where the secret directory is expected, and the pathname version followed it in an elevated process. The test asserts the refusal names the reparse point and that nothing survives on the far side, since refusing while still creating the file would leave the caller holding it.
1 parent ffe71ed commit 83750ae

2 files changed

Lines changed: 81 additions & 2 deletions

File tree

‎internal/sandbox/windows_command_runner_windows.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,23 @@ func ensureWindowsUnelevatedSetup(config WindowsSandboxCommandConfig) error {
229229
// is only recorded on success, so the same plan fails identically on
230230
// every later command until the offending root leaves it. A reader who
231231
// cannot tell which root is at fault has no way out of that.
232+
// Every remedy named below is one the reader can actually carry out.
233+
// Both messages used to offer `--sandbox forbid`, which is not an option
234+
// at all: SandboxPreferenceForbid is an internal engine state with no flag
235+
// behind it, so acting on it produced an unknown option and left the reader
236+
// stuck on the failure they had just been told how to clear. Advice that
237+
// does not work costs more than none, since finding that out takes time.
232238
if denied := windowsACLPlanDeniedPath(err); denied != "" {
233239
return fmt.Errorf("apply unelevated workspace ACLs: %w; %s cannot have its permissions changed by this user, "+
234240
"so the sandbox cannot enforce a write boundary there and will not run the command. "+
235241
"That path is one of this workspace's sandbox roots, usually a system directory that arrived via TEMP or TMP. "+
236-
"Check those, or re-run with `--sandbox forbid` to skip OS sandboxing. "+
242+
"Check those, or turn the sandbox off in your user config with "+
243+
`"sandbox": {"enabled": false}. `+
237244
"Running `zero sandbox setup` elevated will NOT fix this", err, denied)
238245
}
239246
return fmt.Errorf("apply unelevated workspace ACLs: %w — the workspace may be on a filesystem the current user does not own; "+
240-
"run `zero sandbox setup` from an elevated (Administrator) terminal, or re-run with `--sandbox forbid` to skip OS sandboxing", err)
247+
"run `zero sandbox setup` from an elevated (Administrator) terminal, "+
248+
`or turn the sandbox off in your user config with "sandbox": {"enabled": false}`, err)
241249
}
242250
return recordWindowsUnelevatedAppliedPlan(config.SandboxHome, applied)
243251
}
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
//go:build windows
2+
3+
package sandbox
4+
5+
import (
6+
"os"
7+
"path/filepath"
8+
"strings"
9+
"testing"
10+
)
11+
12+
// THE SECRET MUST NOT BE WRITTEN THROUGH A REPARSE POINT.
13+
//
14+
// The sandbox home belongs to the invoking user, who is the party this sandbox
15+
// exists to contain, so they can put a junction where the secret directory is
16+
// expected before elevated setup runs. The pathname version resolved the path
17+
// four times over (MkdirAll, an O_TRUNC open, SetNamedSecurityInfo by name, then
18+
// WriteFile by name) and every one of them followed it, so an Administrator
19+
// process created the principal's password in a directory of the caller's
20+
// choosing and then granted them control of it.
21+
//
22+
// A junction rather than a symlink on purpose: it needs no privilege, so it is
23+
// reachable by exactly the unprivileged user this guards against. os.Symlink
24+
// would need SeCreateSymbolicLinkPrivilege and would skip on an ordinary
25+
// developer or CI account, which is where this most needs to run.
26+
func TestWriteWindowsSandboxSecretRefusesAJunctionedDirectory(t *testing.T) {
27+
base := t.TempDir()
28+
outside := filepath.Join(base, "outside")
29+
if err := os.MkdirAll(outside, 0o700); err != nil {
30+
t.Fatalf("MkdirAll: %v", err)
31+
}
32+
// The swap: the directory the secret is addressed into is a junction that
33+
// leads out of the sandbox home. The pathname never changes.
34+
cfg := filepath.Join(base, "cfg")
35+
makeJunction(t, cfg, outside)
36+
37+
path := filepath.Join(cfg, "zero-sbx-test.secret")
38+
err := writeWindowsSandboxSecret(path, "Zs1!EXAMPLEPASSWORDVALUE")
39+
if err == nil {
40+
t.Fatal("wrote the principal secret through a junction, so an elevated setup would have placed it in a directory the caller controls")
41+
}
42+
if !strings.Contains(err.Error(), "reparse") {
43+
t.Fatalf("refused for the wrong reason: %v", err)
44+
}
45+
// Refusing is only half of it. Nothing may survive on the other side, or the
46+
// caller still ends up holding a file the sandbox created for them.
47+
entries, readErr := os.ReadDir(outside)
48+
if readErr != nil {
49+
t.Fatalf("ReadDir: %v", readErr)
50+
}
51+
if len(entries) != 0 {
52+
t.Fatalf("junction target holds %d entries after the refusal, want none", len(entries))
53+
}
54+
}
55+
56+
// The ordinary path must still work, or the refusal above proves only that the
57+
// function fails.
58+
func TestWriteWindowsSandboxSecretStillWritesAnOrdinaryDirectory(t *testing.T) {
59+
path := filepath.Join(t.TempDir(), "cfg", "zero-sbx-test.secret")
60+
const password = "Zs1!EXAMPLEPASSWORDVALUE"
61+
if err := writeWindowsSandboxSecret(path, password); err != nil {
62+
t.Fatalf("write: %v", err)
63+
}
64+
got, err := readWindowsSandboxSecret(path)
65+
if err != nil {
66+
t.Fatalf("read: %v", err)
67+
}
68+
if got != password {
69+
t.Fatalf("read %q, want the stored password", got)
70+
}
71+
}

0 commit comments

Comments
 (0)