security: decrypt and sign dApp approvals in the popup (closes #157)
All checks were successful
check / check (push) Successful in 28s
All checks were successful
check / check (push) Successful in 28s
The dApp transaction and signature approval paths sent the user's plaintext password to the background over runtime.sendMessage and decrypted there. Both now decrypt in the popup, where the password is typed, and put only the signed artifact on the wire: the raw signed transaction, or the signature. Neither the password, the recovery phrase, the xprv nor the private key crosses the messaging boundary any more. This matches what the popup-side eth_sendTransaction path in confirmTx.js already did. The popup runs the same sequence ethers' own sendTransaction() runs internally (populateTransaction, then signTransaction), so nonce, gas, fee and chain id population are unchanged. The background keeps broadcast and approval resolution, and when the popup cannot produce an artifact it reports the error over the same message so the requesting page still gets a failure rather than hanging. Moving the secret out of the background must not turn the background into a blind relay, so it re-derives the signer from the artifact and checks it against the approval it is holding before acting: shared/approvalVerify.js asserts that a raw transaction is the approved transaction signed by the approved address, and that a signature covers the approved payload and recovers to the approved address. A wrong password is now caught in the popup before anything is sent, so it fails with an inline full-sentence error and leaves the pending approval alive to retry; previously it reached the background and destroyed the approval. Rejection still resolves with EIP-1193 code 4001, and approvals still survive popup close and reopen. Removes the four standing TODO(security) markers, now that the flaw is gone.
This commit is contained in:
124
src/shared/approvalVerify.js
Normal file
124
src/shared/approvalVerify.js
Normal file
@@ -0,0 +1,124 @@
|
||||
// Verification of the signed artifacts produced by the approval popup.
|
||||
//
|
||||
// Signing happens in the popup, where the password is entered; the background
|
||||
// only broadcasts the raw transaction and resolves the pending approval back
|
||||
// to the requesting page. So that moving the signing out of the background
|
||||
// does not turn the background into a blind relay, the background re-derives
|
||||
// the signer from the artifact and checks it against the approval it is
|
||||
// holding before acting on it. All recovery is delegated to ethers.
|
||||
//
|
||||
// Every failure message is a full sentence, because these strings are shown to
|
||||
// the user and returned to the dApp.
|
||||
|
||||
const {
|
||||
Transaction,
|
||||
getAddress,
|
||||
getBytes,
|
||||
verifyMessage,
|
||||
verifyTypedData,
|
||||
} = require("ethers");
|
||||
|
||||
// Case-insensitive address comparison that tolerates absent values on either
|
||||
// side. Two absent addresses compare equal (contract creation has no `to`).
|
||||
function sameAddress(a, b) {
|
||||
const aMissing = a === null || a === undefined || a === "";
|
||||
const bMissing = b === null || b === undefined || b === "";
|
||||
if (aMissing || bMissing) return aMissing && bMissing;
|
||||
try {
|
||||
return getAddress(a) === getAddress(b);
|
||||
} catch {
|
||||
return String(a).toLowerCase() === String(b).toLowerCase();
|
||||
}
|
||||
}
|
||||
|
||||
// Normalize a transaction value (hex string, decimal string, number or
|
||||
// bigint) to a bigint. An absent value is zero, matching ethers.
|
||||
function normalizeValue(v) {
|
||||
if (v === null || v === undefined || v === "") return 0n;
|
||||
return BigInt(v);
|
||||
}
|
||||
|
||||
// Normalize call data to a lowercase hex string. Absent data is "0x".
|
||||
function normalizeData(v) {
|
||||
if (v === null || v === undefined || v === "" || v === "0x") return "0x";
|
||||
return String(v).toLowerCase();
|
||||
}
|
||||
|
||||
// Assert that a raw signed transaction is the transaction the user approved,
|
||||
// signed by the address the approval was raised for. Returns the parsed
|
||||
// ethers Transaction on success, throws otherwise.
|
||||
function verifySignedTx(rawSignedTx, txParams, expectedFrom) {
|
||||
if (typeof rawSignedTx !== "string" || !rawSignedTx.startsWith("0x")) {
|
||||
throw new Error("The signed transaction is missing or malformed.");
|
||||
}
|
||||
|
||||
let parsed;
|
||||
try {
|
||||
parsed = Transaction.from(rawSignedTx);
|
||||
} catch {
|
||||
throw new Error("The signed transaction could not be decoded.");
|
||||
}
|
||||
|
||||
if (!parsed.from) {
|
||||
throw new Error("The signed transaction carries no valid signature.");
|
||||
}
|
||||
if (!sameAddress(parsed.from, expectedFrom)) {
|
||||
throw new Error(
|
||||
"The signed transaction was signed by a different address than the one that was approved.",
|
||||
);
|
||||
}
|
||||
if (!sameAddress(parsed.to, txParams.to)) {
|
||||
throw new Error(
|
||||
"The signed transaction does not go to the approved recipient.",
|
||||
);
|
||||
}
|
||||
if (normalizeValue(parsed.value) !== normalizeValue(txParams.value)) {
|
||||
throw new Error(
|
||||
"The signed transaction does not carry the approved value.",
|
||||
);
|
||||
}
|
||||
if (normalizeData(parsed.data) !== normalizeData(txParams.data)) {
|
||||
throw new Error(
|
||||
"The signed transaction does not carry the approved call data.",
|
||||
);
|
||||
}
|
||||
|
||||
return parsed;
|
||||
}
|
||||
|
||||
// Assert that a signature over the approved message or typed data was
|
||||
// produced by the address the approval was raised for. Returns the recovered
|
||||
// address on success, throws otherwise.
|
||||
function verifySignature(signParams, signature, expectedFrom) {
|
||||
if (typeof signature !== "string" || !signature.startsWith("0x")) {
|
||||
throw new Error("The signature is missing or malformed.");
|
||||
}
|
||||
|
||||
let recovered;
|
||||
try {
|
||||
if (
|
||||
signParams.method === "personal_sign" ||
|
||||
signParams.method === "eth_sign"
|
||||
) {
|
||||
recovered = verifyMessage(getBytes(signParams.message), signature);
|
||||
} else {
|
||||
const typedData = JSON.parse(signParams.typedData);
|
||||
const { domain, types, message } = typedData;
|
||||
// ethers derives EIP712Domain itself and rejects it as an input.
|
||||
delete types.EIP712Domain;
|
||||
recovered = verifyTypedData(domain, types, message, signature);
|
||||
}
|
||||
} catch {
|
||||
throw new Error("The signature could not be verified.");
|
||||
}
|
||||
|
||||
if (!sameAddress(recovered, expectedFrom)) {
|
||||
throw new Error(
|
||||
"The signature was produced by a different address than the one that was approved.",
|
||||
);
|
||||
}
|
||||
|
||||
return recovered;
|
||||
}
|
||||
|
||||
module.exports = { verifySignedTx, verifySignature, sameAddress };
|
||||
Reference in New Issue
Block a user