As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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
:
: Jason Orendorff [:jorendorff]
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Brian Crowder 2008-10-02 13:28:53 PDT
Created attachment 341494 [details] [diff] [review]
Use js_EscapeAttributeValue
Comment 8 User image Brian Crowder 2008-10-02 13:30:21 PDT
With the patch, the testcase produces:
js> default xml namespace = '\'';
js> <foo/>
Comment 9 User image 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 User image 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 User image 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 User image Brian Crowder 2008-10-08 11:42:04 PDT
Created attachment 342279 [details] [diff] [review]
backport to 1.8
Comment 13 User image Brian Crowder 2008-10-08 11:46:17 PDT
Created attachment 342280 [details] [diff] [review]
backport to 1.9
Comment 14 User image Brian Crowder 2008-10-08 11:48:18 PDT
Created attachment 342283 [details] [diff] [review]
correct version of 1.8 backport
Comment 15 User image 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 User image 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 User image 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 User image 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 User image Brian Crowder 2008-10-14 14:00:52 PDT
changeset:   20487:dcf70800258a
Comment 20 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Brian Crowder 2008-10-24 13:15:35 PDT
The testcase from comment #21 is pretty easy.
Comment 28 User image 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 User image Brian Crowder 2008-10-24 14:24:43 PDT
http://www.squarefree.com/shell/  <--- should work fine.
Comment 30 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.