feat/xwing-key-exchange #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/xwing-key-exchange"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Built on top of https://git.distrust.co/public/steve/pulls/6 (include all its commits too), this branch shall be rebased once PR6 is merged (net new is much smaller!)
Caution Platform counterpart: https://codeberg.org/caution/platform/pulls/396
Careful pre-merge review notes for the net-new X-Wing branch:
This branch is built on #6, so the #6 pre-merge issues still need to be addressed here too if this PR is merged directly: wildcard production CORS for E2P and the default
/service-worker passthrough bypass.steve-js-sdk/package.jsonincludesLICENSEin the packagefileslist and declares"license": "MIT", but the repository/package does not contain aLICENSEfile.npm pack --dry-runconfirms no license file is shipped. Please either add the actual license file or correct the package metadata/files list before publishing/merging.The SDK
npm testscript isnode --test test/*.test.js. On Node 18 this fails because the tests assume global WebCrypto (crypto) is present; the same tests pass withnode --experimental-global-webcrypto --test test/*.test.js. If the intended baseline is a newer Node, please add anenginesconstraint; otherwise make the test script/import setup provide WebCrypto so CI and contributors get reproducible results.TODO: once https://git.distrust.co/public/tee-attestation-js/pulls/1 is merged: update:
steve-js-sdk/package-lock.jsonsteve-js-sdk/package.json->
git+https://git.distrust.co/vincent/tee-attestation-js.git->git+https://git.distrust.co/public/tee-attestation-js.gitSecurity/release review notes for PR #7, reviewing the net-new diff over #6 and keeping the #6 context in mind:
Server-side redirect SSRF from #6 is still present. The upstream reqwest client still uses default redirect behavior, so a configured upstream open redirect can make STEVE fetch cross-origin/internal/link-local URLs despite the initial origin-relative path check. Please set
redirect(Policy::none())or only follow same-origin redirects.Upstream DoS is only partially fixed. Protected response bodies are now capped, which is good, but the reqwest client still has no timeout and plaintext fallback still reads
upstream.bytes().awaitwithout a response cap. Please add timeouts and apply bounded streaming to plaintext forwarding too.Browser clients still do not enforce workload/PCR policy. The Rust SDK now requires expected PCRs, but the JS service worker still reports
expectedPcrPolicy: 'not-checked'and only verifies valid Nitro + session binding. A different valid Nitro enclave can still satisfy browser verification unless clients pin the intended workload identity.The production
distappears stale../scripts/test-production-exclusions.shfailed because the StageX-builtsteve-js-sdk/dist/enclave-sw.jsdiffers from the checked-in dist. The built output contains the new scoped bootstrap support, while committed dist does not. Since npm packsdist, please rebuild/commit it and rerun the production exclusion/repro test.The browser attestation verifier is still a security-critical Git dependency from a personal fork:
git.distrust.co/vincent/tee-attestation-js#d74c7ab..., with no npm tarball integrity. I see the TODO to switch this after the public tee-attestation-js PR lands; I’d treat that as a production release blocker.Secondary hardening:
expectedKeyExchange/passthrough/exclude/enclaveOrigin remain persistently mutable by any controlled same-origin page, so a weaker same-origin page can persistently downgrade X-Wing to X25519 or widen bypass policy. Also, the X-Wing WASM runtime hash is useful against fetch tampering, but should be backed by CI/reproducible-build comparison against the checked-in/package artifact.Verification I ran:
cargo test -p steve --all-targets --all-features(32 passed),cargo test -p steve-sdk --all-targets --all-features(31 passed, 2 ignored),NODE_OPTIONS=--experimental-global-webcrypto npm --prefix steve-js-sdk test(passed), and./scripts/test-production-exclusions.sh(failed due to stale dist).Thanks @anton, consolidating the PR #6/#7 feedback:
dist→ valid; we’ll rebuild and commit the canonical StageX output, then rerun the reproducibility gate.expectedKeyExchangecannot downgrade an X-Wing server: it causes a suite mismatch and failure.--locked --offline --package steve. Adding--bin steveis optional future-proofing.cargo package→ valid source-package hygiene issue, but they are excluded from the production container/npm artifact. Will address this if/when the Rust server crate is published.✅ done:
once https://git.distrust.co/public/tee-attestation-js/pulls/1 is merged and BEFORE we merge this PR, update:steve-js-sdk/package-lock.jsonsteve-js-sdk/package.json->
git+https://git.distrust.co/vincent/tee-attestation-js.git->git+https://git.distrust.co/public/tee-attestation-js.gitKimi 3 report:
🔴 High (documented limitation) — Browser client verifies a genuine Nitro enclave, not the expected workload
steve-js-sdk/src/enclave-sw.js:368-371,:438verifyAttestation(doc, { nonce })is called with no PCR policy; status hardcodesexpectedPcrPolicy: 'not-checked'. The untrusted host terminates TLS and can forward/e2p/v2/sessionto its own Nitro enclave (any AWS account) running a modified steve image that logs plaintext. That enclave produces a fully valid attestation (genuine chain, correct nonce, valid binder), so the browser establishes a "verified" session and sends it LLM API keys and prompts in the clear.This is honestly documented in spec §10.1/§11.2, and the native SDK does require PCR0/1/2 (
crates/steve-sdk/src/lib.rs:1014-1038) — but the browser profile's E2E guarantee currently reduces to "some Nitro enclave."Recommendation: accept expected PCR values via
configure()and pass them toverifyAttestation(tee-attestation-jsalready supportsoptions.pcrs). Decide whether the documented gap is acceptable for merge or should land first.🟡 Medium —
enclaveOriginreconfiguration is unauthenticated, persistent, and fails open to plaintextsteve-js-sdk/src/enclave-sw.js:104,:119,:568-571,:643-677Any same-origin script can postMessage
configure(no source authentication) or write the Cache API config entry directly, changingenclaveOrigin. Requests to the former enclave origin then bypass the tunnel as plaintext (return fetch(request)), and because steve still serves plaintext proxying, the app keeps working — the downgrade is silent. Cache persistence means a one-time XSS / malicious dependency survives remediation. Combined with the finding above, pointingenclaveOriginat an attacker host yields a green-looking, fully "attested" exfiltration channel.Prerequisite is same-origin JS execution (which already sees page-level request data); the marginal impact is persistence + silence.
Recommendation: lock
enclaveOriginafter first explicit configuration (or require it at registration), and/or surface origin changes prominently in the status UI.🟢 Low — Unbounded buffering of protocol responses (browser)
steve-js-sdk/src/enclave-sw.js:165,:347await response.arrayBuffer()with no HTTP-layer size cap; the CBOR decoder's 32 MB limit only applies after the body is fully buffered. The hostile host can return arbitrarily large bodies from/e2p/v2/*and exhaust service-worker memory. The native SDK caps these (64 KB session / 4 KB confirm / 32 MB+64 KB application per spec §10.2); the browser client should enforce the same caps at the HTTP layer.🟢 Low — Test backdoor ships in the production bundle
steve-js-sdk/src/enclave-sw.js:813-839; confirmed present indist/enclave-sw.js:2053self.__ENCLAVE_SW_TEST__exposesgetState()(which reachesstate.session.material— raw session traffic keys),configure,resetState, andsetEstablishSessionon the SW global. Not reachable from page JS today due to SW realm isolation, but it contradicts the "test-only" intent.Recommendation: strip it from production builds (e.g. esbuild
define+ tree-shaken guard).🟢 Low — Error broadcasts leak full request URLs to all window clients
steve-js-sdk/src/enclave-sw.js:218-222notifyClients('error', { message, url: request.url, ... })broadcasts to every controlled window, not just the requester. If tokens/keys ever ride in query strings, they are exposed origin-wide. Recipients are same-origin, hence low.🟢 Low — Key hygiene nits
steve-js-sdk/src/key-exchange.js:93-96: X25519 key generated withextractable: true— unnecessary (WebCrypto public keys are always extractable).steve-js-sdk/src/enclave-sw.js:787-790(resetState): session traffic keys are dropped without thefill(0)treatment the shared secret gets inderiveMaterial— keys linger until GC across rotations/resets.🟢 Low / supply-chain — attestation verifier moved to a personal fork
steve-js-sdk/package.jsontee-attestation-jschanged from the unpinnedpublic/repo to thevincent/fork pinned ate4bf335. Pinning is an improvement, but production attestation code now comes from a personal fork — the fork's diff vs. upstream should be reviewed and merged, since git deps carry no tarballintegrity(integrity rests on the git server + SHA-1).Related, in the pinned dep itself: the X.509 chain check validates signatures + dates but not
basicConstraints/keyUsage. Not exploitable without AWS PKI compromise; defense-in-depth gap.Checked and correct ✅
prefix‖seq), AAD, and confirmation records match spec and shared test vectors. X25519 contributory checks both sides. X-Wing draft-10 implicit rejection, no fallback. Suite pinned on both ends; v1 removed — no downgrade path.crates/steve/src/sessions.rs): AEAD authentication before replay-window slot consumption; correct 64-record sliding-window math; one-shot confirmation; semaphore-bounded admission with pruning.*, POST + Content-Type only, scoped to/e2p); origin-relative upstream URL validation (rejects//, CR/LF, backslash); hop-by-hop andx-e2p-*header filtering; identity-only content encoding on the protected path; bounded bodies; uniform non-leaky error codes.eval-attestationfeature; production Containerfile builds without it; eval JS verifier injected only via--evaland absent fromdist/;tests/production/check-exclusions.mjsenforces this.crates/steve-xwing-wasm): one-shot decapsulation consumed even on malformed input; private key never exported; seed from getrandom, zeroized; runtime SHA-256 of the WASM verified against a build-time constant (digest confirmed to match shipped.wasm).Bottom line: nothing critically broken in the cryptography or protocol state machines. The real exposure is the browser profile's missing workload identity (finding 1) amplified by the silently-persistent reconfiguration path (finding 2) — both documented design gaps rather than implementation bugs. Maintainer decision needed: is "documented" acceptable for merge, or should PCR pinning + config lock-down land first?
Thanks Kimi:
tee-attestation-jspin is already fixed inf2f1354I’m leaving plaintext routing for a separate PR—it needs a proper fail-closed design. I also didn’t add an
enclaveOriginlock here: by itself it remains bypassable and could imply a security boundary it doesn’t provide.I deferred PCR policy, key-zeroization lifecycle work, and licensing changes as broader/separate concerns.
The patch is intentionally small: no new APIs, configuration, dependencies, or abstractions. SDK tests, reproducible StageX builds, production exclusions, and X25519/X-Wing E2E all pass.
Updated docs for steve v2: https://codeberg.org/caution/docs/pulls/28
Pull request closed