All moz-icons are local, so we should allow it. This issue actually makes the simple fix in bug 432938 not possible.
Created attachment 349897 [details] [diff] [review] v1.0
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 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.
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.
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.
I think I'll actually go with comment 4. I was wondering why we did those checks anyway.
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?
Created attachment 350048 [details] [diff] [review] v2.0
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?
(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.
Yeah, exactly. Sorry to make you clean this code up as you go, but it's for the greater good, I swear!
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.
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.
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.
Created attachment 350195 [details] [diff] [review] 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...
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
Created attachment 350252 [details] [diff] [review] 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.
Created attachment 350253 [details] [diff] [review] 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 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!
(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?
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.
Created attachment 350840 [details] [diff] [review] v3.3 Updated to address comment about getting the innermost URI.
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'"
Created attachment 351441 [details] [diff] [review] 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 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.
(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.
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).
(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.
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?
(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.
Created attachment 351576 [details] [diff] [review] v4.1 Addresses bz's review comments
Comment on attachment 351576 [details] [diff] [review] v4.1 Looking for approval so we can fix the bug that this blocks on branch.
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 on attachment 351576 [details] [diff] [review] v4.1 Clearing nom until it's had a chance to bake on trunk
Created attachment 352187 [details] [diff] [review] v4.2 Yeah, my test changes just didn't make it in. Weird. This is for checkin.
OK, the right patch has been pushed: http://hg.mozilla.org/mozilla-central/rev/242894260a86
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
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)
(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.
(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?
It's probably not worthwhile for you to verify it. It landed with automated tests that verify it's behavior after every checkin.
Created attachment 379762 [details] 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
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
HA! Thanks. I was just in the middle of verifying this. Thanks.
Yes, any warnings you'd get would happen at registration time.