466582 - smarter handling of remote chrome (and not allowing it)

Status: VERIFIED FIXED

(Toolkit Graveyard :: Security, defect)

Description

[Shawn Wilsher :sdwilsh]

All moz-icons are local, so we should allow it.  This issue actually makes the simple fix in bug 432938 not possible.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Boris Zbarsky [:bzbarsky]]

How mad would you get at me if I asked you to nuke this whole mess (which is wrong anyway, since it allows jar:http://) and replace it with a protocol flag to be checked on the URI of the channel URI instead?  That would allow any protocol that's really local to work here without any more hacks in the future.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 349897 [details] [diff] [review]
v1.0

I wouldn't be mad at all.  There's a good reason why I requested sr from you ;)

Is there some documentation about this protocol flag (either in code, or a wiki) that you can point me at?  I'd like to do this right, but at the same time not sure where to set/add this flag.

Comment 4

[Benjamin Smedberg]

Really, can we just remove the whole mess and not replace it? As a security precaution this is pretty meaningless; it just guards poorly against bad coding practices.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'd be fine with comment 4 too.

As far as flags, you'd add a new flag on nsIProtocolHandler, have the relevant protocols' GetProtocolFlags return it, then use NS_URIChainHasFlags to check for it.

Comment 6

[Shawn Wilsher :sdwilsh]

I think I'll actually go with comment 4.  I was wondering why we did those checks anyway.

Comment 7

[Shawn Wilsher :sdwilsh]

I've got a patch now that removes the if else block here:
http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeProtocolHandler.cpp#557

I just realized that you might have only wanted the else part removed though.  Which do you want?

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350048 [details] [diff] [review]
v2.0

If you're changing that nsIJARURI stuff anyway, want to just use NS_GetInnermostURI from nsNetUtil.h?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> (From update of attachment 350048 [details] [diff] [review])
> If you're changing that nsIJARURI stuff anyway, want to just use
> NS_GetInnermostURI from nsNetUtil.h?
Bah - I meant to attach a -w diff there since it's just a whitespace change, but I can change that too.  That get's rid of the while loop and makes the code easier to read.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, exactly.  Sorry to make you clean this code up as you go, but it's for the greater good, I swear!

Comment 12

[Daniel Veditz [:dveditz]]

With v2.0 what prevents "remote chrome"? Nothing, as far as I can see, and we're not ready for that. Sure, ill behaved extensions can XHR+eval remote scripts, but at least they have to explicitly work around the restrictions.

The fact that the restriction was buggy isn't a reason to defeat it entirely. I still like Boris's suggestion in comment 2, and you don't even need to add a new protocol flag: URI_IS_UI_RESOURCE should already work for the protocols you want (plus URI_IS_LOCAL_FILE). Just be sure to get the innermost URI.

Comment 13

[Benjamin Smedberg]

There are much easier ways of preventing an extension from doing something unintentionally stupid. We could check the URLs at registration time for an http/https blacklist: this would give the same general benefit without having to introduce complex code into the actual channel-load codepath.

Although I still think that trying to prevent remove chrome at this level is security theater, and not worth the effort at all.

Comment 14

[Daniel Veditz [:dveditz]]

It's more like a low cement curb dividing a busy boulevard -- sure, your jeep could drive right over it, but you're going to notice and know that the road designers thought it was unsafe to turn left at that spot.

I'd be happy to see these checks moved to registration time -- I was very surprised to see no checking or validation done at all there.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v3.0

This is more inline with what has been suggested.  I still want to write some tests, but I'm in the middle of a full rebuild since I accidentally pulled and updated...

Comment 16

[Shawn Wilsher :sdwilsh]

We also have the right flags already set for the moz-icon protocol:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIconProtocolHandler.cpp#84

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v3.1

This is the same as v3.0, but with tests.  I have test coverage for content, locale, skin, override, and resource.  There is no way to test overlay and style as far as I can tell.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v3.2

I forgot to fix the other failing test.  We don't currently have any protocols that I can use that are not mapped to the file system for that test.  I think the test I added though covers what I had to remove in that test.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350253 [details] [diff] [review]
v3.2

I'd skip the (void) stuff (and in my more paranoid moments, actually check the rv there).  Also, you don't need to get the innermost URI before calling URIChainHasFlags.  In fact, you don't even want to.  That function walks the inner URI chain and such.

With those nits, looks great.  Thanks for writing those tests!

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> I'd skip the (void) stuff (and in my more paranoid moments, actually check the
> rv there).  Also, you don't need to get the innermost URI before calling
> URIChainHasFlags.  In fact, you don't even want to.  That function walks the
> inner URI chain and such.
Ignoring the return values should be safe there since we assume it's not satisfying the requirement (and XPCOM rules say don't modify out params if you fail which I believe is being statically checked).  As for (void), I'll let bsmedberg comment since he's the module owner.  I personally think it makes code clearer, but I know I'm generally out numbered on that.

NS_GetInnermostURI checks the outermost uri first for the flags.  If it doesn't have them, it keeps going deeper.  I was worried about some case where the outermost uri has the flag, but the innermost one does not, and so we shouldn't really load it.  Is that case just bogus?

Comment 21

[Boris Zbarsky [:bzbarsky]]

In my opinion, yes.  If someone creates such a URI, they have a buggy protocol handler.  At that point, they could just lie about their flags, lie about having the inner URI, etc.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v3.3

Updated to address comment about getting the innermost URI.

Comment 23

[Benjamin Smedberg]

Comment on attachment 350840 [details] [diff] [review]
v3.3

r=me with nits:

* add a test to make sure that data: URIs work as chrome mappings
* please change the console message to say

"Warning: cannot register non-local chrome URI '%s'"

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v4.0

This adds a new flag as discussed on irc and tests for data uri's as well.

I'm re-requesting review given the changes I had to make.  I also need bz to r since I'm changing stuff in network.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 351441 [details] [diff] [review]
v4.0

Seems fine, but drop the random whitespace changes (esp. in the necko IDL), and I would think that just checking for URI_IS_LOCAL_RESOURCE is enough in your chrome reg check, no?  I'm fine with leaving the URL classifier check as-is and filing a followup bug on simplifying that code too.

