453915 - (CVE-2008-5024) XML injection possible in E4X parsing via "default xml namespace"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Evans]

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]

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]

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]

Attachment: 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]

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]

Comment on attachment 341458 [details] [diff] [review]
use js_QuoteString

Nevermind, this is wrong, as mrbkap pointed out on IRC.

Comment 6

[Brian Crowder]

(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]

Attachment: Use js_EscapeAttributeValue

Comment 8

[Brian Crowder]

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

Comment 9

[Igor Bukanov]

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[]   = "'";
...
>     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]

Attachment: smaller

Still worried about gc hazards here, can you double-check that?

Comment 11

[Samuel Sidler (old account; do not CC)]

We'll take this when the trunk picks it up.

Comment 12

[Brian Crowder]

Attachment: backport to 1.8

Comment 13

[Brian Crowder]

Attachment: backport to 1.9

Comment 14

[Brian Crowder]

Attachment: correct version of 1.8 backport

Comment 15

[Igor Bukanov]

(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]

Attachment: diff between 1.8 and 1.9 backport

Comment 17

[Igor Bukanov]

(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]]

Is this going to land on trunk? we'd like some verification of the fix before approving for the branches.

Comment 19

[Brian Crowder]

changeset:   20487:dcf70800258a

Comment 20

[Samuel Sidler (old account; do not CC)]

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]

js> default xml namespace = '---"---&---\'---';
js> uneval(<foo/>)                              
<foo xmlns="---&quot;---&amp;---'---"/>

Looks good to me.

Comment 22

[Daniel Veditz [:dveditz]]

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]]

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]

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]

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 - ex-MoCo]]

(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]

The testcase from comment #21 is pretty easy.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

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]

http://www.squarefree.com/shell/  <--- should work fine.

Comment 30

[Brian Crowder]

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 - ex-MoCo]]

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 - ex-MoCo]]

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 - ex-MoCo]]

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 - ex-MoCo]]

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]

Comment on attachment 342283 [details] [diff] [review]
correct version of 1.8 backport

a=asac for 1.8.0

Comment 36

[Alexander Sack]

Attachment: 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] (inactive)]

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]

http://hg.mozilla.org/mozilla-central/rev/395be2ab5c6b