1290668 - Open view-source:https:// link in a new tab is not working on view-source:http:// pages (and vice versa)

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Swarnava Sengupta (:Swarnava)]

STR:- 
Go to view-source:http://swarnava.in/
Click on any link, or right click and click Open link on new tab.

Expected behavior:-
It should open link on a new tab

Actual behavior:-
Nothing happening

Comment 1

[Michelle Funches - QA]

Reproducible
Version 	47.0.1
Build ID 	20160623154057
User Agent 	Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0
Setting Even Handling for further investigation.
Access link above for swarnava
Open in new tab https://www.kayako.com/ 
Open in new tab https://www.mozilla.org/ 
  - Nothing Happens - No New Tab opens when clicking these links.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Olli, do you know if this is a regression?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, some links do open in a new tab, http://twitter.com/sw4rn4v4 for example, but
https://www.mozilla.org/ doesn't, nor https://www.kayako.com/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Hmm, something somewhere throws
JavaScript error: , line 0: uncaught exception: Load of view-source:https://www.mozilla.org/ from view-source:http://swarnava.in/ denied.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

This is a regression since FF46 works fine.

 NS_IMETHODIMP
 nsViewSourceHandler::GetProtocolFlags(uint32_t *result)
 {
-    *result = URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD |
+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE |
         URI_NON_PERSISTABLE;
     return NS_OK;
 }
fixes this, so backing out part of bug 1172165.

Gijs is on vacation and not accepting needinfos.

Comment 6

[Michelle Funches - QA]

I performed MozRegression - here are the results
14:11.15 INFO: Last good revision: e8d1d92fa59f8648497ba43975e11dbe45e375ad
14:11.15 INFO: First bad revision: c714d452b06e0cdb916909de3682abea7cc987e5
14:11.15 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e8d1d92fa59f8648497ba43975e11dbe45e375ad&tochange=c714d452b06e0cdb916909de3682abea7cc987e5

14:11.41 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1277583

Comment 7

[Andrew Overholt [:overholt]]

I'm taking a stab at a better component here but CAPS still doesn't feel quite right ...

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Gijs, could you please take per comment 5 and comment 6? Thanks.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Hmm, some links do open in a new tab, http://twitter.com/sw4rn4v4 for
> example, but
> https://www.mozilla.org/ doesn't, nor https://www.kayako.com/

So the issue is that not all protocols are equal when an http:// thing links to an https:// thing. It's possible we should allow this for the "are all the protocols the same" check... Would that make sense?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I'm really not at all familiar with recent changes to view-source handling and don't understand why anything was restricted for top level view-source...

But looking at code... shouldn't AllSchemesMatch do some flag checks, not scheme checks.
(and then its name should be changed). Or perhaps AllSchemesMatch should be backed out and
CheckLoadURIFlags should handle nested uris.
Does that make sense?

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] from comment #10)
> I'm really not at all familiar with recent changes to view-source handling
> and don't understand why anything was restricted for top level view-source...

We restricted the top-level view-source: protocol because it's been an ongoing part of security issues, e.g. bug 1248209. It's basically not safe-for-web. IE removed access to it years ago, we did it more recently, to avoid shooting ourselves in the foot with it any longer.

> But looking at code... shouldn't AllSchemesMatch do some flag checks, not
> scheme checks.

