fix(beartrap): wrap navigator getters through the Xray waiver + report the outcome - #133
Merged
Conversation
Michael: 'zero trust dude we don't trust mozilla, we review the upstream and
harden where possible too.' He is right and my earlier framing was still too
trusting: a valid signature proves only that MOZILLA SIGNED IT, not that the
content is benign. A compelled or compromised upstream ships signed records.
review.mjs reads what we are about to serve and FAILS CLOSED on: a collection
with no signature, a URL in signed content the collection has no business
carrying, active-content markers (javascript:/<script/eval(), a new or vanished
collection, or a >25% swing in record count — the shape a poisoned security list
would take. A SHA256 baseline makes silent drift in already-vouched-for data
visible on the next run.
Made it collection-aware after the first pass produced noise — blunt rules get
ignored, which is worse than no rule. ct-logs IS a list of log endpoints,
intermediates embeds CPS URIs in certificate data, plugins/addons rows carry a
user-facing 'why was this blocked' link. Those are inventoried; everywhere else a
URL is a finding. (Same false-positive trap as calling plugins=5 a leak earlier —
verify what upstream legitimately contains before alarming.)
REAL findings from the first review of Mozilla PROD data:
- Mozilla ships PLACEHOLDER urls in production CT data:
https://ct.example.com/bogus/ and .../bogus/ipng/
- plugin-blocklist rows carry CLEARTEXT http:// advisory links (oracle,
adobe, microsoft, divx, mozilla blog)
Neither is fetched in normal operation, but we now SEE them instead of assuming,
and the baseline diff surfaces the next change automatically.
…t the outcome
Why the clamp never worked, now understood. The actor runs PRIVILEGED, so
win.Navigator.prototype reached from it is an XRAY WRAPPER, and
getOwnPropertyDescriptor through that wrapper does not return the content
getter. The old code therefore hit and silently did
nothing — no error, no log, no effect.
Everything else was already correct, which is why this was hard to see: verified
in the shipped v150.0.2 that BearTrapChild IS packaged, the clamp code IS in it,
the pref IS on, and the actor IS registered in DesktopActorRegistry (matches
http+https, allFrames: true, enablePreference set).
(For the record: I first claimed the actor was missing from the registry and put
that in the v150.0.2 release notes. It was WRONG — I grepped browser/omni.ja when
the registry ships in the TOOLKIT omni at moz-src/browser/components/. Zero hits
from a path that does not exist is not evidence of absence. Notes corrected.)
Fix: operate on win.wrappedJSObject (Xray-waived) and install the replacement
getter with Cu.exportFunction so it is created in the CONTENT compartment —
defining a privileged function on a content object would leak chrome into the
page.
And the part that matters as much as the fix: FAILURES ARE NO LONGER SWALLOWED.
Each wrap records why it failed and the result is exposed as
navigator.__bearTrapWrap ('ok' | 'partial:<reasons>' | undefined when the actor
never ran). The audit page now reads it, so the next build ANSWERS the question
instead of us inferring it from a missing side effect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why the clamp never worked
The actor runs privileged, so
win.Navigator.prototypereached from it is an Xray wrapper — andgetOwnPropertyDescriptorthrough it doesn't return the content getter. The old code hitif (!d.get) returnand silently did nothing. No error, no log, no effect.Everything else was already correct, which is exactly why this was hard to see. Verified in shipped v150.0.2: BearTrapChild is packaged ✅, the clamp code is in it ✅, the pref is on ✅, and the actor is registered in
DesktopActorRegistry(matches http+https,allFrames: true,enablePreferenceset) ✅.The fix
Operate on
win.wrappedJSObject(Xray-waived) and install the replacement getter withCu.exportFunctionso it's created in the content compartment — defining a privileged function on a content object would leak chrome into the page.The part that matters as much as the fix
Failures are no longer swallowed. Each wrap records why it failed, exposed as
navigator.__bearTrapWrap:"ok"— wrapping applied"partial:<reasons>"— wrapped some, with the reasonundefined— the actor never ran at allThe audit page reads it, so the next build answers the question instead of us inferring it from a missing side effect.