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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: Gijs)

Tracking

({regression})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52+ fixed, firefox53+ fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
(not sure about the component)

Works in Nightly from 2016-11-10, doesn't work in Nightly from 2016-11-17.
I just ran into this today, also. I suspect investigating the same bug Honza was :-)
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?
Blocks: 1316889
Component: Security: CAPS → DOM: Security
Keywords: regression
Reporter

Comment 4

3 years ago
That was my though too.  Bug it's actually bug 1309310.
Blocks: CVE-2017-5391
No longer blocks: 1316889
Reporter

Updated

3 years ago
Component: DOM: Security → Security: CAPS
Ah, right.

Sounds like our "same except ref" condition needs expanding or something.  :(
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 6

3 years ago
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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bzbarsky)
Assignee

Comment 7

3 years ago
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?
Assignee

Comment 8

3 years ago
(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.)
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.
Flags: needinfo?(bzbarsky)
Assignee

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
tracking as new regression in 52
Assignee

Comment 12

3 years ago
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.)
Flags: needinfo?(bzbarsky)

Comment 13

3 years ago
mozreview-review
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?
Attachment #8814012 - Flags: review+
Attachment #8814012 - Flags: review?(bobbyholley)
Assignee

Comment 14

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)

Comment 15

3 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b14585f3efc
fix about pages linking to themselves with query parameters, r=bz
Assignee

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b14585f3efc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Comment 17

3 years ago
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
Attachment #8814012 - Flags: approval-mozilla-aurora?
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
Attachment #8814012 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter

Updated

2 years ago
Depends on: 1335272
Assignee

Comment 20

2 years ago
This patch also landed on the 51 branch as part of bug 1309310.
Assignee

Updated

2 years ago
No longer depends on: 1335272
You need to log in before you can comment on or make changes to this bug.