1429379 - (CVE-2018-5134) Web extensions can open any url using view-source:

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Abdulrahman Alqabandi]

Attachment: POC.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

Install attached extension


Actual results:

view-source urls for a file URL and about:cache is opened


Expected results:

blocked the same way as when opening directly

Comment 1

[Abdulrahman Alqabandi]

FWIW. Similar to bug 1427448 (check comment 16 and 20 for details) we can read contents of any file/about/web url opened using 'find' api (requires adding 'find' permission) and partially detect file existence using 'browser.tabs.detectLanguage' without 'tabs' permission as its enabled by default. 

Since this bug is very similar to bug 1427448 I will CC the same folks (those available.) Hope it's ok.

Comment 2

[Abdulrahman Alqabandi]

Would like to note that the 'find' API seems to fail fully reading things like 'view-source:about:cache' where as it reads 'view-source:file:///C://' fine. Not sure why but the data received from about:cache is still somewhat legible (I can make out my firefox profile directory)

Comment 3

[Andy McKay]

Thomas, would you have time to take a look at this?

Comment 4

[Thomas Wisniewski]

Not at the moment, I'm afraid. I'm also not sure what the correct fix would be; I'm guessing it would roughly be to detect attempts to access a view-source URL, and then parse out the URL it's trying to vuew, then test that URL against the extension's list of accessible URLs, throwing as necessary with something like a CSP error?

Comment 5

[Abdulrahman Alqabandi]

Probably introduced by bug 1261289

Comment 6

[Andrew Swan [:aswan]]

I don't know if there's a simpler fix but something like changing the view-source handler over to an nsIProtocolHandlerWithDynamicFlags and having it forward flags from the inner URL should do it.

Comment 7

[Thomas Wisniewski]

Attachment: 1429379.diff

Thanks, aswan. Actually that seems to work; this patch prevents the proof-of-concept addon from opening its tabs.

If you think it's probably solid enough to work with, I can give it a try-run and add a test-case if that works out (I'm not sure I'd get around to that until the weekend, though).

Comment 8

[Andrew Swan [:aswan]]

Nice.  I'm about to leave for a week of PTO, so I can't offer much help myself, I would suggest running this by (and then getting review from) somebody like :bz

Comment 9

[Thomas Wisniewski]

Alright. I started a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=623f34673bc8f19d7a9ba064eb4a3853f0564937

Comment 10

[Frederik Braun [:freddy]]

