Bugzilla@Mozilla – Bug 453915
XML injection possible in E4X parsing via "default xml namespace"
Last modified: 2009-01-14 12:38:07 PST
Summon comment box
User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.16) Gecko/20080716 Firefox/2.0.0.16 Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.16) Gecko/20080716 Firefox/2.0.0.16 This Javascript fragment illustrates the problem: default xml namespace = '\''; <foo/> You will get an error: Error: unterminated string literal Source File: http://blah/ Line: 162, Column: 16 Source Code: <parent xmlns='''><blah... The concern is that attacks may be possible via injecting XML into cross-domain XML via the <script src=> facility. Reproducible: Always Steps to Reproduce: 1. 2. 3.
BTW, also affects FF3.0.1. Fixing should be a matter of applying proper escaping to the value of the namespace string, when bolting it into the generated XML string.
Happens on trunk too. This could be a bug in the algorithm in the E4X spec, but that shouldn't stop us from fixing it in SpiderMonkey.
Created attachment 341458 [details] [review] use js_QuoteString I think I need a TVR here, and will respin if you agree. Here is the error with the patch: js> default xml namespace = '\''; js> <foo/> typein:2: SyntaxError: unterminated string literal: typein:2: <parent xmlns='\''><foo/></parent> typein:2: .................^ I think this fixes the problem, since the xmlns string is now escaped. If not, please enlighten me. :)
Using js_QuoteString is wrong here as the string will be parsed as XML attribute value. So we should use the rules for escaping XML attributes like turning '&', '<', '\'' and '"' int &something;
Comment on attachment 341458 [details] [review] use js_QuoteString Nevermind, this is wrong, as mrbkap pointed out on IRC.
(In reply to comment #4) > Using js_QuoteString is wrong here as the string will be parsed as XML > attribute value. So we should use the rules for escaping XML attributes like > turning '&', '<', '\'' and '"' int &something; Yeah, I wrote my comment #5 this morning and then didn't submit, sorry about that. I'll take another stab in a bit.
Created attachment 341494 [details] [review] Use js_EscapeAttributeValue
With the patch, the testcase produces: js> default xml namespace = '\''; js> <foo/>
Comment on attachment 341494 [details] [review] Use js_EscapeAttributeValue >--- mozilla.8a6d94337518/js/src/jsxml.cpp 2008-10-02 13:28:11.000000000 -0700 ... >+const char js_apos_entity_str[] = "'"; ... > static const char prefix[] = "<parent xmlns='"; > static const char middle[] = "'>"; For patch minimality I would suggest not to alter the EscapeAttributeValue and rather change prefix and middle to use ", not ', around the esacped uri value.
Created attachment 341528 [details] [review] smaller Still worried about gc hazards here, can you double-check that?
We'll take this when the trunk picks it up.
Created attachment 342279 [details] [review] backport to 1.8
Created attachment 342280 [details] [review] backport to 1.9
Created attachment 342283 [details] [review] correct version of 1.8 backport
(In reply to comment #14) > Created an attachment (id=342283) [details] > correct version of 1.8 backport Can you attach a plain diff between 1.9 and 1.8 patches?
Created attachment 342292 [details] [review] diff between 1.8 and 1.9 backport
(In reply to comment #16) > Created an attachment (id=342292) [details] > diff between 1.8 and 1.9 backport Thanks! That made it clear what prevented the patch to apply as-is.
Is this going to land on trunk? we'd like some verification of the fix before approving for the branches.
changeset: 20487:dcf70800258a
Bob or Jesse, can you verify this bug on the trunk? We'll take it on the branches after verification. It'd also be nice to have an easy-to-use manual testcase for QA to verify with on the branches. (And an automated one! ;) )
js> default xml namespace = '---"---&---\'---'; js> uneval(<foo/>) <foo xmlns="---"---&---'---"/> Looks good to me.
Comment on attachment 342280 [details] [review] backport to 1.9 Approved for 1.9.0.4, a=dveditz for release-drivers
Comment on attachment 342283 [details] [review] correct version of 1.8 backport Approved for 1.8.1.18, a=dveditz for release-drivers
Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.73; previous revision: 3.50.2.72 done
Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.215; previous revision: 3.214 done
(In reply to comment #20) > Bob or Jesse, can you verify this bug on the trunk? We'll take it on the > branches after verification. > > It'd also be nice to have an easy-to-use manual testcase for QA to verify with > on the branches. (And an automated one! ;) ) Well, since there is no easy case, I'd ask if Bob or Jesse could verify this for 1.9.0.4 and 1.8.1.18 as well with a nightly. Can either of you do this or tell me of a straightforward way for me to do it. :-)
The testcase from comment #21 is pretty easy.
Where do I get the shell or whatnot that he's using there? It might be easy but I don't have the tool or extension in question.
http://www.squarefree.com/shell/ <--- should work fine.
I'm not clear on why we feel a manual testcase for this is necessarily, since it is so easy to automate, though.
Personally, I don't care about a manual test case (or the automated one, though I agree it would be a good idea). I'm going through all of the fixed bugs for the 1.8.18 and 1.9.0.4 release to verify that they are actually fixed. Since there are quite a few and I'm pretty much the person doing this, I'm just trying to get through them before we start testing official builds. Thanks for the link.
Verified using comment 21 testcase for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102404 GranParadiso/3.0.4pre.
Verified for Trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081024 Minefield/3.1b2pre.
Verified for 1.8.1.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008102804 BonEcho/2.0.0.18pre.
Comment on attachment 342283 [details] [review] correct version of 1.8 backport a=asac for 1.8.0
Created attachment 347305 [details] [review] For check-in, 1.8.0 branch based on attachment 342283 [details] [review]; just some context adjustments to apply cleanly.
Checking in e4x/Regress/regress-453915.js; /cvsroot/mozilla/js/tests/e4x/Regress/regress-453915.js,v <-- regress-453915.js initial revision: 1.1 done
http://hg.mozilla.org/mozilla-central/rev/395be2ab5c6b