Closed Bug 453915 (CVE-2008-5024) Opened 12 years ago Closed 12 years ago

XML injection possible in E4X parsing via "default xml namespace"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: scarybeasts, Assigned: crowderbt)

Details

(4 keywords, Whiteboard: [sg:low?])

Attachments

(5 files, 3 obsolete files)

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.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Security → JavaScript Engine
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
QA Contact: firefox → general
Hardware: PC → All
Version: unspecified → Trunk
Keywords: testcase
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Flags: blocking1.8.1.18?
Assignee: general → crowder
Attached patch use js_QuoteString (obsolete) — Splinter Review
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.  :)
Attachment #341458 - Flags: review?(igor)
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] [diff] [review]
use js_QuoteString

Nevermind, this is wrong, as mrbkap pointed out on IRC.
Attachment #341458 - Attachment is obsolete: true
Attachment #341458 - Flags: review?(igor)
(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.
Attached patch Use js_EscapeAttributeValue (obsolete) — Splinter Review
Attachment #341494 - Flags: review?(igor)
With the patch, the testcase produces:
js> default xml namespace = '\'';
js> <foo/>
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.
Attached patch smallerSplinter Review
Still worried about gc hazards here, can you double-check that?
Attachment #341494 - Attachment is obsolete: true
Attachment #341528 - Flags: review?(igor)
Attachment #341494 - Flags: review?(igor)
Attachment #341528 - Flags: review?(igor) → review+
We'll take this when the trunk picks it up.
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.4?
Flags: blocking1.8.1.18?
Attached patch backport to 1.8 (obsolete) — Splinter Review
Attachment #342279 - Flags: review?(igor)
Attached patch backport to 1.9Splinter Review
Attachment #342280 - Flags: review?(igor)
Attachment #342279 - Attachment is obsolete: true
Attachment #342283 - Flags: review?(igor)
Attachment #342279 - Flags: review?(igor)
Attachment #342280 - Flags: review?(igor) → review+
(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?
Attachment #342283 - Flags: review?(igor) → review+
(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.
Attachment #342280 - Flags: approval1.9.0.4?
Attachment #342283 - Flags: approval1.8.1.18?
Is this going to land on trunk? we'd like some verification of the fix before approving for the branches.
Whiteboard: [sg:low?]
Keywords: checkin-needed
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.18+
changeset:   20487:dcf70800258a
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
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="---&quot;---&amp;---'---"/>

Looks good to me.
Comment on attachment 342280 [details] [diff] [review]
backport to 1.9

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #342280 - Flags: approval1.9.0.4? → approval1.9.0.4+
Comment on attachment 342283 [details] [diff] [review]
correct version of 1.8 backport

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #342283 - Flags: approval1.8.1.18? → approval1.8.1.18+
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
Keywords: fixed1.8.1.18
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.215; previous revision: 3.214
done
Keywords: fixed1.9.0.4
(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.
Status: RESOLVED → VERIFIED
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] [diff] [review]
correct version of 1.8 backport

a=asac for 1.8.0
Attachment #342283 - Flags: approval1.8.0.15+
based on attachment 342283 [details] [diff] [review]; just some context adjustments to apply cleanly.
Alias: CVE-2008-5024
Group: core-security
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
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.