Last Comment Bug 453915 - (CVE-2008-5024) XML injection possible in E4X parsing via "default xml namespace"
(CVE-2008-5024)
: XML injection possible in E4X parsing via "default xml namespace"
Status: VERIFIED FIXED
[sg:low?]
: testcase, verified1.8.1.18, verified1.9.0.4, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-05 17:55 PDT by Chris Evans
Modified: 2009-01-14 12:38 PST (History)
10 users (show)
sayrer: blocking1.9.1+
dveditz: blocking1.9.0.4+
samuel.sidler+old: wanted1.9.0.x+
dveditz: blocking1.8.1.18+
samuel.sidler+old: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use js_QuoteString (1.15 KB, patch)
2008-10-02 09:26 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
Use js_EscapeAttributeValue (4.28 KB, patch)
2008-10-02 13:28 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
smaller (1.17 KB, patch)
2008-10-02 15:33 PDT, Brian Crowder
igor: review+
Details | Diff | Splinter Review
backport to 1.8 (2.10 KB, patch)
2008-10-08 11:42 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
backport to 1.9 (2.10 KB, patch)
2008-10-08 11:46 PDT, Brian Crowder
igor: review+
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review
correct version of 1.8 backport (2.11 KB, patch)
2008-10-08 11:48 PDT, Brian Crowder
igor: review+
dveditz: approval1.8.1.18+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
diff between 1.8 and 1.9 backport (612 bytes, patch)
2008-10-08 12:32 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
For check-in, 1.8.0 branch (2.04 KB, patch)
2008-11-10 09:28 PST, Alexander Sack
no flags Details | Diff | Splinter Review

Description Chris Evans 2008-09-05 17:55:59 PDT
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.
Comment 1 Chris Evans 2008-09-05 17:59:19 PDT
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.
Comment 2 Jesse Ruderman 2008-09-05 18:22:30 PDT
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.
Comment 3 Brian Crowder 2008-10-02 09:26:30 PDT
Created attachment 341458 [details] [diff] [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.  :)
Comment 4 Igor Bukanov 2008-10-02 10:32:46 PDT
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 5 Brian Crowder 2008-10-02 13:06:39 PDT
Comment on attachment 341458 [details] [diff] [review]
use js_QuoteString

Nevermind, this is wrong, as mrbkap pointed out on IRC.
Comment 6 Brian Crowder 2008-10-02 13:07:39 PDT
(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.
Comment 7 Brian Crowder 2008-10-02 13:28:53 PDT
Created attachment 341494 [details] [diff] [review]
Use js_EscapeAttributeValue
Comment 8 Brian Crowder 2008-10-02 13:30:21 PDT
With the patch, the testcase produces:
js> default xml namespace = '\'';
js> <foo/>
Comment 9 Igor Bukanov 2008-10-02 14:23:43 PDT
Comment on attachment 341494 [details] [diff] [review]
Use js_EscapeAttributeValue

>--- mozilla.8a6d94337518/js/src/jsxml.cpp	2008-10-02 13:28:11.000000000 -0700
...
>+const char js_apos_entity_str[]   = "&#39;";
...
>     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.
Comment 10 Brian Crowder 2008-10-02 15:33:02 PDT
Created attachment 341528 [details] [diff] [review]
smaller

Still worried about gc hazards here, can you double-check that?
Comment 11 Samuel Sidler (old account; do not CC) 2008-10-03 11:14:52 PDT
We'll take this when the trunk picks it up.
Comment 12 Brian Crowder 2008-10-08 11:42:04 PDT
Created attachment 342279 [details] [diff] [review]
backport to 1.8
Comment 13 Brian Crowder 2008-10-08 11:46:17 PDT
Created attachment 342280 [details] [diff] [review]
backport to 1.9
Comment 14 Brian Crowder 2008-10-08 11:48:18 PDT
Created attachment 342283 [details] [diff] [review]
correct version of 1.8 backport
Comment 15 Igor Bukanov 2008-10-08 12:21:19 PDT
(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?
Comment 16 Brian Crowder 2008-10-08 12:32:17 PDT
Created attachment 342292 [details] [diff] [review]
diff between 1.8 and 1.9 backport
Comment 17 Igor Bukanov 2008-10-08 12:57:32 PDT
(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.
Comment 18 Daniel Veditz [:dveditz] 2008-10-09 10:04:56 PDT
Is this going to land on trunk? we'd like some verification of the fix before approving for the branches.
Comment 19 Brian Crowder 2008-10-14 14:00:52 PDT
changeset:   20487:dcf70800258a
Comment 20 Samuel Sidler (old account; do not CC) 2008-10-16 16:40:13 PDT
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! ;) )
Comment 21 Jesse Ruderman 2008-10-16 16:44:32 PDT
js> default xml namespace = '---"---&---\'---';
js> uneval(<foo/>)                              
<foo xmlns="---&quot;---&amp;---'---"/>

Looks good to me.
Comment 22 Daniel Veditz [:dveditz] 2008-10-20 11:23:38 PDT
Comment on attachment 342280 [details] [diff] [review]
backport to 1.9

Approved for 1.9.0.4, a=dveditz for release-drivers
Comment 23 Daniel Veditz [:dveditz] 2008-10-20 11:23:51 PDT
Comment on attachment 342283 [details] [diff] [review]
correct version of 1.8 backport

Approved for 1.8.1.18, a=dveditz for release-drivers
Comment 24 Brian Crowder 2008-10-20 14:32:27 PDT
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
Comment 25 Brian Crowder 2008-10-20 14:33:18 PDT
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.215; previous revision: 3.214
done
Comment 26 Al Billings [:abillings] 2008-10-24 13:06:14 PDT
(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. :-)
Comment 27 Brian Crowder 2008-10-24 13:15:35 PDT
The testcase from comment #21 is pretty easy.
Comment 28 Al Billings [:abillings] 2008-10-24 14:06:58 PDT
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.
Comment 29 Brian Crowder 2008-10-24 14:24:43 PDT
http://www.squarefree.com/shell/  <--- should work fine.
Comment 30 Brian Crowder 2008-10-24 14:25:11 PDT
I'm not clear on why we feel a manual testcase for this is necessarily, since it is so easy to automate, though.
Comment 31 Al Billings [:abillings] 2008-10-24 14:47:15 PDT
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.
Comment 32 Al Billings [:abillings] 2008-10-24 14:57:38 PDT
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.
Comment 33 Al Billings [:abillings] 2008-10-24 14:58:47 PDT
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.
Comment 34 Al Billings [:abillings] 2008-10-28 11:31:27 PDT
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 35 Alexander Sack 2008-11-10 09:26:33 PST
Comment on attachment 342283 [details] [diff] [review]
correct version of 1.8 backport

a=asac for 1.8.0
Comment 36 Alexander Sack 2008-11-10 09:28:02 PST
Created attachment 347305 [details] [diff] [review]
For check-in, 1.8.0 branch

based on attachment 342283 [details] [diff] [review]; just some context adjustments to apply cleanly.
Comment 37 Bob Clary [:bc:] 2008-11-27 03:40:28 PST
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
Comment 38 Shawn Wilsher :sdwilsh 2008-11-27 11:12:17 PST
http://hg.mozilla.org/mozilla-central/rev/395be2ab5c6b

Note You need to log in before you can comment on or make changes to this bug.