Skip to content

Show what is actually being approved: sanitise untrusted text, render data outputs - #8

Open
cdonnachie wants to merge 2 commits into
mainfrom
fix/sanitize-untrusted-display-text
Open

Show what is actually being approved: sanitise untrusted text, render data outputs#8
cdonnachie wants to merge 2 commits into
mainfrom
fix/sanitize-untrusted-display-text

Conversation

@cdonnachie

Copy link
Copy Markdown

Two related changes to what the approval screen actually shows the user.

1. Sanitise untrusted request text before display

A connect request supplies app, origin and the challenge, and the approval
screen renders all three so the user can decide what they are agreeing to.
cleanString rejects C0 controls and DEL, which stops a request breaking the
layout — but not from reordering it. Bidirectional overrides (U+202A-202E,
U+2066-2069) reverse the visual order of what follows, so a crafted request can
read as one thing on screen while the bytes say another. No invalid UTF-8 and no
control character is needed, and today such a request is accepted and rendered
verbatim.

@lib/displayText adds sanitizeForDisplay, which replaces bidi controls, C1
and invisible spaces with U+FFFD, and hasUnsafeDisplayChars, which reports
whether anything was there. Replacing rather than stripping is deliberate: a
silently cleaned string fails the user the same way the raw one does, because
either way they cannot tell something was removed.

Applied at every site that renders request-supplied text — the sign, PSBT, mint
and swap panels, and the two action pages. The sign panel also warns when a
request carried hidden formatting, so the replacement characters are explained
rather than merely puzzling.

U+200C ZWNJ and U+200D ZWJ are deliberately left alone: they are joiners, not
directional controls, cannot reorder text, and are load-bearing in Devanagari
and emoji sequences.

Validation still happens in the protocol layer and sanitisation only at display,
so nothing changes about which requests are accepted, and the bytes signed
remain exactly the bytes the request supplied.

2. Show what an OP_RETURN output actually carries

The approval screen rendered every unrecognised locking script as
"(non-standard output)". For a data carrier that is the least useful thing it
could say: the output holds 0 satoshis, so nothing about the money is
interesting, and the payload — the part published permanently and impossible to
take back — was the one thing not shown.

@lib/dataOutput describes such an output without interpreting it: total size,
the raw payload, and the pushes it splits into when it is push-structured.
Deciding that some bytes are a timestamp, a token or a message is a claim about
someone else's protocol, and a wallet asserting it would be vouching for
something it cannot check.

PsbtOutputSummary carries the description and the PSBT panel renders
"Data output · N bytes" with the contents behind a toggle. Text is offered for a
push only when it decodes as UTF-8 and contains nothing that could hide or
reorder its own display, and is rendered through sanitizeForDisplay even then;
hex is always shown beside it, because hex cannot misrepresent itself.

Parsing fails open in the honest direction: a payload that is not a clean push
sequence, is truncated, or carries an implausible number of pushes falls back to
raw hex rather than a guess.

Also

Corrects the header comments in sign.ts and its test: signing is deterministic
(RFC 6979 deterministicK), not random-k as documented. signRandomK exists in
radiantjs but Message.sign does not use it.

Testing

  • 18 new unit tests (displayText.test.ts, dataOutput.test.ts), including a
    lastIndex statefulness case — a /g/ regex reused across calls will
    silently answer differently the second time — and one asserting an emoji
    family survives sanitisation.
  • Full suites pass: 971 lib, 551 app. tsc --noEmit clean on both packages.
  • pnpm lint reports 7 errors, all pre-existing; verified by stashing and
    re-running on a clean tree.
  • Exercised against a real request: a sign-request carrying a bidi override in
    app and in the challenge now shows the warning and the replacement
    characters instead of reversed text, and a HashMark v2 record renders as its
    six pushes with the magic readable and the digest, signer and signature in
    hex.

A connect request supplies `app`, `origin` and the challenge, and the
approval screen renders all three so the user can decide what they are
agreeing to. `cleanString` rejects C0 controls and DEL, which stops a
request breaking the layout — but not from reordering it. Bidirectional
overrides (U+202A-202E, U+2066-2069) reverse the visual order of what
follows, so a crafted request can read as one thing on screen while the
signature covers another. No invalid UTF-8 and no control character is
needed, and today such a request is accepted and rendered verbatim.

Adds `@lib/displayText`: `sanitizeForDisplay` replaces bidi controls,
C1, and invisible spaces with U+FFFD, and `hasUnsafeDisplayChars`
reports whether anything was there. Replacing rather than stripping is
deliberate — a silently cleaned string fails the user the same way the
raw one does, because either way they cannot tell something was removed.

Applied at every site that renders request-supplied text: the sign,
PSBT, mint and swap request panels, and the two action pages. The sign
panel also warns when the request carried hidden formatting, so the
replacement characters are explained rather than merely puzzling.

U+200C ZWNJ and U+200D ZWJ are deliberately left alone: they are
joiners, not directional controls, they cannot reorder text, and they
are load-bearing in Devanagari and emoji sequences.

Validation still happens in the protocol layer and sanitisation only
here, so nothing changes about which requests are accepted, and the
bytes signed remain exactly the bytes the request supplied.

Also corrects the header comments in `sign.ts` and its test: signing is
deterministic (RFC 6979 `deterministicK`), not random-k as documented.
The approval screen rendered every unrecognised locking script as
"(non-standard output)". For a data carrier that is the least useful
thing it could say: the output holds 0 satoshis, so nothing about the
money is interesting, and the payload — the part that is published
permanently and cannot be taken back — was the one thing not shown.

Adds `@lib/dataOutput`, which describes such an output without
interpreting it: total size, the raw payload, and the pushes it splits
into when it is push-structured. Deciding that some bytes are a
timestamp, a token or a message is a claim about someone else's
protocol, and a wallet asserting it would be vouching for something it
cannot check.

`PsbtOutputSummary` carries the description, and the PSBT panel renders
"Data output · N bytes" with the contents behind a toggle. Text is
offered for a push only when it decodes as UTF-8 and contains nothing
that could hide or reorder its own display, and is rendered through
`sanitizeForDisplay` even then; hex is always shown beside it, because
hex cannot misrepresent itself.

Parsing is fail-open in the honest direction: a payload that is not a
clean push sequence, is truncated, or carries an implausible number of
pushes falls back to raw hex rather than a guess.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant