1277685 - Nested feed: URIs should only allow http/https as inner URIs

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[:Gijs (he/him)]

See bug 1277583. While we should also prohibit other code from linking to these correctly (as that bug does), we really shouldn't bother even creating URIs to anything other than the web inside feed:.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Taking the opportunity to improve things per bug 1258071, though we have no window in newURI so can't (shouldn't?) generate DOM exceptions.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8759382 [details] [diff] [review]
Patch

Note that if you throw a numeric DOM error code XPConnect will create a DOMException for you....

Comment 3

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8759382 [details] [diff] [review]
> Patch
> 
> Note that if you throw a numeric DOM error code XPConnect will create a
> DOMException for you....

Is that actually the right thing for XPCOM implementations of things like QI, protocolhandler.newURI etc. ? Feels like those should return XPCOM exceptions... am I just missing something? :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

The general rule of thumb is this:  If your exception plans to propagate out to web content, that means there is a spec for what it should be and you should follow that spec.  If it's not going to be seen by web content, do whatever the heck you want that makes the most sense.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Gijs, are you planning on changing the patch based on comment #2?

Comment 6

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Note that if you throw a numeric DOM error code XPConnect will create a
> DOMException for you....

I can't seem to get this to work. Trying to use the constant for NS_ERROR_DOM_SECURITY_ERROR (0x80530012 - not available on Components.results, but this is what shows up when using: new DOMException("foo", "SecurityError"), so I assume is correct?):

>>> Services.io.newURI("feed:chrome://browser/content/", null, null);

20:22:47.085 [Exception... "<error>'<error>' when calling method: [nsIProtocolHandler::newURI]"  nsresult: "0x80530012 (<unknown>)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]
20:22:47.087 uncaught exception: 2152923154

if I just use the DOM number (18), I get:

>>> Services.io.newURI("feed:chrome://browser/content/", null, null);

20:21:02.283 [Exception... "JavaScript component threw a number as an exception'JavaScript component threw a number as an exception' when calling method: [nsIProtocolHandler::newURI]"  nsresult: "0x8057001f (NS_ERROR_XPC_JS_THREW_NUMBER)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: yes]
20:21:02.285 uncaught exception: 18

I guess I'm missing something... what? :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I can't seem to get this to work.

Note that the relevant codepath is conditioned on !nsContentUtils::IsCallerChrome().  Probably matters for your STR, since you're directly poking stuff from chrome code.

Now maybe that IsCallerChrome check (this is in dom::ThrowExceptionObject) shouldn't be there, of course... I haven't looked into why it's there.

Comment 8

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> > I can't seem to get this to work.
> 
> Note that the relevant codepath is conditioned on
> !nsContentUtils::IsCallerChrome().  Probably matters for your STR, since
> you're directly poking stuff from chrome code.
> 
> Now maybe that IsCallerChrome check (this is in dom::ThrowExceptionObject)
> shouldn't be there, of course... I haven't looked into why it's there.

I actually don't think these exceptions are ever web-visible. :-\

The exceptions from both bug 1258071 and anything I can think of here only show up in the browser console, not the web console. They just get swallowed there. Are there cases where we do forward these in some way? If not, I think the attached patch might actually be correct.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Are there cases where we do forward these in some way?

What happens when you try to create an XHR to a URI that fails whatever checks we're talking about?

Comment 10

[:Gijs (he/him)]

Attachment: Patch v2

For reference, per step 6 here: https://xhr.spec.whatwg.org/#dom-xmlhttprequest-open , the correct response is a SyntaxErr DOM exception, which is what the updated patch produces.

Comment 11

[:Gijs (he/him)]

Attachment: Testcase

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760727 [details] [diff] [review]
Patch v2

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

::: browser/components/feeds/FeedConverter.js
@@ -517,5 @@
> -                                                   originalCharset, baseURI);
> -    let netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
> -    const URI_INHERITS_SECURITY_CONTEXT = Ci.nsIProtocolHandler
> -                                            .URI_INHERITS_SECURITY_CONTEXT;
> -    if (netutil.URIChainHasFlags(inner, URI_INHERITS_SECURITY_CONTEXT))

Why is the check for URI_INHERITS_SECURITY_CONTEXT no longer necessary?

Comment 13

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8760727 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8760727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/feeds/FeedConverter.js
> @@ -517,5 @@
> > -                                                   originalCharset, baseURI);
> > -    let netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
> > -    const URI_INHERITS_SECURITY_CONTEXT = Ci.nsIProtocolHandler
> > -                                            .URI_INHERITS_SECURITY_CONTEXT;
> > -    if (netutil.URIChainHasFlags(inner, URI_INHERITS_SECURITY_CONTEXT))
> 
> Why is the check for URI_INHERITS_SECURITY_CONTEXT no longer necessary?

Because we're restricting to http/https, which don't have that flag.

Comment 14

[:Gijs (he/him)]

Attachment: Patch v3

Per discussion with Jared... taking out the error code adjustments, because:

- it seems that there are tests that cover the error code; unsure if it has other magical uses;
- it's technically orthogonal to this
- I think it's surprising that the individual protocol handlers have to deal with the error codes themselves when implementing XPCOM (AKA why doesn't the XHR implementation ensure that the URI creation process results in either a valid URI or a DOM-compatible error code - x-ref bug 1120398 which would also be fixed if we did that)
- I'm going to stop this being web-accessible at all in bug 1277698.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8760754 [details] [diff] [review]
Patch v3

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

