security: the user's plaintext password crosses the extension messaging boundary during dApp approvals #157
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/AutistMask#157
Reference in New Issue
Block a user
Problem
For dApp-initiated transaction and signature approvals, the approval popup
sends the user's plaintext password to the background script over
runtime.sendMessage, and the background does the decryption there.Senders:
src/popup/views/approval.js:449(AUTISTMASK_TX_RESPONSE)src/popup/views/approval.js:488(AUTISTMASK_SIGN_RESPONSE)Consumers:
src/background/index.js:745-748src/background/index.js:804-807The codebase already carries four standing markers for this, all identical:
src/popup/views/approval.js:448src/popup/views/approval.js:487src/background/index.js:744src/background/index.js:803>
// TODO(security): Move decryption to popup to avoid sending password via runtime.sendMessageThe correct pattern already exists in-tree: the popup-side
eth_sendTransactionpath decrypts locally at
src/popup/views/confirmTx.js:305and never puts thepassword on the wire. Only the dApp approval paths diverge.
This is a pre-1.0 hardening item and is explicitly in scope for the
"Pre-1.0 security review" carried in
TODO.md.Implementation requirements
paths, mirroring
confirmTx.js:305. The popup should derive the signer andproduce the signed artifact locally.
natural answer is the signed result (raw signed transaction, or the
signature) rather than the password or the private key. Under no
circumstances should the private key or the recovery phrase replace the
password on the wire — that would be strictly worse.
pending approval back to the requesting page; only the secret handling
moves.
TODO(security)comments once the flaw is actually gone —not before.
retaining it in any closure longer than necessary.
EIP-1193 error code 4001, and approvals must still survive popup
close/reopen as they do today.
src/shared/log.jsoutput on any path,including
debugFetch.Definition of done
runtime.sendMessagepayload anywhere in the codebase contains apassword field. Demonstrate with a grep in the PR description.
eth_sendTransactionapproval still works end to end andthe site receives the tx hash.
personal_signandeth_signTypedData_v4approvals stillwork end to end and the site receives a valid signature.
not resolve the pending approval.
TODO(security)comments are removed.that Firefox verification is blocked on the Firefox compat issue.
TODO.mdupdated in the same commit.make checkpasses.Manager note — dispatching this now as the second work unit of the 1.0.0 push,
ahead of the rest of the milestone. #149 (the hardcoded recovery phrase) is
merge-ready as PR #169 and awaiting merge; this is the next-highest security
item.
Notes for the implementer beyond the issue body:
1. Branch from
main, and expect a smallTODO.mdconflict. PR #169 isopen and unmerged, and it also touches
TODO.md. That is expected and fine —keep your
TODO.mdedit surgical (touch only the lines you actually need)so the rebase is trivial. If #169 has landed by the time you branch, rebase
onto it first and refresh the Status/Next Step block, which will still say
"Land #149 ... PR open, awaiting review" from that PR.
2. Do not let the fix make things worse. The obvious wrong turn here is to
stop sending the password and start sending the decrypted private key or the
recovery phrase instead. That is strictly worse than the current state. What
should cross the boundary is the signed artifact — the raw signed
transaction, or the signature — with the background retaining only broadcast
and approval-resolution duties. The issue body says this; I am repeating it
because it is the one way this change can regress security while looking like
a fix.
3.
confirmTx.js:305is the reference implementation. The popup-sideeth_sendTransactionpath already decrypts locally and never puts the passwordon the wire. Match that shape rather than inventing a new one; the goal is that
all three signing paths look the same afterwards.
4. Removing the four
TODO(security)comments is part of the DoD, but onlyonce the flaw is actually gone. Do not delete them as a tidy-up if you end up
partially implementing this.
5. Firefox verification is expected to be blocked. #153 documents that the
Firefox target is currently non-functional for exactly these approval paths
(Chrome callback APIs against the promise-only
browsernamespace). Verify onChrome, and state plainly in the PR that Firefox verification is blocked on
#153 rather than claiming it works. Do not try to fix #153 here — that is a
separate unit and folding it in would make this PR unreviewable.
6. Watch the logging path. The DoD requires proving no password reaches
src/shared/log.jsoutput, includingdebugFetch. Note that after #149 lands,a release build has
DEBUGfalse but the #145 runtime toggle can still raisethe log level at runtime — so "it isn't logged because DEBUG is off" is not an
acceptable answer. Check the code path, not the current flag value.
Context:
make checkis green onmainat23aeae4. Also notescript/lintis still only
prettier --checkand cannot catch undefined identifiers (#152),so do not rely on it to tell you a refactor is complete — grep.
Implementation plan
Branching from
mainat23aeae4. Branch:fix/issue-157-approval-decrypt-in-popup.What crosses the messaging boundary afterwards
AUTISTMASK_TX_RESPONSEcarriesrawSignedTx— the RLP-serialized,already-signed transaction (a hex string). This is exactly the value that
would have gone out over
eth_sendRawTransactionanyway; it is public themoment it is broadcast, so it is not a secret.
AUTISTMASK_SIGN_RESPONSEcarriessignature— the 65-byte signature hex.Same argument: it is the artifact the dApp receives regardless.
private key ever leaves the popup context.
Popup (
src/popup/views/approval.js)details.txParams/details.signParamsalongside the existingpendingTxDetailswhen the approval view renders, so the popup signs thevery parameters it displayed.
confirmTx.js:305: read the inline password, look up thewallet and address index owning
state.activeAddress, calldecryptWithPassword, thengetSignerForAddress.signer.connect(getProvider(state.rpcUrl)), thenpopulateTransaction+signTransaction. This is the exact sequenceethers' own
AbstractSigner.sendTransactionperforms internally, so gas,nonce, fee and chainId population are unchanged from today.
signMessage(getBytes(sp.message))forpersonal_sign/eth_sign,signTypedData(domain, types, message)foreth_signTypedData_v4/eth_signTypedData— moved verbatim from thebackground.
use, with the same best-effort caveat comment
confirmTx.jsalreadycarries (JS strings are immutable).
full-sentence inline error, re-enables the button, and sends no message at
all — so the pending approval survives and can be retried. That is strictly
better than today, where a bad password reached the background and resolved
(and thus destroyed) the approval.
Background (
src/background/index.js)AUTISTMASK_TX_RESPONSE:provider.broadcastTransaction(msg.rawSignedTx),then resolve the approval and
sendResponsewith the hash. Rejection pathuntouched, still EIP-1193 4001.
AUTISTMASK_SIGN_RESPONSE: resolve the approval withmsg.signature.decryptWithPassword/getSignerForAddressimports.Keeping the background authoritative
Moving the signing out of the background must not turn the background into a
blind relay, so before broadcasting/resolving it verifies the artifact against
the approval it is holding, in a new
src/shared/approvalVerify.js:verifySignedTx(rawSignedTx, txParams, expectedFrom)— parses the rawtransaction with
ethers.Transaction.from, recovers the sender from thesignature, and asserts the recovered
from, plusto,valueanddata,match the approved parameters.
verifySignature(signParams, signature, expectedFrom)— recovers the signerwith
verifyMessage/verifyTypedDataand asserts it is the activeaddress.
Both throw full-sentence errors. This is what makes the change security-neutral
on the background side rather than a trust transfer.
Tests
tests/approvalVerify.test.js: sign real transactions and messages with afixed-key
ethers.Walletand assert the verifiers accept matching artifactsand reject a tampered recipient, a tampered value, tampered call data, a
wrong signer, and a garbage payload.
Logging
Nothing in either changed path passes a password (or the decrypted secret) to
log.*or todebugFetch. After the change the password never exists in thebackground context at all, and in the popup it is only ever an argument to
decryptWithPassword. This will be argued from the code path, not from thevalue of the
DEBUGflag, since the #145 runtime toggle can raise the level atruntime.
Out of scope
on Chrome only; the PR will say so plainly rather than claim Firefox works.
TODO(security)comments come out in the same commit, once the flaw isactually gone.
TODO.mdgets a surgical edit in the same commit (PR #169 is open against it).Landed on
nextvia #171.Carried forward, so it is not lost when this closes: DoD items 2-5 want an interactive Chrome pass over
eth_sendTransaction,personal_signandeth_signTypedData_v4, and that was never run — the code path is covered by unit tests and a round trip only. The browser harness that can supply it is now onnext, and driving these round trips through it is #183.