(In reply to Andrew Swan [:aswan] from comment #6)
> I don't know if there's a simpler fix but something like changing the
> view-source handler over to an nsIProtocolHandlerWithDynamicFlags and having
> it forward flags from the inner URL should do it.

We /also/ have some blacklists in place in the implementation of our WebExtension APIs. I wonder what the right place for restriction is, really.

Comment 11

[:Gijs (he/him)]

(In reply to Thomas Wisniewski from comment #9)
> Alright. I started a try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=623f34673bc8f19d7a9ba064eb4a3853f0564937

As the test failures bear out, this is not a good idea because it makes those URIs accessible to the web as well, which was the "whole point" of making them dangerous to load in the first place...

Note that generally, pushing to try for sec bugs is frowned upon unless the sec bug is already rated sufficiently low. This bug hasn't been rated at all. Please read https://wiki.mozilla.org/Security/Bug_Approval_Process .


view-source is annoying because in an ideal world, the following things would be true:

- things on the web can't link to any view-source URI
- things on the web can't access any loaded view-source documents, not even ones for their own domain (ie 'foo.com' and 'view-source:foo.com' should have separate origins as far as the SOP is concerned)
- webextensions should be able to load view-source: for URIs they would have access to without 'view-source:'.
- webextensions should be able to access such pages once loaded inasmuch as they'd be able to access the same pages when not viewing their source.

I'm not sure what the easiest solution is here, and I'm not familiar enough with our webextension security model to understand what the severity of the current issue is.

Comment 12

[Thomas Wisniewski]

Thanks, gijs. I'll be more careful with this sort of thing in the future. I'll also have to let others more familiar with this stuff come to a decision.

Comment 13

[Andrew Swan [:aswan]]

(In reply to :Gijs from comment #11)
> As the test failures bear out, this is not a good idea because it makes
> those URIs accessible to the web as well, which was the "whole point" of
> making them dangerous to load in the first place...

The half-formed thought that I did a poor job of explaining was not literally passing the flag directly through, but letting the view-source: URL be LOADABLE_BY_EXTENSIONS if the inner URL is LOADABLE_BY_ANYONE.  There are undoubtedly corner cases that I haven't considered though, I agree this could use some help from somebody with a better grasp of the big picture.

Comment 14

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from comment #13)
> (In reply to :Gijs from comment #11)
> > As the test failures bear out, this is not a good idea because it makes
> > those URIs accessible to the web as well, which was the "whole point" of
> > making them dangerous to load in the first place...
> 
> The half-formed thought that I did a poor job of explaining was not
> literally passing the flag directly through, but letting the view-source:
> URL be LOADABLE_BY_EXTENSIONS if the inner URL is LOADABLE_BY_ANYONE.  There
> are undoubtedly corner cases that I haven't considered though, I agree this
> could use some help from somebody with a better grasp of the big picture.

This seems reasonable to me.

Comment 15

[:Gijs (he/him)]

Attachment: Patch v0.1

This blocks the attached poc add-on, and seems to be what comment 13 suggests.

It'll also block "legitimate" add-on-provided view-source operations on file: or other (semi-)privileged pages. I think that's OK.

Comment 16

[Abdulrahman Alqabandi]

Would like to point out that not only can WebExtensions open any view-source: urls but looks like no security is applied when including them as a resource.

For example, within a webextension html page we have something like:

<link rel="stylesheet" href="view-source:file:///C://test.xml"/>
<link rel="stylesheet" href="file:///C://test.xml"/>

The first tag will not be blocked (though a mime type mismatch will occur) yet the second one will be blocked by security.

So I think there is a good chance this bug can be used to read or at least detect local files. Will post a PoC if i come up with something solid.

Comment 17

[Abdulrahman Alqabandi]

Attachment: detectLocalFiles.xpi

PoC of detecting local files.

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

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

That is what I had in mind, yes.  But my r+ shouldn't carry much weight here, bz has vastly more insight into both c++ language issues and any larger implications from this change.
Regarding comment 15 though, I'm not sure there is such a thing as a legitimate load of a view-source url for a file coming from an extension.  There is an open bug (actually probably several) about filesystem access from extensions, so such a thing may become legitimate eventually but I don't think there's any need to worry about that right now.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

>+#define DEFAULT_FLAGS URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD | URI_NON_PERSISTABLE

Parens around the value, please:

#define DEFAULT_FLAGS (URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD | URI_NON_PERSISTABLE)

>+    nsCOMPtr<nsIIOService> io = do_GetIOService();

Might as well cut out the middleman and call services::GetIOService.

Alternately, could do_GetNetUtil instead of the get-and-QI thing.  This is all moderately silly and we should clean up the xpcom goop here a lot.  :(  But not in this bug.

r=me.  My apologies for the lag...

Comment 20

[:Gijs (he/him)]

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

(no sec-rating and not sure about severity so asking for approval. I guess because add-on install is required (x-ref bug 1431371), this probably isn't more than sec-moderate, but I want to hear other opinions on this before landing this.)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly easily. It's easy to see from the patch that it's to do with add-ons + view-source URIs.


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

No, but the code itself does.


Which older supported branches are affected by this flaw?

This regressed in 57 so release 58, beta 59, nightly.


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

bug 1261289


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

Similar / the same, easy to make, not too risky assuming this patch doesn't run into trouble landing - haven't pushed to try because no sec-severity...


How likely is this patch to cause regressions; how much testing does it need?

It's essentially specific to how we deal with view-source URIs. We have reasonable automated test coverage for the basics of those in the tree, and the patch is supposed to only affect whether add-ons can access them, which is exactly what we want to restrict, so I think it needs minimal testing. The worst that should be able to happen if this lands and doesn't break automated testing is that webextensions can't access view-source URIs anymore, which would be unfortunate but I'm (a) pretty confident in the patch and (b) in some respects that'd be better than the status quo, and (c) it'd hopefully be picked up by the same people who filed bug 1261289 in the first place.


My only other qualm is how many other view-source related issues we have. I think bug 1429881 should be mitigated by bug 1431371, even for view-source:addons.mozilla.org . Andrew, is that right?

The ideal fix here is bug 1430257 but at the moment it's blocked on unnesting about:, and removing remote jar support which in turn is blocked on un-mainthread-IO'ing jar:, which isn't progressing at the moment (bug 1373708). :-(

Comment 21

[Andrew Swan [:aswan]]

(In reply to :Gijs from comment #20)
> My only other qualm is how many other view-source related issues we have. I
> think bug 1429881 should be mitigated by bug 1431371, even for
> view-source:addons.mozilla.org . Andrew, is that right?

From inspection, bug 1431371 should prevent the extension from bug 1429881 from working, I'll confirm that actually does that when I get a chance to get back to bug 1431371 (which is on my todo list for today)

Comment 22

[Andrew Swan [:aswan]]

Confirmed that with the patch from bug 1431371 applied, the extension from bug 1429881 now fails with the error "Missing host permission for the tab".

Comment 23

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

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

We've made this a sec-moderate bug. As such, it doesn't need sec-approval to land so I'm clearing that.

Comment 24

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

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

Comment 25

[:Gijs (he/him)]

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1261289
[User impact if declined]: security bug


[Is this code covered by automated tests?]: generally on security infrastructure for view-source: yes; generally for view-source: working: yes; for webextensions and web content URLs: yes; for webextensions and view-source URIs (ie this specific problem): no.
[Has the fix been verified in Nightly?]: it's landed and nobody has complained yet. But not formally, no. @qab, can you confirm the add-on is broken by these changes on today's nightly?

[Needs manual test from QE? If yes, steps to reproduce]: I don't think so.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: we're only limiting access to view-source URIs for add-ons. There are tests that would have flagged if we broke something more serious (like view-source: generally, or view-source:http stuff for webextensions, or view-source being accessible to the general web). And there's still considerable beta runway left.
[String changes made/needed]: none

Comment 26

[Abdulrahman Alqabandi]

Using latest Nightly the original PoC does not work anymore.

Unchecked lastError value: Error: Illegal URL: view-source:file:///C://test.xml  ExtensionCommon.jsm:424
Unchecked lastError value: Error: Illegal URL: view-source:about:cache  ExtensionCommon.jsm:424


Also the PoC in c17 (loading view-source:file: in scripts to detect file existence) stopped working as well. Will test more, will comment if anything comes up.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8948689 [details] [diff] [review]
Patch v0.1

Approved for 59b12.

Comment 28

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

https://hg.mozilla.org/releases/mozilla-beta/rev/27d0c44a5757