1408194 - (CVE-2018-5132) WebExtension Find API can search privileged pages

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Andy McKay]

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

[u462496]

(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?

Comment 2

[Andy McKay]

(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.

Comment 3

[Andy McKay]

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.

Comment 4

[Shane Caraveo (:mixedpuppy)]

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.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent using find api on about urls

Comment 6

[Andrew Swan [: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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent using find api on about urls

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: test for patch

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent using find api on about urls

Comment 11

[Shane Caraveo (:mixedpuppy)]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be82f6125c7502fe00a27d771d3033ceaa9c4d56
Bug 1408194 prevent using find api on about urls

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent using find api on about urls

In that case, just combining these into one test.

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/be82f6125c75

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

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 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84c1cb814b18