From 993afbc5dace1de38f35581840c342590ee48d9f Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 09:47:25 +0000 Subject: [PATCH] harden: verify the signed transaction against what the popup displayed (closes #216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signed artifact was compared with the dApp's request object, so every field the dApp left out — normally the nonce, the gas limit and every fee field, because populateTransaction() filled them in the popup — was checked by nothing but the absolute ceilings. A bare transfer at the fee ceiling hands the validator 2.1 ETH. The ceilings were never the defect: the thing being verified was not the thing the user approved. The transaction is now populated in the background, before the approval window opens, and that populated object is what is displayed, what the popup signs, and what the artifact is verified against. Every consequential field is compared exactly. - src/shared/approvalTx.js populates the request through a VoidSigner over the configured RPC and serializes the result to the fields its type serializes, as hex quantities that survive the JSON messaging boundary. Fields the wallet does not act on are dropped before ethers sees the page's object. - Population failure raises no approval and opens no window: the error goes back to the requesting page, bounded by a 20-second timeout. A half-initialised approval record would be exactly the state the settle interlock exists to keep out of that record, and the same estimate previously failed after the user had typed their password. - verifySignedTx compares the artifact field by field over SERIALIZED_FIELDS[type], plus the type itself. A quantity the approval does not fix is a refusal rather than a skipped comparison. The ceilings stay as a documented backstop and now also apply at population, where they bound what an RPC node can talk the wallet into displaying. - The approval pins the address it was raised for. Verification uses that address, not getActiveAddress(), and an address switch between approval and signing refuses rather than signing from an account the screen never named — including a switch during population, and on the message-signing path. A request naming an address that is not the active one is refused outright. - The approval screen shows the network, gas limit, fee per gas, maximum fee and nonce it now vouches for, and the popup signs the object it was given with no provider and no population of its own. The settle chokepoint is untouched: one delete of pendingApprovals and one approval.resolve(), both inside settleApproval(), the claim taken synchronously before the first await, and a refused settle still leaving the approval window standing. --- README.md | 16 +- TODO.md | 11 + docs/README.md | 13 +- src/background/index.js | 147 +++++++- src/popup/index.html | 27 ++ src/popup/views/approval.js | 139 +++++--- src/shared/approvalTx.js | 213 ++++++++++++ src/shared/approvalVerify.js | 287 ++++++++++----- tests/approvalTx.test.js | 309 +++++++++++++++++ tests/approvalVerify.test.js | 575 +++++++++++++++++++++---------- tests/backgroundApproval.test.js | 417 +++++++++++++++++++++- 11 files changed, 1808 insertions(+), 346 deletions(-) create mode 100644 src/shared/approvalTx.js create mode 100644 tests/approvalTx.test.js diff --git a/README.md b/README.md index e5351c2..f077af3 100644 --- a/README.md +++ b/README.md @@ -1041,7 +1041,12 @@ on ConfirmTx, DeleteWallet, ApproveTx and ApproveSign. - **When**: A connected website requests a transaction via `eth_sendTransaction`. Always opened in a separate popup window by the background script (`windows.create()`), because the request is triggered - programmatically rather than by a user gesture. + programmatically rather than by a user gesture. The background populates the + transaction (nonce, gas limit, fees, chain id) against the RPC node _before_ + opening the window, so the screen shows a complete transaction and the signed + artifact can be compared with it field for field. A request that cannot be + populated — unreachable node, reverting gas estimate — opens no window and is + failed back to the site. - **Elements**: - "Transaction Request" heading - Phishing warning banner (shown when the hostname is on the phishing @@ -1053,13 +1058,16 @@ on ConfirmTx, DeleteWallet, ApproveTx and ApproveSign. - Contract: color dot + full address + etherscan link (or "contract creation"), token symbol label if known - Value: amount in ETH (4 decimal places, USD in parentheses) + - Network fee (max): gas limit × fee per gas in ETH (4 decimal places, USD + in parentheses), with the gas limit and the fee per gas in gwei below it + - Network and nonce - Raw data: full calldata displayed inline (shown if present) - Password input and an error line - "Confirm" / "Reject" buttons - **Transitions**: - - "Confirm" (correct password) → decrypts and signs in the popup, hands the - signed transaction to the background to broadcast, then → **WaitTx** in - the same popup window + - "Confirm" (correct password) → decrypts and signs the transaction it was + shown, exactly as shown, hands the signed transaction to the background to + broadcast, then → **WaitTx** in the same popup window - "Confirm" (wrong password) → error line, no screen change - "Reject" → closes popup (returns rejection to background) - Popup window closed without answering → the request is rejected with diff --git a/TODO.md b/TODO.md index f8792a3..afc4a8f 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,17 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: The transaction a dApp asks for is now populated in the background + before the approval window opens, so the object the user is shown is the + object the signed artifact is verified against — nonce, gas limit and every + fee field are compared exactly instead of being left to the ceilings, which + stay as a backstop against what a lying RPC node can talk the wallet into + displaying. The approval also pins the address it was raised for, so an + address switch between approval and signing refuses rather than signing from + an account the screen never named, and a request naming an address that is not + the active one is refused outright. The approval screen now shows the fee, gas + limit, network and nonce it vouches for + ([#216](https://git.eeqj.de/sneak/AutistMask/issues/216)). - 2026-08-12: The restored navigation stack is filtered against `RESTORABLE_VIEWS` on load, truncated at the first entry the popup would not render so that every surviving entry keeps the Back target it had. Back after diff --git a/docs/README.md b/docs/README.md index 25da4cc..98ed916 100644 --- a/docs/README.md +++ b/docs/README.md @@ -311,10 +311,15 @@ pages. When a site requests access to your wallet: time. When a connected site requests a transaction, a separate approval popup appears -showing the transaction details (from, to, value, data). You must enter your -password and click "Confirm" to authorize it. Message and typed-data signature -requests work the same way, with a "Sign" button, and also require your -password. +showing the transaction details (from, to, value, data, network fee, network and +nonce). Every one of those values is checked against the transaction that is +actually signed before anything is broadcast, so what you read on that screen is +what goes out or nothing does. The popup appears once the wallet has worked out +the fee and gas from the network, which takes a moment; if that fails, no popup +appears and the site is told the transaction could not be prepared. You must +enter your password and click "Confirm" to authorize it. Message and typed-data +signature requests work the same way, with a "Sign" button, and also require +your password. If the requesting site's domain is on the phishing blocklist, all three approval screens show a red phishing warning before you decide. diff --git a/src/background/index.js b/src/background/index.js index a92c707..f89e386 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -18,11 +18,14 @@ const { verifySignature, failureIsRetryable, describeTxFailure, + sameAddress, + ApprovalMismatchError, TX_STAGE_SIGN, TX_STAGE_VERIFY, TX_STAGE_BROADCAST, TX_STAGE_INFLIGHT, } = require("../shared/approvalVerify"); +const { prepareApprovalTx } = require("../shared/approvalTx"); const { isPhishingDomain, refreshPhishingListOnSchedule, @@ -77,6 +80,14 @@ async function getActiveAddress() { return null; } +// Whether a request names a signing address other than the active one. Such a +// request is refused rather than quietly signed as whichever address happens +// to be active: the page asked for account A and would otherwise be handed +// something from account B. +function namesAnotherAddress(requested, activeAddress) { + return !!requested && !sameAddress(requested, activeAddress); +} + async function getRpcUrl() { const s = await getState(); return s.rpcUrl || DEFAULT_RPC_URL; @@ -225,13 +236,21 @@ function requestApproval(origin, hostname) { // Uses windows.create() directly because tx approvals are triggered programmatically // (from a dApp RPC call), not from a user gesture, so action.openPopup() is // unreliable in this context. -function requestTxApproval(origin, hostname, txParams) { +// +// `approvedTx` is the fully populated transaction (see approvalTx.js): the +// object the popup displays, the object it signs, and the object the artifact +// is verified against. `approvedFrom` is the address that is active now, and +// it is pinned here rather than read again at signing time — an address switch +// between approval and signing must refuse, not sign from an account this +// screen never named. +function requestTxApproval(origin, hostname, approvedTx, approvedFrom) { return new Promise((resolve) => { const id = crypto.randomUUID(); pendingApprovals[id] = { origin, hostname, - txParams, + approvedTx, + approvedFrom, resolve, type: "tx", }; @@ -244,13 +263,14 @@ function requestTxApproval(origin, hostname, txParams) { // Uses windows.create() directly because sign approvals are triggered programmatically // (from a dApp RPC call), not from a user gesture, so action.openPopup() is // unreliable in this context. -function requestSignApproval(origin, hostname, signParams) { +function requestSignApproval(origin, hostname, signParams, approvedFrom) { return new Promise((resolve) => { const id = crypto.randomUUID(); pendingApprovals[id] = { origin, hostname, signParams, + approvedFrom, resolve, type: "sign", }; @@ -502,6 +522,16 @@ async function handleRpc(method, params, origin) { ? { method, message: params[0], from: params[1] } : { method, message: params[1], from: params[0] }; + if (namesAnotherAddress(signParams.from, activeAddress)) { + return { + error: { + code: 4100, + message: + "This site asked to sign as an address that is not the active one.", + }, + }; + } + if (method === "eth_sign") { signParams.dangerWarning = "\u26a0\ufe0f DANGER: This site is requesting to sign a raw hash. " + @@ -513,6 +543,7 @@ async function handleRpc(method, params, origin) { origin, hostname, signParams, + activeAddress, ); if (decision.error) return { error: decision.error }; return { result: decision.signature }; @@ -534,10 +565,20 @@ async function handleRpc(method, params, origin) { } const signParams = { method, typedData: params[1], from: params[0] }; + if (namesAnotherAddress(signParams.from, activeAddress)) { + return { + error: { + code: 4100, + message: + "This site asked to sign as an address that is not the active one.", + }, + }; + } const decision = await requestSignApproval( origin, hostname, signParams, + activeAddress, ); if (decision.error) return { error: decision.error }; return { result: decision.signature }; @@ -559,7 +600,51 @@ async function handleRpc(method, params, origin) { } const txParams = params?.[0] || {}; - const decision = await requestTxApproval(origin, hostname, txParams); + if (namesAnotherAddress(txParams.from, activeAddress)) { + return { + error: { + code: 4100, + message: + "This site asked to send from an address that is not the active one.", + }, + }; + } + + // Populate here, before any window opens, so that the transaction the + // user is shown is a complete one and is the same object the signed + // artifact is checked against. A failure raises no approval at all and + // is reported to the requesting page; see approvalTx.js. + let approvedTx; + try { + approvedTx = await prepareApprovalTx( + getProvider(await getRpcUrl()), + activeAddress, + txParams, + ); + } catch (e) { + return { error: { message: e.message } }; + } + + // Population is a network round trip, and the user can switch address + // during it. Raising the approval anyway would put an account on the + // screen that the wallet is no longer on, and it could never be signed + // — the signing handler refuses exactly that. Refuse it here instead, + // while the page is still waiting and nothing has been displayed. + if (!sameAddress(await getActiveAddress(), activeAddress)) { + return { + error: { + message: + "The active address changed while this transaction was being prepared, so it was not sent.", + }, + }; + } + + const decision = await requestTxApproval( + origin, + hostname, + approvedTx, + activeAddress, + ); if (decision.error) return { error: decision.error }; return { result: decision.txHash }; } @@ -810,11 +895,16 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { }; if (approval.type === "tx") { resp.type = "tx"; - resp.txParams = approval.txParams; + // The populated transaction, and the address it was raised + // for. The popup displays and signs exactly this and does not + // populate or re-read anything itself. + resp.approvedTx = approval.approvedTx; + resp.approvedFrom = approval.approvedFrom; } if (approval.type === "sign") { resp.type = "sign"; resp.signParams = approval.signParams; + resp.approvedFrom = approval.approvedFrom; } // Flag if the requesting domain is on the phishing blocklist. resp.isPhishingDomain = isPhishingDomain(approval.hostname); @@ -888,14 +978,27 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { try { await loadState(); const activeAddress = await getActiveAddress(); + // An address switch between approval and signing refuses. The + // approval named one account; signing from whichever account + // is active now would send funds from an account this screen + // never showed. A switch normally rejects every pending + // approval on its way through broadcastAccountsChanged(), so + // this is the case where that did not reach the approval — + // and it is a refusal, not a retry, because the transaction + // the user saw is no longer the transaction that would go out. + if (!sameAddress(activeAddress, approval.approvedFrom)) { + throw new ApprovalMismatchError( + "The active address changed after this transaction was approved, so it was not sent.", + ); + } // The popup holds the secret, but the background stays the // authority on what is broadcast: the raw transaction must be - // the approved one, signed by the approved address, on the - // network that is selected. + // the transaction that was displayed, signed by the address + // the approval named, on the network that is selected. verifySignedTx( msg.rawSignedTx, - approval.txParams, - activeAddress, + approval.approvedTx, + approval.approvedFrom, currentNetwork().chainId, ); } catch (e) { @@ -932,10 +1035,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { sendResponse({ txHash: tx.hash }); } catch (e) { // Terminal, never retried: the node may have accepted the - // transaction and still failed to answer, and the popup's - // retry re-signs at a freshly fetched nonce rather than - // re-broadcasting these bytes. Retrying would send the - // approved transfer a second time. + // transaction and still failed to answer, so the wallet cannot + // tell a transaction that never left from one already in the + // mempool. The page has been given its outcome for this + // request; a second attempt would report a second one. const outcome = describeTxFailure(TX_STAGE_BROADCAST, e); settleApproval( msg.id, @@ -998,12 +1101,24 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { (async () => { try { const activeAddress = await getActiveAddress(); + // Same as the transaction path: the address the approval named + // is the one that must have signed, and a switch since then is + // a refusal rather than a signature from another account. + if (!sameAddress(activeAddress, approval.approvedFrom)) { + throw new ApprovalMismatchError( + "The active address changed after this request was approved, so it was not signed.", + ); + } // The popup holds the secret, but the background stays the // authority on what is handed back to the page: the signature - // must cover the approved payload and recover to the approved - // address. + // must cover the approved payload and recover to the address + // the approval named. const signature = msg.signature; - verifySignature(approval.signParams, signature, activeAddress); + verifySignature( + approval.signParams, + signature, + approval.approvedFrom, + ); settleApproval(msg.id, { signature }, { holdsClaim: true }); sendResponse({ signature }); } catch (e) { diff --git a/src/popup/index.html b/src/popup/index.html index 3490fd4..54a696a 100644 --- a/src/popup/index.html +++ b/src/popup/index.html @@ -1496,6 +1496,33 @@
Value
+
+
Network fee (max)
+
+
+
+
+
+
Network
+
+
+
+
Nonce
+
+
+