See bug 406777 discussion; this bug is about the cross-domain aspect only. Note that this change breaks the UI in some cases; it can be argued that we don't care about those cases, or we might need corresponding UI changes. This bug needs UI owner feedback, basically; I'm not going to work on driving the patch in until/unless that happens.

Copied from http://www.maths.usyd.edu.au/u/psz/ff-utf7-uxss.html (to be saved here for posterity)

I now updated my "demo" page http://www.maths.usyd.edu.au/u/psz/ff-utf7-uxss.html to refer to this bug 408457 also, and saved it here as an attachment. Could you please add '("UTF-7 universal XSS")' to the summary of this bug, and take it off https://bugzilla.mozilla.org/show_bug.cgi?id=406777 ? (I do not seem to be able to do this myself.) About your proposed patch: there you have the lines if (source < aCharsetSource) return PR_TRUE; + // Make sure it's OK to look at this parent document + if (!aParentDocument || !CheckSameOrigin(this, aParentDocument)) { + return PR_FALSE; + } Should not those tests be re-ordered: add the new test above, not below, the source comparison, to ensure we return FALSE when "danger"? (I may be completely off-track here.)

> Could you please add '("UTF-7 universal XSS")' to the summary of this bug, and > take it off https://bugzilla.mozilla.org/show_bug.cgi?id=406777 ? I don't see why it should come off the other summary, since the other bug is precisely about that issue. I'm adding it to this bug. For what it's worth, if you file your own bugs, you'd be able to edit them too. ;) > Should not those tests be re-ordered Doesn't much matter from a behavior standpoint; if source < aCharsetSource the parent charset is simply ignored (since the child already has a "more important" charset specification). So the order is actually an optimization to avoid calling CheckSameOrigin when its return value is irrelevant. Would you be willing to upload a slightly different testcase, one with an outer document and a subframe that just has some UTF-7 encoded stuff, no dependence on google. We can set them up on different hosts in the regression tests when we check them in. Even better would be a fully working mochitest for this, but that might involve a bit of hacking around with document viewers, and I can understand if you don't really feel like doing that.

> I don't see why it should come off the other summary, since the other bug is > precisely about that issue. Seems to me that the "other" bug is about overrides on the one site. There is nothing "cross-site" in that, I do not see that XSS is relevant. Misparse of the one site maybe, or "lone user shoots himself in the foot"? On the other hand, the current title suggests it is security-related and dangerous, so there is some hope that will also get fixed. Except that I do not think there is a bug there, so nothing to fix. Hmm... maybe the title is bad because it diverts the attention of the security people? > ... if you file your own bugs, you'd be able to ... I am learning, slowly: I can already put comments, add attachments, provide testcases ... >> Should not those tests be re-ordered > Doesn't much matter from a behavior standpoint ... the order is actually > an optimization ... Thanks for the explanation. > Would you be willing to upload a slightly different testcase ... no > dependence on google. We can set them up on different hosts ... Sorry, I do not think I can do this myself. We need two hosts, and I only "control" my own www.maths.usyd.edu.au. (Not strictly true, I also have access to another USyd site, but I do not think that should be involved.) However, you can substitute the google reference by practically any site that uses Apache (and the default 404 error page): try www.apache.org maybe? (That is exactly why this bug is dangerous: it works on practically any sites, thus the name "universal" XSS.) (I expect that other similar XSS things can be found for IIS also, but I do not think for their default 404 error page; and I do not have an IIS server to play with.) > Even better would be a fully working mochitest ... I have no idea on what you mean.

