1439163 - Request made for viewing page source bypasses webrequest listeners

Status: RESOLVED DUPLICATE of bug 1657916

(WebExtensions :: Request Handling, defect, P2)

Description

[user234683]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20100101

Steps to reproduce:

1. Have cookies stored for a website, and an extension that blocks the website from accessing them by modifying the webrequest.
2. Accessing the website normally blocks them as expected
3. View the page source, which creates a request to the website
Please see https://github.com/gorhill/uMatrix/issues/957 for the context of this issue


Actual results:

Cookies are not blocked because the extension does not get access to the webrequest, and this could be reflected in the html received, which might be different because of the cookie that was sent (for example, a login cookie results in the source received being that of a logged-in page).


Expected results:

The request should have triggered the webrequest listener, enabling the extension to intercept it and block the cookie.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

According to this [1] line the WebRequest only intercepts http(s) requests at the moment. view-source requests don't fall into that category.

Over to the webextensions team to decide how this should work.

[1] https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/toolkit/modules/addons/WebRequest.jsm#970

Comment 2

[Shane Caraveo (:mixedpuppy)]

ChannelWrapper.match is preventing this use case.  A simple fix would be to have FinalURLInfo strip viewsource: or to have Match do that.

Comment 3

[Kris Maglione [:kmag]]

We don't support view-source: requests in WebRequest, but the view-source: channel makes an underlying HTTP request, which we should support.

The problem is that we set the original URI of the HTTP channel to the view-source: URI that triggered the load, which causes us to ignore the request as lacking permission.

I think we should probably stop setting the original URI of the underlying channel, and make sure its LoadInfo and triggering principal match the details from the original channel, but I'm not sure about all of the implications of that.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> We don't support view-source: requests in WebRequest, but the view-source:
> channel makes an underlying HTTP request, which we should support.
> 
> The problem is that we set the original URI of the HTTP channel to the
> view-source: URI that triggered the load, which causes us to ignore the
> request as lacking permission.

Where exactly is this check and when exactly does it happen and how?

> 
> I think we should probably stop setting the original URI of the underlying
> channel, and make sure its LoadInfo and triggering principal match the
> details from the original channel, but I'm not sure about all of the
> implications of that.

Possible, but:

I'm not sure why it's being set (why the view-source inner channel origURI has to be view-source:).  Probably could be changed since we have bug 1319111 now.  I'm not sure why view-source and its inner channel share the same load info object, the one assigned during the view-source channel creation [1], but it may have its security reasons.

If we split them (probably give the inner channel a CloneForNewRequest copy), letting view-source overlaying channel have it's own LI, set result principal URI on its LI to view-source: since the beginning (and reclone/update LI again in nsViewSourceChannel::OnStartRequest, bug 1403998) we could leave the inner http channel live its normal life and be visible as an http:// load and not view-source:http:// to let it pass some of the security checks (is it actually a good thing?)  

But I'm not sure what all can break and how easily we can figure out what has been broken.  Note that when we set something on the view-source load info now, it promotes automatically to the inner channel's load info (as they are one shared object).  The proposed change would break it and we could very well introduce security bugs that way!

[1] https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/netwerk/protocol/viewsource/nsViewSourceHandler.cpp#120

Comment 5

[Kris Maglione [:kmag]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #3)
> > We don't support view-source: requests in WebRequest, but the view-source:
> > channel makes an underlying HTTP request, which we should support.
> > 
> > The problem is that we set the original URI of the HTTP channel to the
> > view-source: URI that triggered the load, which causes us to ignore the
> > request as lacking permission.
> 
> Where exactly is this check and when exactly does it happen and how?

It happens here:

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#505-511

Either in the mUrls->Matches check or the CanAccessURI check, either of which will fail for a view-source: URI.

FinalURLInfo returns a wrapper around the result of NS_GetFinalChannelURI,

> If we split them (probably give the inner channel a CloneForNewRequest
> copy), letting view-source overlaying channel have it's own LI, set result
> principal URI on its LI to view-source: since the beginning (and
> reclone/update LI again in nsViewSourceChannel::OnStartRequest, bug 1403998)
> we could leave the inner http channel live its normal life and be visible as
> an http:// load and not view-source:http:// to let it pass some of the
> security checks (is it actually a good thing?)  

I suspect it is a good thing. The security checks on the outer channel would remain the same, so a separate set of security checks on the inner channel should only ever add to the security of the request. The only real risk, I think, would be the possibility of breaking view-source: channels in some weird corner cases, which doesn't worry me overmuch.

On the other hand, with the current situation, we wind up treating view-source: requests differently than the underlying http: requests anywhere we make modifications that don't expect this kind of wrapping. Extensions that modify http: requests are one example. I can easily imagine things like the container code and tracker protection having similar issues.

Those probably aren't huge issues, given that the only semi-untrusted code that's generally allowed to load view-source: URLs is from extensions. But it's not great for developer experience, and the difference between view-source: and regular http: loads is probably exploitable by an extension if it tried hard enough.

Comment 6

[Honza Bambas (:mayhemer)]

So, the way we work with LI on v-s channels comes from bug 965413 and bug 1087442.  Since ever we just pass Set/GetLoadInfo of a viewsource channel to its inner channel.

I'll quick-fix a patch to have separate LI on v-s and inner and let Boris feedback it (better than to explain it in words)

Comment 7

[Honza Bambas (:mayhemer)]

The way we handle orig URI on v-s and inner channels comes from bug 171396.

One problem I found by looking for "view-source" around the code is here:
https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/feeds/nsFeedSniffer.cpp#211-213,223-238 - but there should be some way to work that around, IMO.

Another potential problem is that when the http channel gets redirected we don't get updated load info on the view-source channel automatically.  But that may not be a big issue as well.

I have a patch already.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: wip1

- view-source and its inner channel has separate LoadInfo objects
- the inner channel's LoadInfo is a CloneForNewRequest copy
- the inner channel is no longer set OriginalURI to the view-source URI
- on redirect we update result principal URI of the view-source channel only (bug 1403998)

there is one code we will break with this, comment 7.  I believe it can be worked around, and it's also not super critical.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: wip1

qref

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: wip2

Gijs, do you think this is a good idea in light of this bug?  Comment 0 and comment 4 maybe interesting.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8962455 [details] [diff] [review]
wip2

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

I *think* this makes sense, but it should be reviewed by someone who understands this better than me...

Comment 12

[Honza Bambas (:mayhemer)]

Kris, ping on feedback please.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1

So, this could be the first patch worth a regular review.  Comment 7 describe only one problem I found by code inspection when the original URI of the inner channel is not view-source.  It's solved by removing the LOAD_CALL_CONTENT_SNIFFERS load flag from view-source's inner channel.  Not sure it's the best thing to do, since there is not just nsFeedSniffer, there is a number of other sniffers registered.  Not sure what impact not calling them may have - probably missing syntax highlighting?  If that is a big issue, I can find a different, more explicit solution.

Comment 14

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46428e30e7c6008757a45b3140ac99165bab84a

Some test related to view-source are failing.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f46428e30e7c6008757a45b3140ac99165bab84a
> 
> Some test related to view-source are failing.

This may be fatal to this patch/bug, but let's first dissect a bit.  

fetch("view-source:/") is expected to fail with "Security Error: Content at %S may not load or link to %S."

W/o the patch we fail the load here:
 	xul.dll!nsScriptSecurityManager::CheckLoadURIFlags(0x00000172dbf11500, 0x00000172db5ca380, 0x00000172dbf11500, 0x00000172db539400, 0) Line 832	C++
 	xul.dll!nsScriptSecurityManager::CheckLoadURIWithPrincipal(0x00000172dbfb9790, 0x00000172db5ca380, 0) Line 782	C++
>	xul.dll!nsCORSListenerProxy::UpdateChannel(0x00000172db5d8078, Allow, Default) Line 942	C++
 	xul.dll!nsCORSListenerProxy::Init(0x00000172db5d8078, Allow) Line 436	C++
 	xul.dll!DoCORSChecks(0x00000172db5d8078, 0x00000172db5cf940, {...}) Line 299	C++
 	xul.dll!nsContentSecurityManager::doContentSecurityCheck(0x00000172db5d8078, {...}) Line 593	C++
 	xul.dll!mozilla::net::HttpChannelChild::AsyncOpen2(0x00000172da8524c8) Line 2569	C++
 	xul.dll!nsViewSourceChannel::AsyncOpen(0x00000172db544ab0, 0x0000000000000000) Line 373	C++
 	xul.dll!nsViewSourceChannel::AsyncOpen2(0x00000172db544ab0) Line 403	C++
 	xul.dll!mozilla::dom::FetchDriver::HttpFetch({...}) Line 719	C++
 	xul.dll!mozilla::dom::FetchDriver::Fetch(0x0000000000000000, 0x00000172dbf3f200) Line 402	C++

Note that the _channel_ we perform the check on is the inner channel (http) of the view source channel.

Reading the comment in nsViewSourceChannel::AsyncOpen2, we somewhat count on it, that the inner channel will subject itself to sec checking.

If we add nsContentSecurityManager::doContentSecurityCheck call to nsViewSourceChannel::AsyncOpen2 _as well_, it might work.  But here I'm again getting to the area I may know what I'm actually doing :)

Comment 16

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f042d59f93df7f5b1be7e3aba1a57e3ba645db24

Comment 17

[Honza Bambas (:mayhemer)]

David, what priority is this bug actually?  It turns out to be more work then I expected and I have higher priority stuff on my list.

Until that I treat this as low-prio for me and if needed to be worked on, please find someone else to finish this bug.  Maybe the patch may end up in a different code eventually.

Comment 18

[Honza Bambas (:mayhemer)]

Dis-assigning from myself.

Comment 19

[David Durst [:ddurst]]

This fell off the review train. Should we pick it back up as-is?

Comment 22

[Rob Wu [:robwu]]

This got fixed in Firefox 84, in https://hg.mozilla.org/mozilla-central/rev/8fd81380d432c26a9df2ea56f9330868363dec68