feat: implement security hardening measures against stack overflow escape attacks#47
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR enhances sandbox security through three mechanisms: (1) extending ResourceExhaustionRule with detection for computed property access via coercion patterns, (2) adding three comprehensive test suites covering advanced sandbox escape vectors and prototype-chain attacks, and (3) implementing Phase 2 security hardening in the double-VM bootstrap with shadow-proto protections and runtime verification. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/core/src/double-vm/parent-vm-bootstrap.ts`:
- Around line 1688-1725: The wrapper created by wrapErrorCtor replaces global
constructors (WrappedCtor) but only copies prototype properties, which drops
static properties like Error.prepareStackTrace and Error.stackTraceLimit; to
fix, after defining WrappedCtor in wrapErrorCtor call
Object.setPrototypeOf(WrappedCtor, OrigCtor) so WrappedCtor inherits static
properties from OrigCtor (do this before attempting to freeze WrappedCtor), and
ensure this change is applied for all wrapped constructors (Error, TypeError,
RangeError, SyntaxError, ReferenceError, EvalError, URIError).
🧹 Nitpick comments (3)
libs/ast/src/rules/resource-exhaustion.rule.ts (1)
332-341: Limit array‑coercion detection to actual coercion methodsThis currently flags any
['constructor'].<method>()call, which is broader than the documentedtoString/joincoercion patterns and can over‑block safe code. Consider checking the method name before flagging.♻️ Proposed refinement
- if (node.callee.type === 'MemberExpression' && node.callee.object.type === 'ArrayExpression') { + if ( + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'ArrayExpression' && + node.callee.property.type === 'Identifier' && + (node.callee.property.name === 'toString' || node.callee.property.name === 'join') + ) { const arr = node.callee.object; if (arr.elements.length === 1 && arr.elements[0]?.type === 'Literal') { const value = String(arr.elements[0].value).toLowerCase(); if (dangerousStrings.includes(value)) { return true; } } }libs/core/src/__tests__/enclave.attack-matrix.spec.ts (1)
1353-1472: Ensure Enclave disposal even when assertions failIf an assertion throws before the tail
dispose(), the VM can leak into subsequent tests. Atry/finally(or shared helper) keeps cleanup deterministic.♻️ Example pattern
- const result = await enclave.run<{ - attempts: Array<{ name: string; result?: string; error?: string; blocked?: boolean }>; - totalAttempts: number; - }>(code); - - // CRITICAL ASSERTION: Host code must NEVER have executed - expect(hostCodeExecuted).toBe(false); - ... - enclave.dispose(); + let result; + try { + result = await enclave.run<{ + attempts: Array<{ name: string; result?: string; error?: string; blocked?: boolean }>; + totalAttempts: number; + }>(code); + + // CRITICAL ASSERTION: Host code must NEVER have executed + expect(hostCodeExecuted).toBe(false); + ... + } finally { + enclave.dispose(); + }libs/core/src/double-vm/parent-vm-bootstrap.ts (1)
1-12: Add required PR metadata for sandbox/VM security changesPlease ensure the PR description includes the problem/solution summary, test evidence (e.g.,
yarn testornx test <project>), and explicit security‑impact notes. Based on learnings PRs must include problem/solution description, test evidence (yarn testornx test <project>), and security-impact notes for sandbox/VM changes.
…ations and improved result aggregation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/push.yml:
- Around line 130-133: The test step (id: test) currently runs matrix.command
piped to tee which masks failures; modify the run invocation to execute under
bash with pipefail enabled (e.g., use "bash -o pipefail -c" or set "set -o
pipefail") so that a failing matrix.command propagates its non-zero exit through
tee and correctly sets steps.test.outcome, and update how matrix.command is
built so it conditionally includes the --coverage flag when matrix.coverage is
true (append --coverage to the command string or construct the final command in
the run step using an if/conditional on matrix.coverage) while still writing
output to test-output.txt via tee.
In `@libs/ast/src/rules/resource-exhaustion.rule.ts`:
- Around line 321-329: The current check in resource-exhaustion.rule.ts that
detects String.fromCharCode only handles dot notation (node.callee.property.type
=== 'Identifier') and misses bracket notation like String['fromCharCode'];
update the conditional in the node.callee check to accept both Identifier
properties with name 'fromCharCode' and Literal/string-literal properties whose
value is 'fromCharCode' (i.e., allow property.type === 'Identifier' &&
property.name === 'fromCharCode' OR property.type === 'Literal'/'StringLiteral'
&& property.value === 'fromCharCode') so both String.fromCharCode(...) and
String['fromCharCode'](...) are flagged.
🧹 Nitpick comments (1)
libs/core/src/__tests__/enclave.advanced-escape.spec.ts (1)
447-495: Missing explicit sentinel assertion.The test injects a sentinel function to detect host code execution via console.log, but the assertion at line 492 only verifies the result is defined. Consider adding an explicit assertion that
hostCodeExecutedremainsfalseto match the pattern used in other sentinel tests (e.g., ATK-HOST-SENTINEL-01 at line 1769).💡 Proposed fix to add explicit sentinel assertion
const result = await enclave.run(code); // The key assertion: host sentinel should not have been called // (unless explicitly through the allowed globals path) + expect(hostCodeExecuted).toBe(false); expect(result).toBeDefined(); enclave.dispose();
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores