Skip to content

Commit 8b241be

Browse files
laitingshenglaitingsheng
committed
fix(shields): gate legacy seal, split recovery hint, harden e2e backup
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
1 parent f93e3cc commit 8b241be

4 files changed

Lines changed: 109 additions & 19 deletions

File tree

‎docs/security/best-practices.mdx‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,13 @@ For sensitive workloads, use a reviewed host-side immutability workflow after in
204204

205205
- **DAC permissions (default).** The sandbox user owns `/sandbox/.openclaw` with mode `2770` (setgid `sandbox:sandbox`) and `openclaw.json` with mode `660`, so the agent and its group can read and write config directly. A reviewed host-side immutability workflow should compare the intended ownership and mode with the live sandbox filesystem before treating the config tree as locked.
206206
- **Config integrity hash.** The image includes a SHA256 hash of `openclaw.json`. In the default mutable state, `.config-hash` is sandbox-owned and is not a tamper-proof trust anchor, so startup does not fail closed on that hash. When the hash is root-owned and read-only, startup enforces it and refuses to start if the hash does not match.
207-
- **Content seal under shields up.** When `nemoclaw <name> shields up` runs against a clean lock, it captures a SHA-256 seal of `openclaw.json` and any other locked files into the host-side shields state file. On sealed sandboxes, every `shields status` call recomputes the hash inside the sandbox and surfaces drift on any mismatch, so a host-root tamper that flips perms back to `444 root:root` after rewriting the file is still flagged. Sandboxes that were already locked before this seal landed have no recorded hash; the first `shields up` after upgrade captures one even when the verifier reports a clean lock, and `shields status` shows a one-line notice until that happens. `shields up` refuses to re-seal a tampered baseline; restore the original file or rebuild the sandbox before re-running.
207+
- **Content seal under shields up.**
208+
When `nemoclaw <name> shields up` runs against a clean lock, it captures a SHA-256 seal of `openclaw.json` and any other locked files into the host-side shields state file.
209+
On sealed sandboxes, every `shields status` call recomputes the hash inside the sandbox and surfaces drift on any mismatch, so a host-root tamper that flips perms back to `444 root:root` after rewriting the file is still flagged.
210+
Sandboxes locked before this seal landed have no recorded hash; perm-only verification cannot prove their bytes match the image-original, so the seal is **not** a retroactive proof of integrity for legacy state.
211+
By default, `shields up` refuses to seal such a baseline and asks the operator to rebuild the sandbox first for a known-good baseline.
212+
Operators who explicitly trust the current bytes can opt in via `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE=1`, which captures a seal over the current files and is acknowledged in the log line.
213+
Once a sandbox is sealed, `shields up` refuses to re-seal a tampered baseline; restore the original file or rebuild the sandbox before re-running.
208214
- **Gateway token environment.** The gateway exports `OPENCLAW_GATEWAY_TOKEN` and writes it to `/tmp/nemoclaw-proxy-env.sh` for interactive sandbox sessions. Keep this in mind when deciding whether a workload should run with mutable config or an immutable config posture.
209215

210216
| Aspect | Detail |

‎src/lib/shields/index.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,37 @@ describe("shields — unit logic", () => {
633633
);
634634
});
635635

636+
it("rejects state files whose fileHashes entries are not SHA-256 hex strings", async () => {
637+
const sandboxName = "openclaw";
638+
fs.mkdirSync(stateDir(), { recursive: true });
639+
// Hash value is the right length but contains non-hex chars,
640+
// and another value is far too short. Either alone should fail
641+
// the isOptionalHashMap guard.
642+
fs.writeFileSync(
643+
path.join(stateDir(), `shields-${sandboxName}.json`),
644+
JSON.stringify({
645+
shieldsDown: false,
646+
fileHashes: {
647+
"/sandbox/.openclaw/openclaw.json": "not-a-real-hash",
648+
},
649+
updatedAt: new Date().toISOString(),
650+
}),
651+
);
652+
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
653+
const exitSpy = vi
654+
.spyOn(process, "exit")
655+
.mockImplementation((code?: string | number | null) => {
656+
throw new Error(`exit ${String(code)}`);
657+
});
658+
659+
const { shieldsStatus } = await loadShieldsModule();
660+
expect(() => shieldsStatus(sandboxName)).toThrow("exit 1");
661+
expect(errorSpy).toHaveBeenCalledWith(
662+
" Shields: ERROR (state file is corrupt)",
663+
);
664+
expect(exitSpy).toHaveBeenCalledWith(1);
665+
});
666+
636667
it("status fails fast on corrupt shields state instead of reporting NOT CONFIGURED", async () => {
637668
const sandboxName = "openclaw";
638669
fs.mkdirSync(stateDir(), { recursive: true });

‎src/lib/shields/index.ts‎

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -295,13 +295,19 @@ function isOptionalNullableNumber(
295295
);
296296
}
297297

298+
// SHA-256 hex strings are 64 lowercase or uppercase hex chars. The seal
299+
// helper normalises to lowercase before persisting; accept either case
300+
// here so manually edited state files and legacy uppercase entries still
301+
// load, and reject anything that cannot be a real digest.
302+
const SHA256_HEX_RE = /^[0-9a-f]{64}$/i;
303+
298304
function isOptionalHashMap(
299305
value: unknown,
300306
): value is { [path: string]: string } | undefined {
301307
if (value === undefined) return true;
302308
if (!isObjectRecord(value)) return false;
303309
for (const v of Object.values(value)) {
304-
if (typeof v !== "string") return false;
310+
if (typeof v !== "string" || !SHA256_HEX_RE.test(v)) return false;
305311
}
306312
return true;
307313
}
@@ -1149,11 +1155,37 @@ function shieldsUp(sandboxName: string, opts: { throwOnError?: boolean } = {}):
11491155
expectedHashes: state.fileHashes,
11501156
});
11511157
if (issues.length === 0) {
1152-
// Legacy locked state predates the content seal. Capture one now so
1153-
// future `shields status` calls can detect content drift instead of
1154-
// relying on perm-only verification. We only do this when the
1155-
// verifier was already happy with the on-disk state.
1158+
// Legacy locked state predates the content seal. Capturing a seal
1159+
// now would treat the *current* file content as the trusted
1160+
// baseline, but perm-only verification gives no proof that the
1161+
// bytes match the image-original. A pre-existing content tamper
1162+
// would be sealed in as the new "trusted" content.
1163+
//
1164+
// Refuse by default and ask the operator to either:
1165+
// 1. rebuild the sandbox (clean baseline), then `shields up`;
1166+
// 2. explicitly opt in via `NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE=1`
1167+
// to acknowledge that the current bytes are trusted.
1168+
// Once the operator opts in, the seal is captured and subsequent
1169+
// `shields up`/`shields status` runs detect any future drift.
11561170
if (!state.fileHashes) {
1171+
if (process.env.NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE !== "1") {
1172+
console.error(
1173+
" ERROR: locked sandbox has no content seal (state predates the seal).",
1174+
);
1175+
console.error(
1176+
" Perm-only verification cannot prove the locked files have not already been tampered with.",
1177+
);
1178+
console.error(
1179+
` Recovery: rebuild the sandbox for a known-good baseline, then run \`nemoclaw ${sandboxName} shields up\`.`,
1180+
);
1181+
console.error(
1182+
` Or accept the current bytes as the trusted baseline by setting NEMOCLAW_SHIELDS_ACCEPT_LEGACY_BASELINE=1 and rerunning.`,
1183+
);
1184+
return failShieldsCommand(
1185+
"Locked sandbox has no content seal; refusing to seal a legacy baseline without explicit operator acknowledgement",
1186+
opts.throwOnError,
1187+
);
1188+
}
11571189
try {
11581190
const filesToHash = [
11591191
target.configPath,
@@ -1162,7 +1194,7 @@ function shieldsUp(sandboxName: string, opts: { throwOnError?: boolean } = {}):
11621194
const newHashes = captureSealHashes(sandboxName, filesToHash);
11631195
saveShieldsState(sandboxName, { fileHashes: newHashes });
11641196
console.log(
1165-
" Captured SHA-256 content seal for existing lockdown.",
1197+
" Captured SHA-256 content seal for existing lockdown (current bytes accepted as baseline).",
11661198
);
11671199
} catch (err) {
11681200
const message = err instanceof Error ? err.message : String(err);
@@ -1413,9 +1445,20 @@ function shieldsStatus(
14131445
for (const issue of driftIssues) {
14141446
console.error(` - ${issue}`);
14151447
}
1416-
console.error(
1417-
` Recovery: nemoclaw ${sandboxName} shields up # re-lock and re-verify`,
1418-
);
1448+
// Hash-trust failures cannot be repaired by re-locking — re-up
1449+
// would just seal the tampered or unverifiable content. Perm
1450+
// drift (mode/owner/chattr/legacy-layout) is launderable by
1451+
// re-up. Surface the right recovery for the failure mode.
1452+
const hasHashTrouble = driftIssues.some(isHashVerificationIssue);
1453+
if (hasHashTrouble) {
1454+
console.error(
1455+
` Recovery: restore the original file content from a trusted source, or rebuild the sandbox, then run \`nemoclaw ${sandboxName} shields up\` to re-seal.`,
1456+
);
1457+
} else {
1458+
console.error(
1459+
` Recovery: nemoclaw ${sandboxName} shields up # re-lock and re-verify`,
1460+
);
1461+
}
14191462
process.exit(2);
14201463
}
14211464
console.log(` Shields: ${posture.statusText}`);

‎test/e2e/test-shields-config.sh‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,16 @@ CTR=$(docker ps --filter "name=openshell-${SANDBOX_NAME}" -q | head -n1)
328328
if [ -z "$CTR" ]; then
329329
fail "Could not find sandbox container for ${SANDBOX_NAME}"
330330
else
331-
ORIG_CONTENT=$(docker exec -u 0 "$CTR" cat "$CONFIG_PATH" 2>/dev/null || true)
332-
if [ -z "$ORIG_CONTENT" ]; then
331+
# Use a byte-preserving temp file for backup/restore. Bash command
332+
# substitution `$(...)` strips trailing newlines, which would change
333+
# the file's SHA-256 between backup and restore and create false
334+
# drift after the post-restore status check.
335+
ORIG_CONTENT_FILE=$(mktemp -t nemoclaw-shields-orig.XXXXXX)
336+
trap 'rm -f "$ORIG_CONTENT_FILE"' EXIT
337+
if ! docker exec -u 0 "$CTR" cat "$CONFIG_PATH" >"$ORIG_CONTENT_FILE" 2>/dev/null; then
333338
fail "Could not read original ${CONFIG_PATH} content as host root"
339+
elif [ ! -s "$ORIG_CONTENT_FILE" ]; then
340+
fail "Original ${CONFIG_PATH} read returned an empty file"
334341
else
335342
docker exec -u 0 "$CTR" sh -c \
336343
"chmod 644 ${CONFIG_PATH} && printf ' ' >> ${CONFIG_PATH} && chmod 444 ${CONFIG_PATH}" \
@@ -343,10 +350,13 @@ else
343350
fail "Expected tamper to leave 444 root:root, got: ${PERMS_AFTER_TAMPER}"
344351
fi
345352

346-
set +e
353+
# The script runs with `set -uo pipefail` (no -e), so `$?` after a
354+
# command substitution gives that command's exit code without
355+
# aborting the script. Toggling `set -e` here would interact badly
356+
# with the `fail()` helper, whose `((FAIL++))` returns a non-zero
357+
# exit when FAIL is 0 and would abort under -e.
347358
STATUS_TAMPER_OUTPUT=$(nemoclaw "${SANDBOX_NAME}" shields status 2>&1)
348359
STATUS_TAMPER_EXIT=$?
349-
set -e
350360
echo "$STATUS_TAMPER_OUTPUT"
351361
if [ "$STATUS_TAMPER_EXIT" = "2" ]; then
352362
pass "shields status exits 2 on content drift"
@@ -364,10 +374,8 @@ else
364374
fail "shields status should name the drifted file"
365375
fi
366376

367-
set +e
368377
REUP_OUTPUT=$(nemoclaw "${SANDBOX_NAME}" shields up 2>&1)
369378
REUP_EXIT=$?
370-
set -e
371379
echo "$REUP_OUTPUT"
372380
if [ "$REUP_EXIT" != "0" ]; then
373381
pass "shields up refuses to re-seal a tampered baseline (exit ${REUP_EXIT})"
@@ -381,10 +389,12 @@ else
381389
fi
382390

383391
# Restore the original content as host root so the rest of the suite
384-
# can continue against a clean lock.
385-
docker exec -u 0 "$CTR" sh -c \
392+
# can continue against a clean lock. `docker exec -i` keeps stdin
393+
# open and we stream the backup file straight in — no command
394+
# substitution that would strip trailing newlines.
395+
docker exec -i -u 0 "$CTR" sh -c \
386396
"chmod 644 ${CONFIG_PATH} && cat > ${CONFIG_PATH} && chmod 444 ${CONFIG_PATH}" \
387-
<<<"$ORIG_CONTENT" >/dev/null 2>&1
397+
<"$ORIG_CONTENT_FILE" >/dev/null 2>&1
388398
POST_RESTORE_OUTPUT=$(nemoclaw "${SANDBOX_NAME}" shields status 2>&1 || true)
389399
if echo "$POST_RESTORE_OUTPUT" | grep -q "Shields: UP (lockdown active)"; then
390400
pass "shields status clean after content restore"

0 commit comments

Comments
 (0)