Oh, and we should think about adding this flag to about:, but that can be a separate bug if we want to do it.

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
> (From update of attachment 351441 [details] [diff] [review])
> Seems fine, but drop the random whitespace changes (esp. in the necko IDL), and
> I would think that just checking for URI_IS_LOCAL_RESOURCE is enough in your
> chrome reg check, no?  I'm fine with leaving the URL classifier check as-is and
> filing a followup bug on simplifying that code too.
The only reason why I'm checking all three still is because if someone wants to support Firefox 3 and 3.1, they'd probably need to set one of the other two and not this.  Maybe I don't need to do that, but it seemed like a good idea at the time.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Er... why?  This flag is not mutually exclusive with the others.  That's the whole point.  They'd set the other flags (which are all about who's allowed to link to the content) _and_ this one (which is all about how the content behaves when being loaded).

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> Er... why?  This flag is not mutually exclusive with the others.  That's the
> whole point.  They'd set the other flags (which are all about who's allowed to
> link to the content) _and_ this one (which is all about how the content behaves
> when being loaded).
Mostly because this would be an addon-breaking change, and we are past beta 2 (I'd really like to get this in for 1.9.1).  I can do a followup to fix it right for mozilla-central if you want.

Comment 29

[Boris Zbarsky [:bzbarsky]]

addon-breaking in what sense?

The only thing that I can see breaking is that if someone has chrome that currently lives at a URI whose protocol handler is implemented by an add-on and has the URI_IS_LOCAL_FILE or URI_IS_UI_RESOURCE flag then your patch allows loading that URI as chrome but just checking URI_IS_LOCAL_RESOURCE wouldn't?  But that chrome is not allowed right now anyway...

Am I missing something?

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> Am I missing something?
bsmedberg and I talked about it on irc, and I'm now convinced that just checking for the new flag during chrome registration is sufficient.

I'll upload a new patch in just a bit that addresses your review comments.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v4.1

Addresses bz's review comments

Comment 32

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/b6f762fde736

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 351576 [details] [diff] [review]
v4.1

Looking for approval so we can fix the bug that this blocks on branch.

Comment 34

[Shawn Wilsher :sdwilsh]

I backed this out due to a test failure.  It seems like I managed to not qrefresh or something, but I didn't have time to investigate it today.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 351576 [details] [diff] [review]
v4.1

Clearing nom until it's had a chance to bake on trunk

Comment 36

[Shawn Wilsher :sdwilsh]

Attachment: v4.2

Yeah, my test changes just didn't make it in.  Weird.  This is for checkin.

Comment 37

[Shawn Wilsher :sdwilsh]

OK, the right patch has been pushed:
http://hg.mozilla.org/mozilla-central/rev/242894260a86

Comment 38

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bf3f60f5256e

Comment 39

[Shawn Wilsher :sdwilsh]

Added documentation:
https://developer.mozilla.org/index.php?title=En%2FFirefox_3.1_for_developers&diff=0&revision=153
https://developer.mozilla.org/index.php?title=En%2FNsIProtocolHandler&diff=0&revision=5

Comment 40

[u279076]

Is there a manual test case that can be performed to verify this bug FIXED? (apart from creating an add-on that reflects the documentation changes)

Comment 41

[Shawn Wilsher :sdwilsh]

(In reply to comment #40)
> Is there a manual test case that can be performed to verify this bug FIXED?
> (apart from creating an add-on that reflects the documentation changes)
The only thing I can think of is to make an add-on.

Comment 42

[u279076]

(In reply to comment #41)
> (In reply to comment #40)
> > Is there a manual test case that can be performed to verify this bug FIXED?
> > (apart from creating an add-on that reflects the documentation changes)
> The only thing I can think of is to make an add-on.

Ok...that said, I've come to two conclusions:
1. Building an add-on to verify this is a bit over my head as far as add-on dev is concerned
2. Building an add-on to verify this would considerably divert my attention away from higher priority 3.5-related tasks

Can you suggest another way to verify this bug?

Comment 43

[Shawn Wilsher :sdwilsh]

It's probably not worthwhile for you to verify it.  It landed with automated tests that verify it's behavior after every checkin.

Comment 44

[Nochum Sossonko [:Natch]]

Attachment: test file

This is the test file, to be dropped in c:\Test.

Comment 45

[Nochum Sossonko [:Natch]]

Attachment: manifest file

manifest file, to be dropped in the application install dir under "chrome"

In 3.0 the page has a red box because the binding works, on trunk the binding is ignored.

After both files are saved, navigate to chrome://Test/content/test.xul

Comment 46

[Nochum Sossonko [:Natch]]

Verified with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090525 Minefield/3.6a1pre

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre

Comment 47

[u279076]

HA!  Thanks.  I was just in the middle of verifying this.  Thanks.

Comment 48

[Nochum Sossonko [:Natch]]

FTR there are other manifestations of this bug, see the tests in the patch. Specifically, using a data: uri for the actual content of the chrome package seems to be what this bug is really about. Either way, simply copy/pasting the test file's manifests and dropping the file into the 'chrome' directory should be sufficient to test those cases (to test, set javascript.options.strict to true and you should get a strict warning when the package is registered).

I didn't check that, seems redundant IMO. I also didn't get any strict warnings when I loaded the package, but I don't think there should be any, it seems the warnings are only thrown at registration time?

Comment 49

[Shawn Wilsher :sdwilsh]

Yes, any warnings you'd get would happen at registration time.