r+ with the below test change

::: browser/components/feeds/test/unit/test_355473.js
@@ -36,5 @@
>    do_check_true(httpsURI.equals(httpsChannel.URI));
> -
> -  // check that we don't throw creating feed: URIs from file and ftp
> -  var ftpFeedURI = ios.newURI("feed:ftp://example.com/feed.xml", null, null);
> -  var fileFeedURI = ios.newURI("feed:file:///var/feed.xml", null, null);

Can you instead flip this to check that we *do* throw here?

Comment 16

[:Gijs (he/him)]

Attachment: Patch v3.1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's not clear there's any exploit left to be constructed, but it's fairly clear what direction to go looking in if there is.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yup, though again, not clear to what degree that'd be a wild goose chase

Which older supported branches are affected by this flaw?
n/a

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
should be trivial to have backports for beta and aurora, and even esr45 if we want it

How likely is this patch to cause regressions; how much testing does it need?
there are existing automated tests which the patch updates; I don't think it needs more than casual testing that feed links still work (which basically involves smoketesting the "subscribe to" menuitem). AFAICT those are fine with this patch.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8760841 [details] [diff] [review]
Patch v3.1

Eh, didn't realize this got a sec-other rating already.

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2974a3e8359293611f4789cf1165c6317e119f1a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2974a3e83592

Comment 20

[:Gijs (he/him)]

Comment on attachment 8760841 [details] [diff] [review]
Patch v3.1

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: people can link to feed:chrome:// URIs, which they shouldn't be allowed to do
[Describe test coverage new/current, TreeHerder]: limited test coverage
[Risks and why]: reasonably low risk. It's very early in all cycles, feed usage is hypothesized to be pretty low anyway, and the things we're restricting arguably shouldn't even have been allowed in the first place
[String/UUID change made/needed]: no.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8760841 [details] [diff] [review]
Patch v3.1

Fix a potential sec issue, taking it.
Should be in 48 beta 3

Comment 22

[Jay Gilbert]

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #7)
> > I can't seem to get this to work.
> 
> Note that the relevant codepath is conditioned on
> !nsContentUtils::IsCallerChrome().  Probably matters for your STR, since
> you're directly poking stuff from chrome code.
> 
> Now maybe that IsCallerChrome check (this is in dom::ThrowExceptionObject)
> shouldn't be there, of course... I haven't looked into why it's there.

I know I'm a little late here.  IIRC that has to do with __exposedProps__ which could be placed on certain exceptions that *used* to reach content which would allow one to possibly access functions that should be restricted.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/050a2562452a

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8908fd014ee7