Bug 453915 (CVE-2008-5024)

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Chris Evans, Assigned: Brian Crowder)

Tracking

(4 keywords)

Trunk
testcase, verified1.8.1.18, verified1.9.0.4, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.4 +
wanted1.9.0.x +
blocking1.8.1.18 +
wanted1.8.1.x +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low?])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
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

9 years ago
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

Updated

9 years ago
Keywords: testcase
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Flags: blocking1.8.1.18?
Assignee: general → crowder
(Assignee)

Comment 3

9 years ago
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.  :)
Attachment #341458 - Flags: review?(igor)

Comment 4

9 years ago
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;
(Assignee)

Comment 5

9 years ago
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)
(Assignee)

Comment 6

9 years ago
(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.
(Assignee)

Comment 7

9 years ago
Created attachment 341494 [details] [diff] [review]
Use js_EscapeAttributeValue
Attachment #341494 - Flags: review?(igor)
(Assignee)

Comment 8

9 years ago
With the patch, the testcase produces:
js> default xml namespace = '\'';
js> <foo/>

Comment 9

9 years ago
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.
(Assignee)

Comment 10

9 years ago
Created attachment 341528 [details] [diff] [review]
smaller

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)

Updated

9 years ago
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?
(Assignee)

Comment 12

9 years ago
Created attachment 342279 [details] [diff] [review]
backport to 1.8
Attachment #342279 - Flags: review?(igor)
(Assignee)

Comment 13

9 years ago
Created attachment 342280 [details] [diff] [review]
backport to 1.9
Attachment #342280 - Flags: review?(igor)
(Assignee)

Comment 14

9 years ago
Created attachment 342283 [details] [diff] [review]
correct version of 1.8 backport
Attachment #342279 - Attachment is obsolete: true
Attachment #342283 - Flags: review?(igor)
Attachment #342279 - Flags: review?(igor)

Updated

9 years ago
Attachment #342280 - Flags: review?(igor) → review+

Comment 15

9 years ago
(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?
(Assignee)

Comment 16

9 years ago
Created attachment 342292 [details] [diff] [review]
diff between 1.8 and 1.9 backport

Updated

9 years ago
Attachment #342283 - Flags: review?(igor) → review+

Comment 17

9 years ago
(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.
(Assignee)

Updated

9 years ago
Attachment #342280 - Flags: approval1.9.0.4?
(Assignee)

Updated

9 years ago
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?]
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.18+
(Assignee)

Comment 19

9 years ago
changeset:   20487:dcf70800258a
Status: NEW → RESOLVED
Last Resolved: 9 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! ;) )

Comment 21

9 years ago
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+
(Assignee)

Comment 24

9 years ago
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
(Assignee)

Comment 25

9 years ago
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. :-)
(Assignee)

Comment 27

9 years ago
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.
(Assignee)

Comment 29

9 years ago
http://www.squarefree.com/shell/  <--- should work fine.
(Assignee)

Comment 30

9 years ago
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.
Keywords: fixed1.9.0.4 → verified1.9.0.4
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.
Keywords: fixed1.8.1.18 → verified1.8.1.18

Comment 35

9 years ago
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+

Comment 36

9 years ago
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.
Alias: CVE-2008-5024
Group: core-security

Comment 37

9 years ago
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-
http://hg.mozilla.org/mozilla-central/rev/395be2ab5c6b

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1

Updated

9 years ago
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.