feat(prekeys): validate companion device-identity (ADV) on fetched bundles - #772
Merged
Merged
Conversation
…ndles When establishing a Signal session from a fetched prekey bundle for a companion device (device != 0), WA Web (SessionApi.createSignalSession -> validateADVwithIdentityKey) cryptographically binds the fetched identity key to the account's ADV chain. We did not: the bundle was built straight from <identity>/<skey>/<key> and the <device-identity> child was ignored, so a malicious relay could substitute a fabricated identity key for a victim's companion device and we would silently set up a session with it. Add wacore::adv::validate_adv_with_identity_key (account-sig over prefix||details||identity, plus device-sig made with the fetched identity over prefix||details||identity||accountKey -- the binding check), and call it on both session-establishment paths: the prekey fetch (node_to_pre_key_bundle_ref) and the retry receipt (process_retry_key_bundle). A present-but-invalid device-identity rejects the bundle (the fetch loop skips that device); a missing one is logged but not fatal, since the live/mock server set that omits it is unverified (WA Web throws). Tries E2EE then business-hosted prefixes instead of replicating the bizHostedDevicesEnabled gating.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds ADV chain signature validation to prevent companion-device key-bundle forgery. A new public validator in ChangesCompanion Device ADV Validation
Sequence Diagram(s)The PR modifies three validation touchpoints (validator library, prekey parsing, retry processing) that all apply the same ADV signature verification logic. Rather than three separate diagrams, the validator diagram above captures the core verification flow that all three layers depend on. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchmark Results67 unchanged benchmark(s)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When establishing a Signal session from a fetched prekey bundle for a companion device (
device != 0), WA Web (WAWebSignalSessionApi.createSignalSession->WAWebAdvSignatureApi.validateADVwithIdentityKey) cryptographically binds the fetched identity key to the account's ADV signature chain. We did not: the bundle was built straight from<identity>/<skey>/<key>and the<device-identity>child was ignored on both the prekey-fetch path and the retry path. A malicious or compromised relay could therefore substitute a fabricated identity key (and signed prekey) for a victim's companion device and we would silently set up a session with the attacker.process_prekey_bundleonly checks the signed-prekey signature against the bundle's own identity key (self-signed), which a forger also controls.This adds
wacore::adv::validate_adv_with_identity_key, mirroringvalidateADVwithIdentityKey:verify(accountSignatureKey, ADV_PREFIX_DEVICE_IDENTITY_ACCOUNT_SIGNATURE || details || fetchedIdentity, accountSignature)verify(fetchedIdentity, ADV_PREFIX_DEVICE_IDENTITY_DEVICE_SIGNATURE || details || fetchedIdentity || accountSignatureKey, deviceSignature)The device signature is made with the fetched identity key, so it's the check that actually binds the fetched identity to the account chain; a relay-substituted identity fails it. Both message constructions were cross-checked against the captured
SignatureApi.js($/M) and against our own pairing direction inpair.rs(which signs the same[6,0]/[6,1]prefixed buffers). It tries the E2EE prefixes then the business-hosted[6,5]/[6,6]set rather than replicating WA Web'sbizHostedDevicesEnabledgating — the prefix is only a domain separator, so accepting whichever the signer used stays sound.Wired into both session-establishment paths:
node_to_pre_key_bundle_ref(prekey fetch) andprocess_retry_key_bundle(retry receipt).Behavior on the absent/invalid cases, and a note for review:
<device-identity>rejects the bundle. On the fetch path the parse loop then skips that device (matches WA Web's KeyBundleInWorker batch path).<device-identity>for a companion is logged but not fatal. WA Web throws here, but the e2e mock server is external and its device-identity behavior is unverified; hard-requiring it could break session establishment against servers/mocks that omit it. This is the one intentional divergence — worth tightening to a hard requirement once verified against the live/mock server. Please watch the E2E job on this PR: if the mock sends a dummy (present-but-invalid) device-identity, this would (correctly, by design) reject it and we'd adjust.Tests:
validate_adv_with_identity_keyhas unit coverage (valid E2EE + hosted accepted; substituted identity, missing device signature, and garbage rejected), plus wiring tests thatparse_prekeys_responsedrops a companion bundle with an unverifiable device-identity and keeps a primary (device 0) bundle regardless.