1318664 - Security Error: Content at about:cache may not load or link to about:cache?storage=disk&context=

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Honza Bambas (:mayhemer)]

(not sure about the component)

Works in Nightly from 2016-11-10, doesn't work in Nightly from 2016-11-17.

Comment 1

[Honza Bambas (:mayhemer)]

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d38d06f85ef59c5dbb5d4a1a8d895957a78714de&tochange=fc104971a4db41e38808e6412bc32e1900172f14

Comment 2

[Daniel Veditz [:dveditz]]

I just ran into this today, also. I suspect investigating the same bug Honza was :-)

Comment 3

[Daniel Veditz [:dveditz]]

Based on that range I'm going to blame bug 1316889 -- maybe that code wasn't as dead as we thought.

about:support (privileged about) links to other about: urls (e.g. about:plugins) without problem. What's special with about:cache?

Comment 4

[Honza Bambas (:mayhemer)]

That was my though too.  Bug it's actually bug 1309310.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, right.

Sounds like our "same except ref" condition needs expanding or something.  :(

Comment 6

[:Gijs (he/him)]

I'm not sure how to fix this in a sane way.

about: URIs don't actually claim to have a sane concept of query strings. They don't implement nsIURL, and in JS using `new URL("about:cache?foo")` produces an empty "search" member and "?foo" is part of the URL path.

How is network (presumably, the about: protocol handler? Or something?) routing about:cache?<params> to the same thing as about:cache ? Whatever that logic is would need to be included in CAPS' understanding of "is this the same URL", I guess.

Comment 7

[:Gijs (he/him)]

Eh, I guess DXR can answer my question:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolUtils.h?q=%2Bfunction%3A%22NS_GetAboutModuleName%28nsIURI+%2A%2C+nsCString+%26%29%22&redirect_type=single#27-34

    nsresult rv = aAboutURI->GetPath(aModule);
    NS_ENSURE_SUCCESS(rv, rv);

    int32_t f = aModule.FindCharInSet(NS_LITERAL_CSTRING("#?"));
    if (f != kNotFound) {
        aModule.Truncate(f);
    }


I'm going to just take a moment to be very sad that this code exists.



Given that this will need uplift to 52 which is now on aurora, the 'simple' solution involves reusing this inline function ( NS_GetAboutModule ) in the caps code and checking that both pages use the same about: module name. Boris, does that sound like the least crazy option to you, too?

Comment 8

[:Gijs (he/him)]

(The long-term option is unnesting about URIs, and then implementing them on top of nsStandardURL or whatever so some of the hacks can go away.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, I guess there's no better option, really.  Or at least I can't think of one.  :(  I hate adding this about-special-casing to CAPS; I really wish about: were not so messed up.

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1318664 - fix about pages linking to themselves with query parameters,

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

Comment 11

[Julien Cristau [:jcristau]]

tracking as new regression in 52

Comment 12

[:Gijs (he/him)]

Boris, Bobby noted on IRC that he doesn't have cycles to dive into this. Much as I hate to ask, would you mind taking the review here? (Happy to ask others as well if you have suggestions.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8814012 [details]
Bug 1318664 - fix about pages linking to themselves with query parameters,

https://reviewboard.mozilla.org/r/95156/#review96860

::: caps/tests/mochitest/browser_checkloaduri.js:55
(Diff revision 1)
>      ["feed:view-source:http://www.example2.com", false, false, false],
>      ["data:text/html,Hi", true, false, true],
>      ["view-source:data:text/html,Hi", true, false, true],
>      ["javascript:alert('hi')", true, false, true],
>    ]],
> +  ["about:foo", [

Should we test that linking to some other about: is not allowed?

Comment 14

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> Comment on attachment 8814012 [details]
> Bug 1318664 - fix about pages linking to themselves with query parameters,
> 
> https://reviewboard.mozilla.org/r/95156/#review96860
> 
> ::: caps/tests/mochitest/browser_checkloaduri.js:55
> (Diff revision 1)
> >      ["feed:view-source:http://www.example2.com", false, false, false],
> >      ["data:text/html,Hi", true, false, true],
> >      ["view-source:data:text/html,Hi", true, false, true],
> >      ["javascript:alert('hi')", true, false, true],
> >    ]],
> > +  ["about:foo", [
> 
> Should we test that linking to some other about: is not allowed?

Good idea. Will try and fix this up tomorrow.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b14585f3efc
fix about pages linking to themselves with query parameters, r=bz

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b14585f3efc

Comment 17

[:Gijs (he/him)]

Comment on attachment 8814012 [details]
Bug 1318664 - fix about pages linking to themselves with query parameters,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1309310
[User impact if declined]: about:cache doesn't work correctly
[Is this code covered by automated tests?]: it is now - they're included in this patch
[Has the fix been verified in Nightly?]: Not by QA, but the automated tests pass and it's been 2 weeks with the bug marked as fixed, so I kinda assume I would have heard if this was still massively broken.
[Needs manual test from QE? If yes, steps to reproduce]: "need" is pretty strong - I think we're pretty good with the automated tests, but otherwise:
1) ensure there's some data in the cache
2) open about:cache
3) try clicking the "list cache entries" link
ER: you should see the cache entries
AR without this fix: nothing happens, and the browser console shows a security error as in the summary of this bug.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no, it's very low-risk
[Why is the change risky/not risky?]: all it does is specialcase pages using the about: protocol when it comes to determining whether URI A can link to URI B, and there is now reasonable unit-test coverage of the code that it's changing. It's also baked on Nightly for 2 weeks now with no complaints I'm aware of.
[String changes made/needed]: nope

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8814012 [details]
Bug 1318664 - fix about pages linking to themselves with query parameters,

special case "about" scheme to fix links in about:cache, take in aurora52

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/344a95f48dde

Comment 20

[:Gijs (he/him)]

This patch also landed on the 51 branch as part of bug 1309310.