Almost all flag checks already happen across the entire nested URI chain (I just noticed we missed out URI_IS_UI_RESOURCE, which doesn't impact this bug, or any others, AIUI). This was the change that bug 1172165 made, in fact. Previously, we did flag checks *on the inner URI*. So if you had:

view-source:jar:file:///

the flag checks happened on file:///

and if you had:

view-source:data:

then it happened on data: .

This meant that restricting view-source was impossible, because flag checks happened on the inner URI, ignoring flags on the outer URI.

Simultaneously, we used to have a check for the inner schemes to be equal that ignored flags. So if you had a foo:// URI it could always link to another foo:// URI. But also, view-source:jar:foo:// could link to foo:// and http:// could link to view-source:http:// .

So we changed the flag checks to happen on the entire URI (outer-to-inner) and made view-source "dangerous to load". If *any* of the protocols in the URI chain have that flag, we deny access.

Unfortunately, that wouldn't be enough to stop http:// linking to view-source:http://, because the inner schemes are still the same, and we do the scheme equality check before the flag checks. So we also changed the scheme matching to check *all* the schemes. So we only ignore protocol flags if the entire scheme chain is identical, so jar:file:// can link to jar:file://, but file:// can only link to jar:file:// if the protocol flags allow it to do so.

> (and then its name should be changed). Or perhaps AllSchemesMatch should be
> backed out and
> CheckLoadURIFlags should handle nested uris.
> Does that make sense?

Based on the above, I don't think so? CheckLoadURIFlags already uses aTargetURI for the check for DANGEROUS_TO_LOAD, which is a flag that's set for view-source (and for feed:, by now, in fact...). Maybe you mean something else when you say "handle" nested URIs?

In a way, the issue is that one DANGEROUS_TO_LOAD URI/principal can't load another one unless all their schemes are identical. In fact, even if we relaxed that last check to make it check only the inner schemes, that would still break view-source:http to view-source:https links.

Thinking about this a bit more, I'm not even convinced my earlier suggestion to treat http and https as identical for the purposes of AllSchemesMatch is a sane thing to do. Right now, the plain linking of http to https passes through the flag checking path where both have URI_LOADABLE_BY_ANYONE, but linking from http to http passes through the "these schemes are equal" path (and did so before the changes in bug 1172165 as well).

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Gijs, I am inferring this is something to fix in the next release/in a few months, so set P2. Please change it as you see fit.

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Hi Gijs, I am inferring this is something to fix in the next release/in a
> few months, so set P2. Please change it as you see fit.

P2 sounds OK. I just wish we had an idea about *how* to fix, exactly, short of very ugly view-source specific hacks...

Comment 14

[Andrew Overholt [:overholt]]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > Hi Gijs, I am inferring this is something to fix in the next release/in a
> > few months, so set P2. Please change it as you see fit.
> 
> P2 sounds OK. I just wish we had an idea about *how* to fix, exactly, short
> of very ugly view-source specific hacks...

Have you had any ideas recently? :)

Comment 15

[:Gijs (out for now; he/him)]

The best thing I can think of is a new protocol flag called something like "URI_LOADABLE_IF_INNER_LOADABLE" or whatever, and iff

- both URI A and URI B have the same scheme/protocol
- that protocol has that flag
- both have inner URIs

we return the result of CheckLoadURIWithPrincipal for the two inner URIs.

