Closed Bug 466582 Opened 16 years ago Closed 16 years ago

smarter handling of remote chrome (and not allowing it)

Categories

(Toolkit Graveyard :: Security, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

(Keywords: addon-compat, dev-doc-complete, verified1.9.1)

Attachments

(3 files, 8 obsolete files)

All moz-icons are local, so we should allow it.  This issue actually makes the simple fix in bug 432938 not possible.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #349897 - Flags: superreview?(bzbarsky)
Attachment #349897 - Flags: review?(benjamin)
Summary: nsChromProtocol should not treat moz-icon as remote → nsChromeProtocolHandler should not treat moz-icon as remote
Whiteboard: [has patch][needs review bsmedberg][needs sr bz]
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.
Attachment #349897 - Attachment is obsolete: true
Attachment #349897 - Flags: superreview?(bzbarsky)
Attachment #349897 - Flags: review?(benjamin)
Whiteboard: [has patch][needs review bsmedberg][needs sr bz]
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.
Whiteboard: [needs new patch]
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?
Attached patch v2.0 (obsolete) — Splinter Review
Attachment #350048 - Flags: superreview?(bzbarsky)
Attachment #350048 - Flags: review?(benjamin)
Whiteboard: [needs new patch] → [has patch][needs review bsmedberg][needs sr bz]
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.
Attached patch v3.0 (obsolete) — Splinter Review
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...
Attachment #350048 - Attachment is obsolete: true
Attachment #350048 - Flags: superreview?(bzbarsky)
Attachment #350048 - Flags: review?(benjamin)
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
Whiteboard: [has patch][needs review bsmedberg][needs sr bz] → [needs new patch]
Attached patch v3.1 (obsolete) — Splinter Review
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.
Attachment #350195 - Attachment is obsolete: true
Attachment #350252 - Flags: superreview?(bzbarsky)
Attachment #350252 - Flags: review?(benjamin)
Whiteboard: [needs new patch] → [has patch][needs review bsmedberg][needs sr bz]
Attached patch v3.2 (obsolete) — Splinter Review
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.
Attachment #350252 - Attachment is obsolete: true
Attachment #350253 - Flags: superreview?(bzbarsky)
Attachment #350253 - Flags: review?(benjamin)
Attachment #350252 - Flags: superreview?(bzbarsky)
Attachment #350252 - Flags: review?(benjamin)
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!
Attachment #350253 - Flags: superreview?(bzbarsky) → superreview+
(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?
Whiteboard: [has patch][needs review bsmedberg][needs sr bz] → [has patch][needs review bsmedberg][has sr]
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.
Summary: nsChromeProtocolHandler should not treat moz-icon as remote → smarter handling of remote chrome (and not allowing it)
Attached patch v3.3 (obsolete) — Splinter Review
Updated to address comment about getting the innermost URI.
Attachment #350253 - Attachment is obsolete: true
Attachment #350840 - Flags: review?(benjamin)
Attachment #350253 - Flags: review?(benjamin)
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'"
Attachment #350840 - Flags: review?(benjamin) → review+
Attached patch v4.0 (obsolete) — Splinter Review
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.
Attachment #350840 - Attachment is obsolete: true
Attachment #351441 - Flags: superreview?(bzbarsky)
Attachment #351441 - Flags: review?(bzbarsky)
Attachment #351441 - Flags: review?(benjamin)
Whiteboard: [has patch][needs review bsmedberg][has sr] → [has patch][needs review bsmedberg][needs sr bz]
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.
Attachment #351441 - Flags: superreview?(bzbarsky)
Attachment #351441 - Flags: superreview+
Attachment #351441 - Flags: review?(bzbarsky)
Attachment #351441 - Flags: review+
(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.
Whiteboard: [has patch][needs review bsmedberg][needs sr bz] → [has patch][needs review bsmedberg][has sr]
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.
Attached patch v4.1 (obsolete) — Splinter Review
Addresses bz's review comments
Attachment #351441 - Attachment is obsolete: true
Attachment #351576 - Flags: review?(benjamin)
Attachment #351441 - Flags: review?(benjamin)
Attachment #351576 - Flags: review?(benjamin) → review+
Whiteboard: [has patch][needs review bsmedberg][has sr] → [has patch][has review][has sr][can land]
http://hg.mozilla.org/mozilla-central/rev/b6f762fde736
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has sr][can land]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 351576 [details] [diff] [review]
v4.1

Looking for approval so we can fix the bug that this blocks on branch.
Attachment #351576 - Flags: approval1.9.1?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 351576 [details] [diff] [review]
v4.1

Clearing nom until it's had a chance to bake on trunk
Attached patch v4.2Splinter Review
Yeah, my test changes just didn't make it in.  Weird.  This is for checkin.
Attachment #351576 - Attachment is obsolete: true
OK, the right patch has been pushed:
http://hg.mozilla.org/mozilla-central/rev/242894260a86
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Keywords: late-compat
Flags: in-testsuite+
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.
Attached file test file
This is the test file, to be dropped in c:\Test.
Attached file 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
Status: RESOLVED → VERIFIED
HA!  Thanks.  I was just in the middle of verifying this.  Thanks.
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?
Yes, any warnings you'd get would happen at registration time.
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: