harden: escape every interpolation into popup innerHTML, and add default-src to both manifests (closes #307)
A hostile ERC-20's symbol() reached an innerHTML string unescaped, and neither manifest declared default-src, so an attacker deploying a token with 1,000+ holders and airdropping one unit could render a full-viewport cross-origin iframe over the wallet's own UI, on screens where the user types their password. escapeHtml is now a pure string replace over & < > " ' — the old version round-tripped through textContent, which escapes neither quote, while already being used inside data-copy="...". All 19 files in src/popup/views/ were audited: beyond the reported symbol site, the explorer-supplied directionLabel in all three transaction lists, wallet.name, addr.ensName, the blockie data: URIs and two ad-hoc quote-only escapes were also unescaped. Explorer URLs now go through one helper that percent-encodes the path segment. Both manifests add default-src 'self', frame-src 'none', form-action 'none' and base-uri 'none'. Three loosenings are pinned in tests/manifest.test.js and justified in README.md: style-src 'unsafe-inline' (39 static style attributes; Firefox implements neither style-src-attr nor 'unsafe-hashes'), img-src data: (blockies), connect-src https: http: (user-configurable RPC). Note frame-src 'none' blocks a frame loading, not the element existing, so the zero-iframe assertion is a claim about the escaping alone; the test asserts the element count and the literal rendered text separately, taking the count before any click an overlay could intercept. Verified: make check 39 suites / 811 tests, test-e2e 55/55 including the WebAssembly-under-CSP assertion, test-e2e-firefox 8/8, zero CSP violations asserted rather than merely unobserved. Reverting only balanceLine's interpolation reproduces the attack as 2 iframes on the address screen.
This commit was merged in pull request #327.
This commit is contained in:
110
tests/htmlEscape.test.js
Normal file
110
tests/htmlEscape.test.js
Normal file
@@ -0,0 +1,110 @@
|
||||
// The escape every view depends on, and the length bound on a displayed
|
||||
// token symbol. Both were added for #307, where a token whose symbol()
|
||||
// returned an <iframe> tag rendered that iframe inside the popup.
|
||||
|
||||
const { escapeHtml } = require("../src/shared/html");
|
||||
const {
|
||||
displaySymbol,
|
||||
MAX_SYMBOL_LENGTH,
|
||||
UNKNOWN_SYMBOL,
|
||||
} = require("../src/shared/symbolDisplay");
|
||||
|
||||
// The payload from the issue's reproduction, verbatim.
|
||||
const HOSTILE_SYMBOL =
|
||||
'<iframe id="pwn" src="https://dapp.e2e.test/" ' +
|
||||
'style="position:fixed;left:0;top:0;width:360px;height:600px;z-index:99999"></iframe>';
|
||||
|
||||
describe("escapeHtml", () => {
|
||||
test("escapes all five characters, quotes included", () => {
|
||||
expect(escapeHtml("&<>\"'")).toBe("&<>"'");
|
||||
});
|
||||
|
||||
// The regression this function was rewritten for. The previous
|
||||
// implementation round-tripped through a detached div's textContent,
|
||||
// and an HTML text node serializes a quote as itself — so a value with
|
||||
// a quote in it broke straight out of data-copy="..." and href="...".
|
||||
test("escapes quotes, which the textContent round trip did not", () => {
|
||||
expect(escapeHtml('a"b')).toBe("a"b");
|
||||
expect(escapeHtml("a'b")).toBe("a'b");
|
||||
});
|
||||
|
||||
test("does not double-escape an ampersand it just introduced", () => {
|
||||
expect(escapeHtml("<")).toBe("&lt;");
|
||||
expect(escapeHtml("&")).toBe("&amp;");
|
||||
});
|
||||
|
||||
test("leaves a string with nothing to escape untouched", () => {
|
||||
expect(escapeHtml("USDC")).toBe("USDC");
|
||||
expect(escapeHtml("")).toBe("");
|
||||
});
|
||||
|
||||
test("renders the hostile symbol inert", () => {
|
||||
const out = escapeHtml(HOSTILE_SYMBOL);
|
||||
expect(out).not.toContain("<");
|
||||
expect(out).not.toContain(">");
|
||||
expect(out).not.toContain('"');
|
||||
expect(out).toContain("<iframe");
|
||||
});
|
||||
|
||||
// A quoted attribute is broken out of by a quote, a bare one by a
|
||||
// space; both are closed here. Asserted as a whole attribute rather
|
||||
// than character by character, because it is the attribute that has to
|
||||
// survive, not the escape table.
|
||||
test("a value carrying a quote stays inside its attribute", () => {
|
||||
const evil = '" onload="alert(1)';
|
||||
const attr = `data-copy="${escapeHtml(evil)}"`;
|
||||
expect(attr).toBe('data-copy="" onload="alert(1)"');
|
||||
expect(attr.split('"').length - 1).toBe(2);
|
||||
});
|
||||
|
||||
test("null and undefined render as nothing rather than as words", () => {
|
||||
expect(escapeHtml(null)).toBe("");
|
||||
expect(escapeHtml(undefined)).toBe("");
|
||||
});
|
||||
|
||||
test("coerces a non-string without losing the escape", () => {
|
||||
expect(escapeHtml(42)).toBe("42");
|
||||
expect(escapeHtml({ toString: () => "<b>" })).toBe("<b>");
|
||||
});
|
||||
});
|
||||
|
||||
describe("displaySymbol", () => {
|
||||
test("passes every symbol in the bundled list through unchanged", () => {
|
||||
const { TOKENS } = require("../src/shared/tokenList");
|
||||
for (const t of TOKENS) {
|
||||
expect([t.address, displaySymbol(t.symbol)]).toEqual([
|
||||
t.address,
|
||||
t.symbol,
|
||||
]);
|
||||
}
|
||||
});
|
||||
|
||||
test("caps an over-long symbol and marks it as truncated", () => {
|
||||
const long = "A".repeat(4096);
|
||||
const out = displaySymbol(long);
|
||||
expect(out.length).toBe(MAX_SYMBOL_LENGTH);
|
||||
expect(out.endsWith("…")).toBe(true);
|
||||
});
|
||||
|
||||
test("keeps a symbol of exactly the cap intact", () => {
|
||||
const exact = "A".repeat(MAX_SYMBOL_LENGTH);
|
||||
expect(displaySymbol(exact)).toBe(exact);
|
||||
});
|
||||
|
||||
test("substitutes a placeholder for an absent symbol", () => {
|
||||
expect(displaySymbol("")).toBe(UNKNOWN_SYMBOL);
|
||||
expect(displaySymbol(null)).toBe(UNKNOWN_SYMBOL);
|
||||
expect(displaySymbol(undefined)).toBe(UNKNOWN_SYMBOL);
|
||||
});
|
||||
|
||||
// The cap is a layout bound and nothing more: it must not be mistaken
|
||||
// for the thing that makes a symbol safe to render. A short hostile
|
||||
// symbol passes through it untouched, and is inert only because the
|
||||
// caller escapes it afterwards.
|
||||
test("does not sanitize — a short markup symbol survives it verbatim", () => {
|
||||
expect(displaySymbol("<img src=x>")).toBe("<img src=x>");
|
||||
expect(escapeHtml(displaySymbol("<img src=x>"))).toBe(
|
||||
"<img src=x>",
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user