security: decrypt and sign dApp approvals in the popup (closes #157)
All checks were successful
check / check (push) Successful in 24s
All checks were successful
check / check (push) Successful in 24s
The password no longer crosses the extension messaging boundary: the popup decrypts and signs, and sends only the raw signed transaction or the signature. The background re-derives the signer from the artifact and checks it against the approval it holds before broadcasting, so it is not a blind relay.
This commit was merged in pull request #171.
This commit is contained in:
@@ -5,7 +5,6 @@
|
||||
const { DEFAULT_RPC_URL } = require("../shared/constants");
|
||||
const { SUPPORTED_CHAIN_IDS, networkByChainId } = require("../shared/networks");
|
||||
const { onChainSwitch } = require("../shared/chainSwitch");
|
||||
const { getBytes } = require("ethers");
|
||||
const {
|
||||
state,
|
||||
loadState,
|
||||
@@ -14,8 +13,7 @@ const {
|
||||
} = require("../shared/state");
|
||||
const { refreshBalances, getProvider } = require("../shared/balances");
|
||||
const { debugFetch } = require("../shared/log");
|
||||
const { decryptWithPassword } = require("../shared/vault");
|
||||
const { getSignerForAddress } = require("../shared/wallet");
|
||||
const { verifySignedTx, verifySignature } = require("../shared/approvalVerify");
|
||||
const {
|
||||
isPhishingDomain,
|
||||
updatePhishingList,
|
||||
@@ -725,39 +723,28 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
|
||||
return true;
|
||||
}
|
||||
|
||||
// The popup signs; it reports back here when it could not. Fail the
|
||||
// request the same way this handler used to when it did the signing.
|
||||
if (msg.error) {
|
||||
approval.resolve({ error: { message: msg.error } });
|
||||
sendResponse({ error: msg.error });
|
||||
return false;
|
||||
}
|
||||
|
||||
(async () => {
|
||||
try {
|
||||
await loadState();
|
||||
const activeAddress = await getActiveAddress();
|
||||
let wallet, addrIndex;
|
||||
for (const w of state.wallets) {
|
||||
for (let i = 0; i < w.addresses.length; i++) {
|
||||
if (w.addresses[i].address === activeAddress) {
|
||||
wallet = w;
|
||||
addrIndex = i;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (wallet) break;
|
||||
}
|
||||
if (!wallet) throw new Error("Wallet not found");
|
||||
// TODO(security): Move decryption to popup to avoid sending password via runtime.sendMessage
|
||||
let decrypted = await decryptWithPassword(
|
||||
wallet.encryptedSecret,
|
||||
msg.password,
|
||||
// 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.
|
||||
verifySignedTx(
|
||||
msg.rawSignedTx,
|
||||
approval.txParams,
|
||||
activeAddress,
|
||||
);
|
||||
const signer = getSignerForAddress(
|
||||
wallet,
|
||||
addrIndex,
|
||||
decrypted,
|
||||
);
|
||||
// Best-effort: clear decrypted secret after use.
|
||||
// Note: JS strings are immutable; this nulls the reference but
|
||||
// the original string may persist in memory until GC.
|
||||
decrypted = null;
|
||||
const provider = getProvider(state.rpcUrl);
|
||||
const connected = signer.connect(provider);
|
||||
const tx = await connected.sendTransaction(approval.txParams);
|
||||
const tx = await provider.broadcastTransaction(msg.rawSignedTx);
|
||||
approval.resolve({ txHash: tx.hash });
|
||||
sendResponse({ txHash: tx.hash });
|
||||
} catch (e) {
|
||||
@@ -784,55 +771,23 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
|
||||
return true;
|
||||
}
|
||||
|
||||
// The popup signs; it reports back here when it could not. Fail the
|
||||
// request the same way this handler used to when it did the signing.
|
||||
if (msg.error) {
|
||||
approval.resolve({ error: { message: msg.error } });
|
||||
sendResponse({ error: msg.error });
|
||||
return false;
|
||||
}
|
||||
|
||||
(async () => {
|
||||
try {
|
||||
await loadState();
|
||||
const activeAddress = await getActiveAddress();
|
||||
let wallet, addrIndex;
|
||||
for (const w of state.wallets) {
|
||||
for (let i = 0; i < w.addresses.length; i++) {
|
||||
if (w.addresses[i].address === activeAddress) {
|
||||
wallet = w;
|
||||
addrIndex = i;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (wallet) break;
|
||||
}
|
||||
if (!wallet) throw new Error("Wallet not found");
|
||||
// TODO(security): Move decryption to popup to avoid sending password via runtime.sendMessage
|
||||
let decrypted = await decryptWithPassword(
|
||||
wallet.encryptedSecret,
|
||||
msg.password,
|
||||
);
|
||||
const signer = getSignerForAddress(
|
||||
wallet,
|
||||
addrIndex,
|
||||
decrypted,
|
||||
);
|
||||
// Best-effort: clear decrypted secret after use.
|
||||
// Note: JS strings are immutable; this nulls the reference but
|
||||
// the original string may persist in memory until GC.
|
||||
decrypted = null;
|
||||
|
||||
const sp = approval.signParams;
|
||||
let signature;
|
||||
|
||||
if (sp.method === "personal_sign" || sp.method === "eth_sign") {
|
||||
signature = await signer.signMessage(getBytes(sp.message));
|
||||
} else {
|
||||
// eth_signTypedData_v4 / eth_signTypedData
|
||||
const typedData = JSON.parse(sp.typedData);
|
||||
const { domain, types, message } = typedData;
|
||||
// ethers handles EIP712Domain internally
|
||||
delete types.EIP712Domain;
|
||||
signature = await signer.signTypedData(
|
||||
domain,
|
||||
types,
|
||||
message,
|
||||
);
|
||||
}
|
||||
|
||||
// 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.
|
||||
const signature = msg.signature;
|
||||
verifySignature(approval.signParams, signature, activeAddress);
|
||||
approval.resolve({ signature });
|
||||
sendResponse({ signature });
|
||||
} catch (e) {
|
||||
|
||||
Reference in New Issue
Block a user