smarter handling of remote chrome (and not allowing it)

VERIFIED FIXED in mozilla1.9.2a1

Status

Toolkit Graveyard
Security
VERIFIED FIXED
9 years ago
a year ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({addon-compat, dev-doc-complete, verified1.9.1})

Trunk
mozilla1.9.2a1
addon-compat, dev-doc-complete, verified1.9.1
Bug Flags:
in-testsuite +

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 349897 [details] [diff] [review]
v1.0
Attachment #349897 - Flags: superreview?(bzbarsky)
Attachment #349897 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Summary: nsChromProtocol should not treat moz-icon as remote → nsChromeProtocolHandler should not treat moz-icon as remote
(Assignee)

Updated

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review bsmedberg][needs sr bz]

Comment 4

9 years ago
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.
(Assignee)

Comment 6

9 years ago
I think I'll actually go with comment 4.  I was wondering why we did those checks anyway.
Whiteboard: [needs new patch]
(Assignee)

Comment 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
Created attachment 350048 [details] [diff] [review]
v2.0
Attachment #350048 - Flags: superreview?(bzbarsky)
Attachment #350048 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 10

9 years ago
(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.

Comment 13

9 years ago
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.
(Assignee)

Comment 15

9 years ago
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...
Attachment #350048 - Attachment is obsolete: true
Attachment #350048 - Flags: superreview?(bzbarsky)
Attachment #350048 - Flags: review?(benjamin)
(Assignee)

Comment 16

9 years ago
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]
(Assignee)

Comment 17

9 years ago
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.
Attachment #350195 - Attachment is obsolete: true
Attachment #350252 - Flags: superreview?(bzbarsky)
Attachment #350252 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch] → [has patch][needs review bsmedberg][needs sr bz]
(Assignee)

Comment 18

9 years ago
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.
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+
(Assignee)

Comment 20

9 years ago
(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?
(Assignee)

Updated

9 years ago
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.
(Assignee)

Updated

9 years ago
Summary: nsChromeProtocolHandler should not treat moz-icon as remote → smarter handling of remote chrome (and not allowing it)
(Assignee)

Comment 22

9 years ago
Created attachment 350840 [details] [diff] [review]
v3.3

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 23

9 years ago
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+
(Assignee)

Comment 24

9 years ago
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.
Attachment #350840 - Attachment is obsolete: true
Attachment #351441 - Flags: superreview?(bzbarsky)
Attachment #351441 - Flags: review?(bzbarsky)
Attachment #351441 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 26

9 years ago
(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).
(Assignee)

Comment 28

9 years ago
(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.
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 30

9 years ago
(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.
(Assignee)

Comment 31

9 years ago
Created attachment 351576 [details] [diff] [review]
v4.1

Addresses bz's review comments
Attachment #351441 - Attachment is obsolete: true
Attachment #351576 - Flags: review?(benjamin)
Attachment #351441 - Flags: review?(benjamin)

Updated

9 years ago
Attachment #351576 - Flags: review?(benjamin) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review bsmedberg][has sr] → [has patch][has review][has sr][can land]
(Assignee)

Comment 32

9 years ago
http://hg.mozilla.org/mozilla-central/rev/b6f762fde736
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][has review][has sr][can land]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 33

9 years ago
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?
(Assignee)

Comment 34

9 years ago
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 → ---
Attachment #351576 - Flags: approval1.9.1?
Comment on attachment 351576 [details] [diff] [review]
v4.1

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

Comment 36

9 years ago
Created attachment 352187 [details] [diff] [review]
v4.2

Yeah, my test changes just didn't make it in.  Weird.  This is for checkin.
Attachment #351576 - Attachment is obsolete: true
(Assignee)

Comment 37

9 years ago
OK, the right patch has been pushed:
http://hg.mozilla.org/mozilla-central/rev/242894260a86
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 38

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bf3f60f5256e
Keywords: fixed1.9.1
(Assignee)

Comment 39

9 years ago
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
Keywords: dev-doc-complete

Updated

9 years ago
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)
(Assignee)

Comment 41

9 years ago
(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?
(Assignee)

Comment 43

9 years ago
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 379761 [details]
test file

This is the test file, to be dropped in c:\Test.
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
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?
(Assignee)

Comment 49

9 years ago
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.