Linking to view-source: is only useful for browser developers. IE already dropped support in XP SP2, citing security concerns. We should probably remove it too. The view-source: protocol has caused quite a few security bugs in Firefox, msotly due to security checks failing to take view-source's specialness into account: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=481342,762705,561051,645564,290982,290949,262689 And the protocol continues to be a danger to the security of web sites with private information or CSRF tokens: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=263290,624883,973837,1171853

We could probably just mark this as URI_DANGEROUS_TO_LOAD. As long as links inside view-source still work (they should) and typing it in the url bar works (it should) and the actual view-source UI works, it should be fine.

Bug 1172165 - make view-source dangerous to load, r?sworkman

This didn't seem to break view-source UI, or view-source links. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2387f57bb8d

Having worked on view source tabs lately, I tried some manual test cases too, and this seems fine to me.

Could we add a test for this? It seems like something we wouldn't want to regress.

(In reply to Valentin Gosu [:valentin] from comment #5) > Could we add a test for this? It seems like something we wouldn't want to > regress. +1 If you can add tests, please do. Valentin has a good handle of what's going on here, so I'm delegating future reviews to him. Please r?valentin for subsequent patch versions :)

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Interestingly, I don't see this code change having any testable impact. Opening view-source links in an iframe already fails, showing netError instead. Opening a view-source link using window.open from inside the page works both with and without this patch. I don't know if this means the patch is insufficient (do we set different load flags on the channels that override the protocol handler's, or something?) or if I'm simply missing something obvious here in terms of what the reporter in bug 1171853 means with "open a view-source URL using javascript". Note also that that report was regarding a same-origin URL, and in my testing, a window.open call even with non-same-origin URLs works. Valentin, any ideas on what I'm missing here?

Gah, thanks bzexport. Anyway, this was the test I tried to use (I mean, initially I used an iframe, then I switched to window.open - the former passed before the patch, the latter fails even with the patch).

> I don't know if this means the patch is insufficient (do we set different > load flags on the channels that override the protocol handler's, or > something?) or if I'm simply missing something obvious here in terms of what > the reporter in bug 1171853 means with "open a view-source URL using > javascript". Note also that that report was regarding a same-origin URL, and > in my testing, a window.open call even with non-same-origin URLs works. Yes, window.open is exactly what I meant, so if the test fails with that, I'm guessing the patch isn't sufficient. Same-origin is required to read things from the page, that's why I mentioned it in the original report. The window.open part will of course work with any origin.

From what I can tell, the intent here is to disallow the content of view-source from being accessed from a regular http page. bug 1171853 comment 0 seems to have a testable use case.

The intent here is to prevent the window.open thing. I bet it fails because the security manager unwinds nested uris in CheckLoadURIWithPrincipal and then works with the result...

(In reply to Not doing reviews right now from comment #12) > The intent here is to prevent the window.open thing. > > I bet it fails because the security manager unwinds nested uris in > CheckLoadURIWithPrincipal and then works with the result... Indeed. I tried fixing that (as conservatively as I could, because wth why am I patching caps/...), but that then makes this change break links in view-source pages. :-\

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Bug 1172165 - make view-source dangerous to load, f?valentin,bz

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Unless I'm missing something obvious, if we're making this URI_DANGEROUS_TO_LOAD, this means that unprivileged content (like other view-source pages...) can no longer link to it. Now that we have view-source in a tab, I guess we could technically maybe work around this by catching navigations using the <a> tags from a privileged framescript or something, but that seems hacky. Boris, did you have something else in mind or did I botch the patch in some way? :-)

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz > Now that we have view-source in a tab But are not planning to break the non-tab version, I keep being told, yes? In any case, it seems like we fundamentally have two options here: 1) Stop using a nested URI for view-source:, change things that currently rely on that for non-security stuff to use some other mechanism for getting the underlying URI to view the source of (for example). This is likely to also end up giving view-source a codebase principal for the view-source URI, so it won't be same-origin with the actual site (which is not a problem per se). 2) Change CheckLoadURIWithPrincipal to operate more on the original URIs, not the unwrapped ones, for the various URIChainHasFlags bits. This might make sense to do anyway, and may not even be mutually exclusive with option 1. This patch is kinda trying to do #2, but just not getting it quite right. ;)

(In reply to Not doing reviews right now from comment #16) > Comment on attachment 8617466 [details] > MozReview Request: Bug 1172165 - make view-source dangerous to load, > f?valentin,bz > > > Now that we have view-source in a tab > > But are not planning to break the non-tab version, I keep being told, yes? No idea, but fwiw, the frame script approach could probably be made to work even for view-source-in-a-view-source-window. > In any case, it seems like we fundamentally have two options here: > > 1) Stop using a nested URI for view-source:, change things that currently > rely on that for non-security stuff to use some other mechanism for getting > the underlying URI to view the source of (for example). This is likely to > also end up giving view-source a codebase principal for the view-source URI, > so it won't be same-origin with the actual site (which is not a problem per > se). > > 2) Change CheckLoadURIWithPrincipal to operate more on the original URIs, > not the unwrapped ones, for the various URIChainHasFlags bits. This might > make sense to do anyway, and may not even be mutually exclusive with option > 1. > > This patch is kinda trying to do #2, but just not getting it quite right. ;) mm, I'm worried about breaking too much. I guess we should make sure that one view-source URL can load another view-source URL (which is about the original URI rather than the unwrapped one) before bailing if the target URL is dangerous-to-load. I can add more specialcasing for that, but it feels too hacky. It makes sense that this check came (I mean, does come - without the changes in the patch) after the same-protocol check in order to avoid an issue like this. I'm wary of changing that entire comparison to be based on non-base URIs, though, because it would cause issues where e.g. jar:file can link to jar:http or vice-versa. Would making a helper here to check same-scheme throughout the chain be sensible? That would break links from http to view-source:http (desired) but maybe it would also break other nested URIs that we do want to keep working?

> I'm wary of changing that entire comparison to be based on non-base URIs, though, because > it would cause issues where e.g. jar:file can link to jar:http or vice-versa. Yes, that would be bad. Though _almost_ as bad is if view-source:http:// can link to view-source:file// Seems like we should define the actual policy we want here in detail before we worry about how to implement it.

(In reply to Not doing reviews right now from comment #18) > > I'm wary of changing that entire comparison to be based on non-base URIs, though, because > > it would cause issues where e.g. jar:file can link to jar:http or vice-versa. > > Yes, that would be bad. Though _almost_ as bad is if view-source:http:// > can link to view-source:file// > > Seems like we should define the actual policy we want here in detail before > we worry about how to implement it. OK. We currently unwrap nested URIs. So if I have jar:<x> or feed:<x> or view-source:<x> *** then <x> can link to them (and they can link to each other), but jar:<x> and jar:<y> can't link to each other. It seems we want to break this first property (<x> and nesting:<x>) at least for view-source. Do we mind breaking this property (<x> can link to jar:<x>/feed:<x> etc. and vice versa) for nested protocols besides view-source? (and/or how much of the web will that break?) If we do not mind, then I would suggest that the policy should be that rather than just comparing the schemes of the base URIs, we should compare the entire list of nested schemes. This will break: <a>:<b>: linking to <b>: and vice versa, for all <a> and <b>. If we *do* mind for some of these cases, then I think we need to decide which <a> we do want to do this for (starting with view-source, but potentially not including any others). Does that sound right or am I missing something again? (I'm somewhat assuming we'll need to treat about/moz-safe-about specially... but I could be wrong?) *** aside: are these the only ones? (it seems *some* about: URIs also implement nsINestedURI in order to point to moz-safe-about... which kind of messes with this idea) I had "fun" trying to find out what other users of nsINestedURI there were because there are a variety of ways of implementing it (actually implementing it in C++, nsSimpleNestedURI, IOService/NetUtil::newSimpleNestedURI, using it from a JS implementation of nsIURI (which nobody seemed to be doing)).

> Do we mind breaking this property (<x> can link to jar:<x>/feed:<x> etc. and vice versa) > for nested protocols besides view-source? We probably don't care about jar:. But we use the same setup to make some about: URIs linkable from everywhere, as you noticed. I assure you that making about:blank not linkable will Break The Web (TM). That's not a showstopper; we just need to keep about: working somehow. > and vice versa, for all <a> and <b>. That makes no sense. If <b> is "http", then <a>:<b>: should be able to link to it. _Everything_ can link to it. So what I think we can consider doing is this, to sort of get at the idea I think you're trying to get at: 1) Change the current "any scheme can link to itself" check to a "start at the base URIs, not the innermost ones, and allow linking if all the schemes in the chain match". 2) Change the flag-checking setup to operate on the base URI of the target, not the innermost one (note that flag-checks walk the whole chain). Audit the order we do the flag-checks in to make sure it's most-restrictive first. 3) Since in the new setup inner URIs can only restrict things more, not loosen then, reverse the way about: works to make about: linkable from anywhere but have a moz-unsafe-about: inner on the things that are not. So http: would still be able to link to jar:http:, since jar: doesn't restrict linking to it. But we could mark view-source: as not linkable, and then http: would not be able to link to view-source:http:. view-source:http: would still be able to link to itself under the modified "all the schemes in the chain match" condition. Does that give us the behavior we want?

(In reply to Not doing reviews right now from comment #20) > 3) Since in the new setup inner URIs can only restrict things more, not > loosen then, reverse the way about: works to make about: linkable from > anywhere but have a moz-unsafe-about: inner on the things that are not. This last part seems (from having poked at a bit of the code earlier today) like it will be a significant undertaking and/or potentially break things for add-ons / non-core Firefox stuff (moz-safe-about shows up in our codebase in a surprising number of places), maybe even in ways that have security implications. Or am I being (unhealthily) paranoid? It feels like we're (I'm) missing a simpler solution. Or maybe I'm just naive in thinking a simpler solution must exist? :-) > So http: would still be able to link to jar:http:, since jar: doesn't > restrict linking to it. But we could mark view-source: as not linkable, and > then http: would not be able to link to view-source:http:. > view-source:http: would still be able to link to itself under the modified > "all the schemes in the chain match" condition. > > Does that give us the behavior we want? I believe so, though I feel like the whole thing is complex enough that I might still be wrong. I'm also not sure I'm the best person to ask... when I wrote the strawman (and even when I wrote the second strawman) this seemed simpler than it turned out (which is not a complaint, I'm just pointing out that I am hardly a subject matter expert).

Ugh. People hardcoding stuff... We could always makes about: nested. So keep moz-safe-about: as-is but make moz-unsafe-about: for the unsafe case. It sucks, because there's performance overhead for moz-safe-about: that hits about:blank and I'd like to get rid of, but it would be simpler and more backportable if we decide to backport. Then we could figure out how to get rid of moz-safe-about: and whether we can. > Or maybe I'm just naive in thinking a simpler solution must exist? :-) When implementing a "no one ever figured out what behavior we want" security policy? You tell me. ;) OK, Bobby, Daniel, what do you think of the proposal in comment 20?

Sounds good to me, I think.

(In reply to Not doing reviews right now from comment #20) > > Do we mind breaking this property (<x> can link to jar:<x>/feed:<x> etc. and vice versa) > > We probably don't care about jar:. I care that http://foo can link to jar:http://foo. I think your proposal preserves this. > 1) Change the current "any scheme can link to itself" check to a "start at > the base URIs, not the innermost ones, and allow linking if all the schemes > in the chain match". By "base" you mean "outermost"? In CheckLoadURIWithPrincipal "base" is used in variable names holding the innermost URI. On that assumption this sounds like a good start. > 2) Change the flag-checking setup to operate on the base URI of the target, > not the innermost one (note that flag-checks walk the whole chain). Audit > the order we do the flag-checks in to make sure it's most-restrictive first. Most-restrictive flag or most-restrictive scheme? How do we define "most-restrictive"? In any case yes, all the NS_URIChainHasFlags() calls make no sense if we've already stripped off the chain and are operating on the innermost. The two literal scheme checks (javascript and chrome) do need to be done on the innermost scheme, though. > 3) Since in the new setup inner URIs can only restrict things more, not > loosen then, reverse the way about: works to make about: linkable from > anywhere but have a moz-unsafe-about: inner on the things that are not. That sounds like a PITA, but unavoidable if about: remains a nested type. Especially if there are hardcoded moz-safe-about checks and that type goes away. > So http: would still be able to link to jar:http:, since jar: doesn't > restrict linking to it. But we could mark view-source: as not linkable, and > then http: would not be able to link to view-source:http:. > view-source:http: would still be able to link to itself under the modified > "all the schemes in the chain match" condition. But view-source:http: would not be able to link to view-source:jar:http: -- I can live with that. In the unlikely event you're debugging something that convoluted you can copy the URL and open it in a new tab manually.

> By "base" you mean "outermost"? Yes, should have been clearer about that. Naming is hard. :( > How do we define "most-restrictive"? Hmm. URI_DANGEROUS_TO_LOAD is most restrictive, URI_LOADABLE_BY_ANYONE least restrictive. That leaves URI_IS_UI_RESOURCE, URI_IS_LOCAL_FILE, URI_LOADABLE_BY_SUBSUMERS. We can figure out what we want to do there... Conceptually, if any of those flags is set and would cause us to deny the load, we should deny the load. > But view-source:http: would not be able to link to view-source:jar:http: That's true. I can also live with that.

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Bug 1172165 - make view-source dangerous to load, r?bholley

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Bobby, how close is this to what bz was suggesting? The test passes and I can still navigate from one view-source thing to another, but that's about the extent of my local checking. Try push is here: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d75ff6b0ed86

For the moz-safe-about stuff, I think we should use the new nsIProtocolHandlerWithDynamicFlags infrastructure I added in bug 1186152, and get rid of moz-safe-about. Sorry for letting this sit so long. I'm going to throw it back in bz's pile, because he's back and I'm going on PTO soon, and probably won't be around long enough to see this patch through.

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz I can't flag bz yet, so moving this to an NI on Gijs, who can pass it to bz when he opens up his queue (presumably in a day or two).

(In reply to Bobby Holley (:bholley) from comment #28) > For the moz-safe-about stuff, I think we should use the new > nsIProtocolHandlerWithDynamicFlags infrastructure I added in bug 1186152, > and get rid of moz-safe-about. Hmm. This: https://hg.mozilla.org/mozilla-central/rev/b72d4867ae69#l3.12 seems to indicate that if add-ons call this on about: if we made it dynamic, that would throw... Is that right? > Sorry for letting this sit so long. I'm going to throw it back in bz's pile, > because he's back and I'm going on PTO soon, and probably won't be around > long enough to see this patch through. OK. I think I just realized that the scheme loop comparison thing I wrote is still broken, so I'll have a look at the patch again before requesting review.

(In reply to :Gijs Kruitbosch from comment #30) > (In reply to Bobby Holley (:bholley) from comment #28) > > For the moz-safe-about stuff, I think we should use the new > > nsIProtocolHandlerWithDynamicFlags infrastructure I added in bug 1186152, > > and get rid of moz-safe-about. > > Hmm. This: https://hg.mozilla.org/mozilla-central/rev/b72d4867ae69#l3.12 > seems to indicate that if add-ons call this on about: if we made it dynamic, > that would throw... Is that right? Yes, and I think I'm fine with that.

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Bug 1172165 - make view-source dangerous to load, r?bz

Try push: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=893b58a9688e

(In reply to :Gijs Kruitbosch from comment #33) > Try push: remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=893b58a9688e Well, that didn't go over well. Looks like this is related to about: URIs, but I haven't figured out exactly how to make this work yet. I'll have another look tomorrow.

(In reply to Bobby Holley (busy) from comment #28) > For the moz-safe-about stuff, I think we should use the new > nsIProtocolHandlerWithDynamicFlags infrastructure I added in bug 1186152, > and get rid of moz-safe-about. Right... so this seems to be what's causing all the orange. I added the protocol-with-dynamic-flags stuff and in local testing that seems to fix all of the randomly-picked bits of orange, so here's a new push: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a683051408 Unfortunately, getting rid of moz-safe-about is going to be tricky because we do hardcoded checks for moz-safe-about and in some cases I *think* we will have been using the inner URI for e.g. the origin for which we store databases (if our devtools code for that is to be believed: https://dxr.mozilla.org/mozilla-central/rev/b40ba117fa757861c9caa660ae989122718b494b/devtools/server/actors/storage.js#1341-1347 ) Considering it's the last week for an ESR cycle (and considering some recent issues...), I think it would be neat to get this landed and postpone getting rid of moz-safe-about (though I have a patch in hand that makes the requisite changes to netwerk/, updating the rest of the tree and add-ons and not missing anything is going to be "fun", I suspect...). Bobby, does that sound like a plan?

See bug 1228118 for getting rid of the moz-safe-about thing; it will need a bit of work.

Discussed this with Bobby and Boris in person. I have a patch for the reftest/crashtest stuff (basically, pref-based exception specifically for file: loading view-source:file) and the xpcshell and mochitest-1-5 failures from the try push. Which covers 99% of them, I believe - but we'll find out soon enough: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee991a7b917

(In reply to :Gijs Kruitbosch from comment #37) > Discussed this with Bobby and Boris in person. I have a patch for the > reftest/crashtest stuff (basically, pref-based exception specifically for > file: loading view-source:file) and the xpcshell and mochitest-1-5 failures > from the try push. Which covers 99% of them, I believe - but we'll find out > soon enough: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee991a7b917 Gah, so it turns out android uses http: for reftests, presumably so the files are served off something other than the emulator. So this is still turning all of Android ref/crashtests orange. I can expand the exception to include http but I'm not sure I like that idea very much. Then again, not sure I have much choice...

Looks like besides Android, there's nothing that is obviously related to my set of patches. Tried to fix Android, testing fewer platforms with this change: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e55390f95213

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/10633/diff/4-5/

Bug 1172165 - part 1: Fix about URI flags to work in our new nested-checking world, r?bz

Bug 1172165 - part 2: remove broken bit of test for view-source linking now that it is completely disallowed, r?bz

Bug 1172165 - part 3: Fix view-source test of html parser

Bug 1172165 - part 4: fix up reftest/crashtest, r?bz

(Bobby, sorry, but bz has already marked himself as not available for reviews - if you want to redirect to someone else, please do!)

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz After aging it to perfection in my queue cellar, I'm passing this one back to bz. ;-)

Comment on attachment 8697610 [details] MozReview Request: Bug 1172165 - part 1: Fix about URI flags to work in our new nested-checking world, r?bz https://reviewboard.mozilla.org/r/27649/#review29461 ::: netwerk/protocol/about/nsAboutProtocolHandler.h:16 (Diff revision 1) > + public nsIProtocolHandler When you merge, move the ',' to the start of the second line here, lik the other superclass we have now. ::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:41 (Diff revision 1) > -NS_IMPL_ISUPPORTS(nsAboutProtocolHandler, nsIProtocolHandler) > +NS_IMPL_ISUPPORTS(nsAboutProtocolHandler, nsIProtocolHandler, nsIProtocolHandlerWithDynamicFlags) And watch the 80th column here. r=me

Comment on attachment 8697611 [details] MozReview Request: Bug 1172165 - part 2: remove broken bit of test for view-source linking now that it is completely disallowed, r?bz https://reviewboard.mozilla.org/r/27651/#review29463 r=me

Comment on attachment 8697612 [details] MozReview Request: Bug 1172165 - part 3: Fix view-source test of html parser https://reviewboard.mozilla.org/r/27653/#review29465 r=me

Comment on attachment 8697613 [details] MozReview Request: Bug 1172165 - part 4: fix up reftest/crashtest, r?bz https://reviewboard.mozilla.org/r/27655/#review29467 r=me, I guess. I can't think of a better approach. :(

Comment on attachment 8617466 [details] MozReview Request: Bug 1172165 - part 0: make view-source dangerous to load, check nested URI schemes, r?bz https://reviewboard.mozilla.org/r/10633/#review29469 r=me, but I think you should squash these changesets together. Or at least this part and the next one, since the browser will act all sorts of weird with this but without the about: fixes. I'm probably OK with leaving the test fixes in separate changesets. Or even better, reordering the test fixes to _before_ this one, since they're independent of it. Except the reftest/crashtest one, which you should fold in here. Also, it seems like the core parts of https://bug624883.bmoattachments.org/attachment.cgi?id=8371536 can now be backed out. A followup on that, please.

New try push with rebased and folded commits: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b52f7ef17a

https://hg.mozilla.org/integration/fx-team/rev/81a25ce11f29 https://hg.mozilla.org/integration/fx-team/rev/d59fd186550c

https://hg.mozilla.org/mozilla-central/rev/81a25ce11f29 https://hg.mozilla.org/mozilla-central/rev/d59fd186550c

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/view-source-urls-can-no-longer-be-linked-from-web-pages/

Added a note: https://developer.mozilla.org/en-US/Firefox/Releases/47#Security

I'm afraid this broke [View Source] on Firefox OS. The regression window matches quite nicely.

This broke our website https://hashbang.sh The homepage has a link to view-source so that people can see what they're being asked to run.

That never worked across all browsers, and can't be expected to, since view-source: has never been a standard URL scheme. I know your current setup is clever and nice and all, but it doesn't actually tell people you're not malicious (e.g. you could be changing the source based on whether the Referer header is your page or not), so people who really care have to either manually view source anyway or download and then check the source they actually got...