(In reply to comment #4) > > Would you be willing to upload a slightly different testcase ... no > > dependence on google. We can set them up on different hosts ... > > Sorry, I do not think I can do this myself. We need two hosts, and I only > "control" my own www.maths.usyd.edu.au. Anything on bugzilla.mozilla.org is also on a few other domains. See bug 332179 comment 14 for a list. This should be plenty to demo these bugs, in general. > > Even better would be a fully working mochitest ... > > I have no idea on what you mean. Full details are at <http://developer.mozilla.org/en/docs/Mochitest>. If you feel like setting up a Firefox build environment (not at all hard, just requires a few hours of time), you can write tests that follow that style and check that they work correctly without too much effort. This would be ideal, but as it requires a fair amount of effort to set up don't feel obligated to do this. We actually should take some steps on our end to make these easier to author/test without having to have a build environment...

bz, i am shooting in the dark: does your patch works for nested iframes. i.e. pageA -> iframeA -> iframeOTHER

> Anything on bugzilla.mozilla.org is also on a few other domains. Since the "uploaded" testcase is hosted on bugzilla.mozilla.org, could use that to attack developer.mozilla.org. But that would not be "impressive", because developer.m.o may not have any cookies of interest. Would be nicer to steal the https://bugzilla.mozilla.org/ cookies; yes I could do that, but so can (any of) you (with access to any, even non-mozilla, webserver). > Full details are at <http://developer.mozilla.org/en/docs/Mochitest>. Thanks for the pointer. However, at the risk of being accused of laziness ("expect someone else to do the work for you" were the exact words), I decline.

You can make a demo that steals bugzilla.mozilla.org cookies by uploading a testcase and loading it from http://test1.bugzilla.mozilla.org/.

(In reply to comment #8) > You can make a demo that steals bugzilla.mozilla.org cookies by uploading a > testcase and loading it from http://test1.bugzilla.mozilla.org/. > well, this is quite hypocritical. b.m.o stores testcases as html and servers them from b.m.o. why bother using another host?

Jesse wrote: > ... demo that steals bugzilla.mozilla.org ... loading from > test1.bugzilla.mozilla.org ... Two details. If I upload, then this bug will "contain" the attachment from bugzilla. The user would manually need to change the host to test1. The testcase html might have an href reference to test1, but how would it know the attachment ID it is going to get? (Of course clever JS could get that...) And since test1.bugzilla has "direct" access to bugzilla cookies, there is no "exploit". Georgi wrote: > ... why bother using another host? Because we want to show that any host can steal b.m.o cookies. That would prove that firefox is at fault, since we know that b.m.o is perfect.

(In reply to comment #10) > > ... why bother using another host? > > Because we want to show that any host can steal b.m.o cookies. That would > prove that firefox is at fault, since we know that b.m.o is perfect. > b.m.o is serving arbitrary text/html *and* application/trojan via bug attachments (of reasonable size) stealing b.m.o cookies is like beating a dead horse is still stealing b.m.o cookies a challenge?

i think this is right: 1. create an attachment to be loaded from test1.bugzilla.mozilla.org and have it set cookie for test1.bugzilla.mozilla.org 2. create an attachment to be loaded from test2.bugzilla.mozilla.org and try to steal the cookie from test1.bugzilla.mozilla.org

Georgi, Of course (in the sense you mean) b.m.o is wide open to abuse. But we want to prove that FF is broken, that it allows cross-site interaction leading to (cookie theft or other) "data loss" from practically any victim site. The one site BZ trusts is b.m.o, so we attack his baby. And no, we do not make such a testcase: we know it would be pointless. Attacking google is "cool enough" for most of us. About your earlier comment: > does your patch works for nested iframes pageA -> iframeA -> iframeOTHER Maybe it does, as it stops the propagation at either cross-domain boundary? Be that as it may, since the override charset seems to be a property of the toplevel document, I think I would like to add a test like if (!aTopDocument || !CheckSameOrigin(this, aTopDocument)) { return PR_FALSE; } also (or however you "say" that in Mozilla code).

Sure, attacking Google is "cool", but having a testcase in Bugzilla allows us to test the bug/fix even if Google changes, and gets us closer to having an automated regression test.

> ... having a testcase [fully] in Bugzilla ... Take my testcase, substitute google for any website you can think of (not the very-same site you serve from: www.mozilla.org, or www.apache.org maybe). Do you really want me to do this? I am sure google will not change, but can you guarantee any of those others won't either?

(In reply to comment #14) > Sure, attacking Google is "cool", but having a testcase in Bugzilla allows us > to test the bug/fix even if Google changes, and gets us closer to having an > automated regression test. > this makes sense. but then you need setup up a server on another domain, don't make it wide open by allowing uploading arbitrary content. and you may make additional subdomains with little bugs like no character set, etc. test{1,2}.b.m.o were mentioned but for me they redirect to b.m.o so i can't use them

Hmm, they redirect if you use http, but not if you use https. But they don't have valid certificates for https. Doh.

https://test1.bugzilla.mozilla.org/ and https://test2.bugzilla.mozilla.org/ work fine for me (no redirects or invalid certs). If there are problems perhaps they're related to bug 401005.

> There is nothing "cross-site" in that The other bug is about the fact that any site that allows plaintext injection (e.g. the Google page involved) is vulnerable if the user loads it and changes the charset (which the injected plaintext can tell him to do). Since you say that the Google page is not doing anything unsafe, clearly the fact that it's vulnerable is a browser bug. The cross-site aspect is the injection. > could use that to attack developer.mozilla.org. I'm not asking for a testcase that attacks anything, nor even a testcase that does XSS. I just want a single HTML page that, when decoded as UTF7, executes some script, or better yet some easy instructions for transcoding ASCII to UTF7 so that I can write said script myself. Given that, I can create a testcase in mochitest which will put the pages on different servers, etc, etc. The rest of the discussion in this bug is really pointless as far as this bug is concerned; if there is breakage with the bugzilla multi-domain testing setup can someone please get a bug filed on it?

> The other bug is about ... The cross-site aspect is the injection. Though the "bug" is the user-requested mis-parse and not the injection, please hang on to the XSS name if you wish. > I'm not asking for a testcase ... [just] some easy instructions for > transcoding ASCII to UTF7 ... Sorry, I did not imagine that you did not already know how to do that. Examples for Unix perl below (single lines, may display wrapped). perl -e '$c = "<"; use MIME::Base64; $x = encode_base64("\x00$c"); $x =~ s/=\n//; print "Char $c is +$x- in UTF-7\n"' perl -e 'use MIME::Base64; foreach (32..126) { $c = chr($_); $x = encode_base64("\x00$c"); $x =~ s/=\n//; print "Char $c is +$x- in UTF-7\n" }' perl -e 'use MIME::Base64; $line = "Gotcha!"; $u = ""; foreach (split(//,$line)) { $x = encode_base64("\x00$_"); $x =~ s/=\n//; $u .= "+$x-" } print "Line $line is $u in UTF-7\n"'

the scientific way for converting is ICONV(1) (though it lacks some features like alternative encodings) $ echo '<script>alert(1)</script>' | iconv -f ascii -t utf-7 +ADw-script+AD4-alert(1)+ADw-/script+AD4

The *.mozilla.org cert is invalid for subdomains of bugzilla.mozilla.org. Firefox currently allows cert wildcards to match dots (bug 159483) but Safari and IE do not.

OK, so, this has been nom'd, but can I get a few comments on how we should prioritize this?

> ... how we should prioritize this? It is a security risk. The fix is trivial (a simple one- or two-line change), "arguably" without any side-effects. So I say: do it now.

(In reply to comment #19) > I'm not asking for a testcase that attacks anything, nor even a testcase that > does XSS. I just want a single HTML page that, when decoded as UTF7, executes > some script, See attachment 247715 [details] in bug 356280. It's framing http://www.mozilla.org/projects/security/utf7xss.html http://ha.ckers.org/utf-7.html If this bug is fixed then that testcase should not alert twice if you force the testcase page to be utf-7 (it already is utf-7, but the act of forcing it currently overrides the frames). Neither page is likely to go anywhere, but if you're putting this in a mochitest we probably shouldn't be hitting external sites repeatedly, especitally not ha.ckers.org > or better yet some easy instructions for transcoding ASCII to UTF7 > so that I can write said script myself. function makeutf7(str) { // str must be ascii or latin-1 return '+' + btoa(mytest.replace(/(.)/g,"\0$1")) + '-'; } note: over-converts but easier than a routine to do it right. if you have iconv that's pretty easy and does it right.

> return '+' + btoa(mytest.replace(/(.)/g,"\0$1")) + '-'; Sorry, should be 'str' instead of 'mytest'

Sadly, this patch doesn't seem to pass this test (although it does pass the testcases pointed to from the other bug). Unless the document viewer function I'm using is not the one the charset menu uses? In any case, I can't figure out how to tell apart the case in this test from a user directly overriding the charset on a given window; someone would have to dig carefully through the document viewer and charset info mess to sort it out.

Comment on attachment 293265 [details] [diff] [review] Patch Removing review requests, since this doesn't pass my test.

> ... doesn't seem to pass this test ... A stab in the dark: could it be that the webserver containing the test pages does not set an explicit charset? Would the tests pass if the test pages contained a meta charset declaration?

Another stab in the dark: maybe, your patch does not protect against nested frames (as per comment #6)? I think it is your new automated test only that contains nested frames.

Yet another stab in the dark. Looking in file content/html/document/src/nsHTMLDocument.cpp I see the functions TryHintCharset TryUserForcedCharset TryCacheCharset TryBookmarkCharset TryParentCharset TryDefaultCharset defined (TryParentCharset is what Boris patched) and TryChannelCharset used but not defined. Guessing: is it not TryUserForcedCharset that sets the charset chosen by the user? If so, should not it check that the top-level document and the current frame are same-origin?

The user-forced charset can be forced on a particular frame programmatically, so that would be the wrong check. Also, the default charset stuff might propagate through charsets from previous documents loaded in the same window. Someone needs to sit down and audit all these codepaths. That someone is not me, at least until May.

> The user-forced charset can be forced on a particular frame > programmatically ... How can that be done: could you please point me to an example? Could my PoC be changed so as to not require user interaction?

> How can that be done: could you please point me to an example? Yeah. Take the user interface code that changes the zoom level, and have it do it on some subframe's docshell instead of the root browser one. > Could my PoC be changed so as to not require user interaction? Only if it were running with UniversalXPConnect privileges (at which point it could do whatever it wanted anyway).

> Take the user interface code that changes the zoom level ... But that is with UniversalXPConnect, so not for "mere mortals"? Then, we do not need to worry about programmatic changes in your comment #32, just about default charset propagation. I suggest to change nsHTMLDocument.cpp so as to check whether top-level and current frame are same-origin; if yes then go ahead with the sequence of Try*Charset calls as now, if not then do TryChannelCharset and UseWeakDocTypeDefault only. I do not know how to say "top-level" in that cpp, so cannot give a patch file.

> But that is with UniversalXPConnect, so not for "mere mortals"? Yes. > Then, we do not need to worry about programmatic changes in your > comment #32, just about default charset propagation. But the point is that the user-forced charset could be forced on a particular subframe by chrome code. So checking that the subframe and the root are same-origin won't work: they could be different origins, with the forcing happening explicitly on the subframe. In that case, the forcing should happen.

> ... charset could be forced ... by chrome code. I am confused. Who/what is "chrome code": is not that just Mozilla, without user (thus attacker) access? > In that case, the forcing should happen. Maybe the test I suggested should become: if ( (top-level and current frame are same-origin) || (force comes from chrome) ) { // Proceed with current sequence of Try*Charset calls } else { // do TryChannelCharset and UseWeakDocTypeDefault only } except maybe we cannot tell where force comes from. Anyway, why would "chrome" want to do anything to a frame? If that is not (often) used, then we can dispense with the check.

> Who/what is "chrome code" The application user interface. Including extensions. > except maybe we cannot tell where force comes from. Correct. At the moment we cannot. > Anyway, why would "chrome" want to do anything to a frame? Because an extension wants to add the option to override charset on a single frame and not on the whole frame tree?

(In reply to comment #38) > > Because an extension wants to add the option to override charset on a single > frame and not on the whole frame tree? > this is clearly exploitable and extensions shouldn't do this

How is it any more exploitable than allowing to override charset on the root? frames are in no way special!

>> Anyway, why would "chrome" want to ... > Because an extension wants to add the option to ... Looking at https://addons.mozilla.org/en-US/firefox/search?q=charset shows just three extensions, and I hope none uses such features. If the extension is really determined, then with all-powerful chrome magic it can re-set the domain of the frame (I think). Seems that a "proper" solution requires a major re-design of the charset inheritance code, which will take some time. Could we now please fix the security issue with a one-line patch to the right function; possibly breaking the functionality of some devious extensions? Then, at leisure someone could do the re-design and improve the fancy features. (As things stand, we do not have a Firefox: is insecure, should not be used, users will be flocking back to IE. Whether we can do features in extensions or otherwise, is irrelevant.) >>> ... extension wants to override ... >> this is clearly exploitable ... > How is it ... frames are in no way special! Frames may be invisible: as per my original PoC. - Show us an extension that does overrides, Georgi or I will exploit :-)

Not all extensions are on AMO. In particular, future extensions are not. JS code, even chrome JS code, can't change the security context of a document. > Seems that a "proper" solution requires a major re-design of the charset > inheritance code That's what I keep saying, yes. Sadly, you seem more interested in getting your (minor) pet peeves fixed than in constructively working on an actual solution to the problem. The "proper" solution is needed to actually close up the security holes, but you seem to want to ignore that. > Could we now please fix the security issue with a one-line patch to the right > function We tend to not take security fixes that break things unless it's a super-critical security issue and no fix that won't break things is in sight. This isn't one of those cases, imo, but it's up to the branch drivers. I'll note again that your "one-line" fix: 1) Is not one line 2) Doesn't fix the bug, as far as I can tell due to the default charset inheritance in time (not across frames). There's nothing "devious" about allowing a user to override charsets; unfortunately it's required functionality in many parts of the world. I think I'm done with this bug. It's taking away too much time from much more serious security problems, and all we're doing is repeating the same things over and over again. :( Sadly, you forced me to file this so I can't un-cc myself. But it's getting added to the trash filter.

> I'll note again that your "one-line" fix: > 1) Is not one line Was a figure of speach ... will do exact count later. > 2) Doesn't fix the bug, as far as I can tell due to the default charset > inheritance in time (not across frames). That would be a different bug. And anyway, is there such a bug: could you please provide exploit or testcase, or point us to the bug report? > I think I'm done with this bug. ... Before you go: can you (anyone) please tell us how to say "top-level" in Mozilla code? Pointing us to an example would be enough.

> That would be a different bug. My point is that redesigning this code so it's not broken and letting through security issues like a sieve is probably less time-consuming than engaging in complicated, fragile whack-a-mole of said security issues. > Before you go: can you (anyone) please tell us how to say "top-level" You mean "top level content window"? <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsDocumentViewer.cpp&rev=1.558&mark=1850-1853#1850>. You should be able to get a docshell tree item from the document's container.

Thanks for the pointer. Not clear to me what is the relation between mContainer mentioned there and aContainer used in nsHTMLDocument.cpp; seems "simpler" to find parent, then parent of that etc until no more. Will think about it for a while; all the while hoping that someone with experience will jump in and provide a patch. If you could redesign things, that surely would be best. But you are not around until May...

Just walking the document parent chain you'll walk into the chrome document. You don't want that.

Within nsHTMLDocument.cpp, could we simply use nsContentUtils::CheckSameOrigin(mRootContent, aContainer) ? (I may have got it all wrong...)

Since no-one objected to my mRootContent idea, here is a suggested patch; this includes (re-does) Boris's patch to TryParentCharset. Please test: that it compiles, that it passes the testcases, and that it still allows "normal" charset operations. (I have not tested it in any way as that seems not-too-easy, so am prepared that it may not compile or otherwise may not work.)

I now built Firefox with my patches, got to compile by using some CASTs. However it does not actually detect any "top-level and current frame not same origin" conditions... will need to think more about how to to that.

This patch seems to work fine, making things secure while allowing "normal" charset operations, so am asking for it to be reviewed. See comment #32 (and comment #38): this patch would break "chrome" trying to override charset on a selected cross-site frame. I believe no current extensions do that. See comment #32 (and comment #42): there is a suggestion that "default charset" could be inherited (might propagate) cross-site. I do not think this patch would protect against that; there is no proof that such propagation ever existed. This may be a non-issue (in the security sense): defaults would not be used when the page specifies a charset, and we expect all pages to specify one, lest they fall victim to IE autodetect.

In comment #50 I wrote: > ... suggestion that "default charset" could be inherited ... > ... would not be used when the page specifies a charset ... Thus any such problem would not be this (or any future) bug but would be part of bug 356280, which we know(?!) is solved.

(In reply to comment #51) > Thus any such problem would not be this (or any future) bug but > would be part of bug 356280, which we know(?!) is solved. > i haven't looked at the patch, but bug 356280 caused trouble, so your patch shouldn't regress it (some of the concerns in comment #50) :)

I notice that my latest patch https://bugzilla.mozilla.org/attachment.cgi?id=298374 is "protected" from mere mortals (and I wonder if any login to bugzilla will do, or need to be special to this bug, to see it). Is that "normal", or a mistake? Seems useless: my older (obsolete) patch is still public, and so is the testcase/PoC.

Looks like bz did that. bz: was that intentional?

> bz: was that intentional? No.

> Simon Montagu <smontagu@smontagu.org> changed: > AssignedTo psz@maths.usyd.edu.au That's cool, am honoured. Does this mean I need to do something? Anything I can do to make the superreview happen? May I set blocking1.8.1.12?

You can request blocking1.8.1.12, i.e. set it to "?"

1.8.1.12 is already on release candidate 4, this won't be able to make it in. dveditz has already requested blocking1.8.1.13, so this won't be forgotten when 1.8.1.3 release planning work begins.

jst: can you please review the patch?

For what it's worth, the patch is wrong, for the reasons described above. I'm against taking it as-is: it breaks things without really fixing the security issues.

And if we think this should be blocking branch, then we need to get someone on this to fix it the right way.

> ... breaks things without really fixing the security issues. Please point us to anything it breaks, and give an example (testcase, PoC) where a security issue remains. (You did say "for the reasons described above", but I seem unable to find anything specific.)

Comment on attachment 298374 [details] [diff] [review] patch Requested approval1.8.1.13? as per email from Samuel Sidler <ss@mozilla.com>.

Comment on attachment 298374 [details] [diff] [review] patch Needs review first.

dveditz says this blocks 1.8.1.13, but bz says the patch is wrong, so we're really not sure what to do for trunk. We're not going to block on this, but if it's accepted into 1.8.1.13, then someone should renominate.

> ... bz says the patch is wrong ... BZ has misgivings about default charset inheritance: that would be part of bug 356280, which he skillfully solved already; and about the UI not providing sub-menus for charset override on each subframe separately: that is long-standing feature request bug 63054.

This should be at least wanted1.9.0.x. Ideally we would actually fix this before we ship, but we should definitely fix it ASAP. Comment 66 mischaracterizes my concerns about the patch actually posted in teh bug, but since Paul seems to have not understood them all along I don't think it's worth my time to retype them yet again. Please just read what I actually wrote instead.

Boris, Sorry if I am unable to correctly represent your ego. Great pity it clashes with actual knowledge. - Paul

Of course the patch is "wrong": because if Mozilla maintainers cared about clean code and security, they would have provided functions am_I_same_origin_as_parent() am_I_same_origin_as_toplevel() am_I_same_origin_as_tree_all_the_way_up() and then the patch would truly be one-line, and there would not be this repetitious spaguetti code all over the place, and people would maybe know what things are meant to do. But, it is always the "easy fix" which, as bug 356280 aptly demonstrates, does not do what is expected... So yes, someone should do this the "right way". Sorry, but borrowing some self-aggrandizing words, that is not me for now.

My rude outburst above was wrong, and I apologize unreservedly. Will take care to avoid future recurrence. - Paul

Paul - sorry to see you frustrated, and please know that your efforts are appreciated. In the meantime, as per comment 67, marking blocking1.9.0.x

Coming up against the 1.8.1.13 code-freeze. Since this hasn't gotten reviewed and landed on trunk we're going to have to wait on the branch landing too.

> ... hasn't gotten reviewed ... Why is that? Could that be corrected (done)?

Moving this to the next release. This should also block 1.9.0.1.

Comment on attachment 298374 [details] [diff] [review] patch I already removed myself from a review on this patch once... it's basically the same as the previous thing I looked at in this bug, with the same issues. What this bug needs is a patch _without_ those problems...

Then why not r-, if you think the patch isn't right?

> ... why not r- ... Does that mean "hide by making unreadable" or remove? Either way would be wrong as not his patch or bug. For now is the only patch available...

It means "mark as unsuitable for commit", which I think is bz's assessment.

All I think is that I need a lot of convincing that the patch is right (which is not the same as thinking it's wrong; just that this is complicated code and no one seems to have done the thinking through it to make sure the patch really is correct). That convincing will come either from the patch author, in the form of a clear analysis of the code and how this patch fits in, or from me digging through the code and generating said analysis. The patch author seems unwilling to do the legwork, and I'm not in a position to do it myself until a few weeks from now. It's certainly on my to-do list for then if no one else has bothered, but so are about 150 other items (literally; it might be more now, but it was 150-ish two weeks ago). In the meantime, there are other patches that are a higher review priority for me (because there will be a much higher payoff-to-review-time ratio and because they have clear explanations of what's going on with the code they touch or because I know the code they touch already), so I took this out of my review queue for now so it won't get in my way. r- would imply the patch is definitely wrong, and I'm not claiming that. It's just that the effort of reviewing it as things stand is the same as the effort involved in fixing this bug from scratch, as far as I can tell.

> The patch author seems unwilling to do the legwork ... Thanks for reminding me that I am a lazy slob. However: > That convincing will come ... from ... a clear analysis of the code ... Please see comment #35 and comment #37 and (gasp!) comments in patch code.

I wouldn't call you a lazy slob by any means. The analysis needed here is a huge pain in the behind. The comments you point to analyze _one_ of a number of possible codepaths and possibilities here... They address the one specific testcase, but may well not address simple variations. I'd much rather reduce complexity and fix more bugs here (what I think should happen) than increase complexity and possibly fix fewer bugs.

> ... number of possible codepaths ... You have hinted at "charset inheritance in time" (comment #42). How could that possibly happen? I could only find one place, in nsHTMLDocument.cpp, where charset choice is done.

That one place looks at a whole bunch of charset information sources, including as one example the previous document loaded in the docshell, which means looking at ancestors of that previous document, as well as the documents loaded before those (indirectly, since all those things might have been used in determining the charset of the previous document).

> That one place looks at a whole bunch of charset information sources ... ... which may be "compromised", bogus, under attacker's control. That is exactly why, as per comment #35, when cross-frame the patch ignores all such sources and looks in TryChannelCharset and UseWeakDocTypeDefault only (which seemed safe enough to me: please correct me if wrong). Does this allay your fears?

No, because it doesn't address the non-subframe case.

> ... it doesn't address the non-subframe case. That is outside the scope of this bug: this one is titled "... into subframes across domain boundaries" thus it deals with frames and boundaries. What you seem to be talking about is a visit to a website making some persistent changes to the browser state so as to affect future visits to other (top-level) websites. I would hope that the browser is safe against such nastiness... but I do not know whether it is, I do not even know what persistent state we should scrutinize besides charsets. Surely that is important; but is not this bug. An attack against that other bug would not be a simple variation of the testcase of this bug. And there is silly me agonizing whether it is enough to compare current frame to toplevel (as the patch now does), or should check every one on parent chain up to toplevel. (That might affect search engines or webmail services if they chose to show an "evil subframe" which could then steal the cookies of that same service; but is easily fixed by the provider showing links or sanitized data, not frames; and would not be "universal" but an attack against the one service only.)

An attack against the other bug could be a pretty trivial variation of this testcase: get the user to select UTF-7, then set document.location. As for scope, as I said above complicating this code to fix this one attack vector makes it harder to fix the others; we generally aim to fix all related attack vectors when we're fixing one of them...

> ... get the user to select UTF-7, then set document.location. Could you please provide a testcase/PoC: that would seem a pretty serious bug. Being so simple, it should have been discovered, used and published long ago. Please see comment #50, which argues that no such bug is in fact present or possible in normal circumstances. And anyway that would be a new bug, not this one; and that attack is not a "trivial variation" of this testcase. > ... code to fix this ... makes it harder to fix the others ... Any factual basis for that statement? > ... we generally aim to fix all related attack vectors ... As bug 356280 aptly demonstrates. --- Only hope against such circular procrastination seems that bug 441876 will succeed, remove UTF-7 from everywhere and extinguish this bug.

> Any factual basis for that statement? Yes. You're adding yet another codepath that would need to be understood by whoever undertakes to really fix this whole rats' nest. > As bug 356280 aptly demonstrates. Precisely. The fact that fixing that bug didn't fix this one is what made it clear that this code needs an actual fix instead of whack-a-mole. > Only hope against such circular procrastination Hey, I was pretty up-front all along about all this (mozilla stuff in general) being an extremely low priority for me for most of the last year. That's changing starting in a week and a half. For this next week and a half it'll be an even lower priority. Fact is, I should be unpacking boxes and putting my baby to bed instead of trying to explain, yet again, why what I think is needed to really fix this code (as opposed to this bug, note) is what is needed to fix this code. Seems to be a complete waste of my time, since no one is going to pick it up and do it anyway.

> You're adding yet another codepath that would need to be understood by > whoever undertakes to really fix this whole rats' nest. Some hair-splitting: my patch does not add "code paths" but takes away; if/when this is "fixed", all this will be thrown out, not understood. > The fact that fixing that bug didn't fix this one is what made it > clear that this code needs an actual fix instead of whack-a-mole. We agree that bug 356280 was botched; my patch is the proper fix. Your splitting this bug off into bug 406777 and 408457 was wrong. The essence of this latest round: if you think a vulnerability remains after my patch is applied, then please demonstrate: "put up or shut up". Seeing that a "proper fix" is years away, would not have been better to whack this mole about 6 months ago? --- Wish you the best with your baby. Thankfully mine are now grown up, 19- and 22-year old.

Amazing that bug 441876 succeeded, though it is "wrong"... while this one is in limbo. Anyway as I understand things, with 441876 in place no-one will ever be able to select UTF-7 and we are safe. No need to keep working on this bug... until someone comes up with another encoding attack.

Not blocking, but wanted.

Comment on attachment 298374 [details] [diff] [review] patch Clearing out old reviews. If this is still relevant, please re-request review for this patch.

Yes still relevant: is an example where mozilla design is wrong, allowing cross-domain interaction. The solution in bug 441876 (removing UTF7 from various menus) was "wrong", meant loss of (infrequently used) functionality, and only protects against UTF7 issues. (Other encodings are not known dangerous; future research may show them to be, or future encodings may be.) The patch I submitted may no longer apply; and unfortunately now I do not have time to devote to further development.

I believe Henri has recently changed how this stuff works, actually. Henri, _is_ this bug still relevant?

(In reply to Boris Zbarsky (:bz) from comment #96) > I believe Henri has recently changed how this stuff works, actually. Henri, > _is_ this bug still relevant? This is still relevant in the sense that the UI override still propagates across origins and in the cases where this election is not ASCII-compatible. However, the impact has been reduced, because UTF-7 is no longer supported. The only ASCII-incompatible encodings that we still support are the UTF-16 family. My preferred fix would be removing the UTF-16 family from the menu and disabling the menu if the encoding of the page (or any of its subframes) is one of the encodings of the UTF-16 family. However, I don't know if that's feasible, but I'd like to try. See bug 802033 commet 3. (Of course, it would be totally awesome if we could do to UTF-16 what we did to UTF-7 and UTF-32, but that’s less likely to happen.) I think we should also disable the menu in many cases: bug 234628 comment 14. (In reply to Paul Szabo from comment #95) > Yes still relevant: is an example where mozilla design is wrong, > allowing cross-domain interaction. So if the UTF-16 case was taken care of, the remaining threat model would be that ASCII-compatible encodings are vulnerable to *yet-unknown* attacks and a malicious page frames the victim and shows mojibake to bait the user into choosing another encoding from the menu so that the chosen encoding propagates to the victim page. > The solution in bug 441876 > (removing UTF7 from various menus) was "wrong", meant loss of > (infrequently used) functionality, and only protects against > UTF7 issues. Removing UTF-7 support was exactly the right call. It was a feature with no use cases other that XSS exploits. > or future encodings may be.) There won't be future encodings. UTF-8 covers all of Unicode and the architecture of Gecko (or the Web or pretty much any modern software) doesn't support stepping outside of Unicode. Therefore, UTF-8 already addresses all realistic future use cases and new encoding cannot be more expressive than UTF-8. The other encodings are supported only in order to support legacy content. If an encoding is not needed for legacy content, it should be removed (see bug 711101). Attempts to introduce new encodings should be r-’ed with extreme prejudice, because they cannot be more expressive than UTF-8 and come at a great maintenance cost. (Witness the vast amount of engineering, testing and speccing time wasted on UTF-7, UTF-16 and UTF-32 alone.) (The encoding that has the greatest risk of getting a patch proposed is BOCU-1. Fortunately, the HTML spec already says “User agents must not support the CESU-8, UTF-7, BOCU-1 and SCSU encodings.”, so it should be easy to r-.) If you ever see a proposal to introduce a new encoding into Gecko, please CC me on the bug.

> This is still relevant in the sense that the UI override still > propagates across origins ... Yes, still relevant. > However, the impact has been reduced ... Impact levels are irrelevant. Security requires no cross-domain interaction. (Yes, security is black-and-white: either you can prove it is secure, or you cannot.) > My preferred fix would be removing ... That is a "user experience" issue, and I have no opinion. (Obviously if you remove UI charset overrides altogether then that will cease to exist and will not cross domain boundaries, but I do not think you propose to be that drastic.) > So if the UTF-16 case was taken care of, the remaining threat > model would be that ASCII-compatible encodings are vulnerable > to *yet-unknown* attacks ... You seem to imply that UTF-16 is "known vulnerable". References? > Removing UTF-7 support was exactly the right call. It was a > feature with no use cases other that XSS exploits. Was the right call from a UI perspective, was wrong as response to a security issue. > There won't be future encodings. Some prophecies turn out to be wrong.

(In reply to Paul Szabo from comment #98) > > So if the UTF-16 case was taken care of, the remaining threat > > model would be that ASCII-compatible encodings are vulnerable > > to *yet-unknown* attacks ... > > You seem to imply that UTF-16 is "known vulnerable". References? https://bugzilla.mozilla.org/show_bug.cgi?id=406777#c1 Demo: http://hsivonen.iki.fi/test/moz/never-show-user-supplied-content-as-utf-16.htm Fortunately, only sites that serve user-supplied content as UTF-16 are affected, hence the impact is very limited—perhaps even just theoretical. (Though I vaguely recall that Google Search was vulnerable to this at some point in the past.) The reverse attack (injecting UTF-16 into a non-UTF-16 page as user-supplied content) requires the site that accepts user-supplied content to be inept enough to allow user-supplied content to contain U+0000, so the attack is less likely to succeed than UTF-7 attacks. But as I said in my previous comment, I think we should 1) Remove UTF-16 family from the menu (to defend against the second attack) 2) Disable the menu if the page or any of its subpages is encoded as UTF-16 (to defend against the first attack) > > There won't be future encodings. > > Some prophecies turn out to be wrong. We control the code and, therefore, can opt not to introduce new encodings.

You seem to accept that sites that use UTF-16, or sites that pass NULLs in user-supplied content, are at risk. While maybe currently no sites are vulnerable, you knew about Google Search, and failed to protect. To a large extent, the vulnerability is not within the site but through Mozilla allowing things cross-domain. Only by getting Mozilla design correct, with a flag for cross-domain and preventing access, can you stay secure in future. > We ... can opt not to introduce new encodings. Was UTF-7 a Mozilla invention? You may need encodings if/when they become popular.

(In reply to Paul Szabo from comment #100) > You seem to accept that sites that use UTF-16, or sites that pass NULLs > in user-supplied content, are at risk. No, I said we should remove UTF-16 from the menu (the NULL case) and disable the menu when there’s an UTF-16-encoded document loaded (including subframes). I’d love to get rid of the menu altogether if feasible. > While maybe currently no sites > are vulnerable, you knew about Google Search, and failed to protect. I didn’t know about until after Google fixed it. > Was UTF-7 a Mozilla invention? You may need encodings if/when they > become popular. Encodings are unlikely to become popular on the Web if they are not supported by browsers.

> I'd love to get rid of the menu altogether if feasible. You propose to solve cross-domain propagation of charset overrides by doing away with those pesky overrides. Cool. Taken to its logical conclusion, should we do away with Mozilla altogether and avoid all security issues? > Encodings are unlikely to become popular on the Web if they > are not supported by browsers. Mozilla will always have to support any Redmond concoctions. --- We seem to be going around in circles. The point is: cross-domain interaction is forbidden, but Mozilla does not enforce it. Mozilla is broken and needs to be fixed. The problem stems from the fact that Mozilla fails to provide (internal) calls like am_I_same_origin_as_parent() am_I_same_origin_as_toplevel() am_I_same_origin_as_tree_all_the_way_up() (see comment 69 above). With those it would be so much easier to write secure code, and then maybe it would be written so, not just for charsets but any other cross-domain issues.

Is this still an issue? It seems UTF-7 was removed in bug 414064.

The issue is a cross-domain interaction within FF. Currently there are no known security PoC demos, none are mentioned within this bug; but we cannot be certain that there are no active exploits "out there" using this FF bug. (UTF7 was used in one specific, theatrical, PoC demo, but is not the essence of the bug.)

(In reply to Mats Palmgren (:mats) from comment #103) > Is this still an issue? It seems UTF-7 was removed in bug 414064. Bug 871161 fixed cross-origin inheritance, but this bug is about the override (menu selection) propagating cross-origin. UTF-7, ISO-2022-CN, ISO-2022-KR and HZ-GB-2312 are gone. So ISO-2022-JP and UTF-16 (times two for endianness) are the only dangerous encodings left and the UTF-16 has been dealt with. UTF-16 documents ignore the override. This leaves ISO-2022-JP. We currently intentionally misclassify ISO-2022-JP by not listing it at http://mxr.mozilla.org/mozilla-central/source/dom/encoding/EncodingUtils.cpp#57 . If it was listed there, documents declared as ISO-2022-JP would reject the override thanks to work done previously (before HZ-GB-2312 was desupported outright) to defend against HZ-GB-2312 attacks. If it was removed from the menu, there'd be no way to request other documents to be overridden to ISO-2022-JP. I'd by very sympathetic to teaching EncodingUtils that ISO-2022-JP is ASCII-incompatible and to removing it from the menu. If we did that, this bug would be moot. ISO-2022-JP is currently intentionally incorrectly classified (by failing to classify it as ASCII-incompatible), because 1) no one has shown a PoC for an ISO-2022-JP attack and 2) when Microsoft issued a security patch against ISO-2022-JP (in their detectors, I gather), they had to revert the security fix over compatibility concerns. I'd prefer WONTFIXing this bug (due to its collateral damage to legacy that's not actually XSS-dangerous) and making ISO-2022-JP special over making the override not work for different-origin iframes in general. However, instead of jumping right to it, at this point, the best you could do is to come up with a realistic PoC for an ISO-2022-JP attack. It's always easier to justify compatibility risk if the security risk has been shown to be practical.

Thanks Henri, based on that it seems to me the security risk here is far lower than with UTF-7, so changing the sec rating to sec-low for now.