Of course, we could consider doing this without a special flag if both URIs have the same outer protocol and both have an inner URI (though we might need to change some of the reporting so it's clearer, ie so we complain about e.g. foo:bar can't link to foo:baz rather than just bar can't link to baz). I just don't know if that would break anything else. Olli/Boris, thoughts / sanity checks?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Still trying to page this stuff back in, but it seems like a reasonable general policy might be to pass CheckLoadURI checks if, going through the entire protocol chain, at each step the first protocol can link to the second.  That is, either they're identical or the second one allows linking from the first one in general.

Comment 17

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

Most likely too late for 49 (building RC2 tomorrow), but if someone works on this it could still land for 50.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this something we should try to find cycles to fix this cycle, Gijs?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(And is this something we could perhaps mentor another engineer to work on if you don't have the cycles?)

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> (And is this something we could perhaps mentor another engineer to work on
> if you don't have the cycles?)

It's not that I'm unwilling to mentor someone, but I'm worried that while comment 16 is a general direction, I'm not sure how many problems you'd run into when implementing it. That is, when Boris says "still trying to page this stuff back in", and from my experience with this (having to fix reftest frameworks, edgecases, getting it wrong initially and having to crashland something on RC not to ship a vulnerable browser, ...) it's totally possible that when we try to do what comment 16 suggests, we run into issues on the way. It'd also be a significant refactoring of that bit of caps/ code, which not a lot of people are awfully familiar with. So all in all, doesn't seem like an amazing fit for mentoring new-ish contributors. If there are new platform engineers that we pay to get stuck into this kind of thing, sure... but I don't know where to find those people, if they even exist.

I think most things in my queue today will be finished in relatively short order (famous last words), so I might just see if I can at least get a WIP / some more ideas about how hard this is before either taking myself or mentoring someone else. I'll leave needinfo.

Comment 21

[:Gijs (out for now; he/him)]

Attachment: Bug 1290668 - unbreak view-source links between http and https pages,

Review commit: https://reviewboard.mozilla.org/r/81394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81394/

Comment 22

[:Gijs (out for now; he/him)]

I ripped out the b2g code because AIUI it's now officially dead ( https://groups.google.com/forum/#!msg/mozilla.dev.fxos/FoAwifahNPY/Lppm0VHVBAAJ ) and it was in the way. I could potentially put it back because I ended up with (ignoring the view-source case) only one NS_OK, so I guess I could reinsert it there, maybe with a helper or something to keep that loop readable? But if we're killing it anyway...

Comment 23

[:Gijs (out for now; he/him)]

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

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

I prefer smaug to review this code.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

https://reviewboard.mozilla.org/r/81394/#review80334

::: caps/nsScriptSecurityManager.cpp
(Diff revision 1)
> -                return NS_ERROR_DOM_BAD_URI;
> -            }
> +    }
> +
> +    // If we get here, check all the schemes can link to each other, from the top down:
> +    auto stringComparator = nsCaseInsensitiveCStringComparator();

Simpler to write:
nsCaseInsensitiveCStringComparator stringComparator;

::: caps/tests/mochitest/browser_checkloaduri.js:47
(Diff revision 1)
>      ["feed:https://www.example2.com", false, false, true],
>      ["chrome://foo/content/bar.xul", false, false, true],
>      ["feed:chrome://foo/content/bar.xul", false, false, false],
>      ["view-source:http://www.example2.com", true, true, true],
> +    ["view-source:https://www.example2.com", true, true, true],
>      ["view-source:feed:http://www.example2.com", false, false, true],

Could you add also tests for view-source:http* linking to view-source:data://*

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2554774482
unbreak view-source links between http and https pages, r=smaug

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hey Gijs, should we uplift this to Aurora51/Beta50?

Comment 29

[:Gijs (out for now; he/him)]

I'm not convinced about how safe this is to uplift. Thoughts, Olli?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

aurora maybe, but beta feels a bit risky. This needs some baking time.

Comment 31

[:Gijs (out for now; he/him)]

Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

Approval Request Comment
[Feature/regressing bug #]: bug 1172165
[User impact if declined]: inter-view-source links that traverse from http to https or vice versa are broken
[Describe test coverage new/current, TreeHerder]: there are automated tests that cover this
[Risks and why]: low to medium. Changes to this bit of CAPS don't have an awesome history risk-wise (hi, bug 1277583). OTOH, there are now tests for obvious breakage, so we should have noticed if I made another stupid mistake on that scale. As Olli says, this needs baking. It's still another 3-4 weeks until aurora goes to beta, so there is indeed some actual baking potential. :-)
[String/UUID change made/needed]: no.

Comment 32

[:Gijs (out for now; he/him)]

TBH, if sheriffs are keen to take this, you can... but I personally don't think the regression is bad enough to warrant the extra risk of uplifting. Very few users use view-source:, fewer users have links affected by this bug, and we already shipped this (twice!).

Comment 33

[Ryan VanderMeulen [:RyanVM]]

To be clear, s/sheriffs/RelMan

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> To be clear, s/sheriffs/RelMan

Ugh, yes. Sorry. It's late, I should leave bugzilla until tomorrow.

Comment 35

[Gerry Chang [:gchang]]

Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

Fix a regression. Take it in 51 aurora.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/121e2e591c88

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on Smaug and Gij's assessment and the fact that this is a regression since 48 (at least), we'll let this fix ride the 51 train.