Bug 1408194 (CVE-2018-5132)

WebExtension Find API can search privileged pages

RESOLVED FIXED in Firefox 59

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: andy+bugzilla, Assigned: mixedpuppy)

Tracking

({sec-moderate})

unspecified
mozilla60
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 fixed, firefox60 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main59+])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
The Find API allows you to search across privileged pages. This means you could see whats on about:debugging by searching for that string, if the user has that page open.

It doesn't seem to work on about:config, that seems to be more because of how about:config works than anything else.

You can't search for regex, all you can really do is search for the presence of a string on a tab. But at some point I'm sure someone will file a bug searching for regex and someone might redesign about:config and suddenly you can read any pref.

For future proofing I'd like to recommend that we explicitly prevent this API from searching any about: page to prevent something accidentally leaking in the future.

Comment 1

2 years ago
(In reply to Andy McKay [:andym] from comment #0)
> The Find API allows you to search across privileged pages. This means you
> could see whats on about:debugging by searching for that string, if the user
> has that page open.
> 
> It doesn't seem to work on about:config, that seems to be more because of
> how about:config works than anything else.
> 
> You can't search for regex, all you can really do is search for the presence
> of a string on a tab. But at some point I'm sure someone will file a bug
> searching for regex and someone might redesign about:config and suddenly you
> can read any pref.
> 
> For future proofing I'd like to recommend that we explicitly prevent this
> API from searching any about: page to prevent something accidentally leaking
> in the future.

Just as an aside here, wouldn't the same security issue exist for any tab we can inject a content script in, and read all the text on that page, via treeWalker or document.getSelection() => selection.selectAllChildren(), etc?
(Reporter)

Comment 2

2 years ago
(In reply to Kevin Jones from comment #1)
> Just as an aside here, wouldn't the same security issue exist for any tab we
> can inject a content script in, and read all the text on that page, via
> treeWalker or document.getSelection() => selection.selectAllChildren(), etc?

Yes you can't inject content scripts into (currently) any privileged pages though, we explicitly block that. So I can't inject a context script on about:support, but if the user ever opens about:support, I can search it try and find specific strings from it.fingerprinting.

This may absolutely nothing to worry about but I just wanted us to explicitly think about it.
(Reporter)

Updated

2 years ago
(Reporter)

Comment 3

2 years ago
I think we'd like to limit this API down to not searching on any about: page. It's hopefully a pretty easy patch that should help prevent any long term surprises for about: pages changes.
Priority: -- → P2
(Reporter)

Updated

a year ago
Priority: P2 → P3
(Assignee)

Updated

a year ago
Assignee: nobody → mixedpuppy
(Assignee)

Comment 4

a year ago
I've gone back and forth on this, searching on about: pages is broken anyway (e.g. bug 1432948), so I think preventing it is fine.
(Assignee)

Comment 5

a year ago
Attachment #8945252 - Flags: review?(aswan)
Comment on attachment 8945252 [details] [diff] [review]
prevent using find api on about urls

Review of attachment 8945252 [details] [diff] [review]:
-----------------------------------------------------------------

A few things:
1. Why not put the check inside runFindOperation() instead of duplicating the code to identify the tab being searched
2. Is just checking if the scheme is "about" adequate here?  I guess we shouldn't generally have chrome: or resource: urls open in tabs but if they are open, what should happen?  Somebody with a better grasp of the big picture than me should evaluate this.  (perhaps kmag?  or Gijs?)
3. Tests for sec issues should be in a separate patch
Attachment #8945252 - Flags: review?(aswan) → review-
(Assignee)

Comment 7

a year ago
Attachment #8945252 - Attachment is obsolete: true
Attachment #8945593 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 8

a year ago
Posted patch test for patch (obsolete) — Splinter Review
Attachment #8945594 - Flags: review?(gijskruitbosch+bugs)

Comment 9

a year ago
Comment on attachment 8945593 [details] [diff] [review]
prevent using find api on about urls

Review of attachment 8945593 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the system principal check in addition ( or / disjunction / || ) to the url-based check, and perhaps an exception for about:blank.

::: browser/components/extensions/ext-find.js
@@ +20,5 @@
>    let mm = browser.messageManager;
>    tabId = tabId || tabTracker.getId(tab);
>  
> +  // We disallow find in about: urls.
> +  if (["about", "chrome", "resource"].includes(tab.linkedBrowser.currentURI.scheme)) {

This will block things on about:blank as well. Is that OK?


Separately, you will want to also check:

tab.linkedBrowser.contentPrincipal.isSystemPrincipal
Attachment #8945593 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

a year ago
Attachment #8945594 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 10

a year ago
Attachment #8945593 - Attachment is obsolete: true
Attachment #8946806 - Flags: review+
(Assignee)

Comment 11

a year ago
Comment on attachment 8946806 [details] [diff] [review]
prevent using find api on about urls

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  The find api can probably scan information on about pages.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  yes

Which older supported branches are affected by this flaw? fx 57 and later

If not all supported branches, which bug introduced the flaw? 1332144

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, but patch should apply

How likely is this patch to cause regressions; how much testing does it need?  If any extensions using the api expect to work on these pages, they now wont.  Otherwise shoudn't have any negative side affects.

Test patch should also land at some point.
Attachment #8946806 - Flags: sec-approval?
Comment on attachment 8946806 [details] [diff] [review]
prevent using find api on about urls

Sec-moderate or lower rated issues don't need sec-approval to be checked in so this can just go into trunk.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Attachment #8946806 - Flags: sec-approval?
(Assignee)

Comment 14

a year ago
In that case, just combining these into one test.
Attachment #8945594 - Attachment is obsolete: true
Attachment #8946806 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
Comment on attachment 8946949 [details] [diff] [review]
prevent using find api on about urls

Approval Request Comment
[Feature/Bug causing the regression]: find api
[User impact if declined]: find api can search some priviledged pages
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: minimal
[Why is the change risky/not risky?]: small patch constrained to the find api.
[String changes made/needed]: none
Attachment #8946949 - Flags: review+
Attachment #8946949 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/be82f6125c75
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
While this doesn't need sec-approval, I think we still should avoid landing tests for security-sensitive issues until after we unhide the bug.
Comment on attachment 8946949 [details] [diff] [review]
prevent using find api on about urls

Looks like a fairly low risk patch, let's uplift for beta 6.
Attachment #8946949 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: toolkit-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Alias: CVE-2018-5132
Summary: Find API can search privileged pages → WebExtension Find API can search privileged pages

Updated

10 months ago
Product: Toolkit → WebExtensions
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.