Closed
Bug 453915
(CVE-2008-5024)
Opened 15 years ago
Closed 15 years ago
XML injection possible in E4X parsing via "default xml namespace"
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: scarybeasts, Assigned: crowderbt)
Details
(4 keywords, Whiteboard: [sg:low?])
Attachments
(5 files, 3 obsolete files)
1.17 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
2.10 KB,
patch
|
igor
:
review+
dveditz
:
approval1.9.0.4+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
igor
:
review+
dveditz
:
approval1.8.1.18+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
612 bytes,
patch
|
Details | Diff | Splinter Review | |
2.04 KB,
patch
|
Details | Diff | Splinter Review |
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•15 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•15 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•15 years ago
|
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Flags: blocking1.8.1.18?
Updated•15 years ago
|
Assignee: general → crowder
Assignee | ||
Comment 3•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
||
Attachment #341494 -
Flags: review?(igor)
Assignee | ||
Comment 8•15 years ago
|
||
With the patch, the testcase produces: js> default xml namespace = '\''; js> <foo/>
Comment 9•15 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[] = "'"; ... > 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•15 years ago
|
||
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•15 years ago
|
Attachment #341528 -
Flags: review?(igor) → review+
Comment 11•15 years ago
|
||
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•15 years ago
|
||
Attachment #342279 -
Flags: review?(igor)
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #342280 -
Flags: review?(igor)
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #342279 -
Attachment is obsolete: true
Attachment #342283 -
Flags: review?(igor)
Attachment #342279 -
Flags: review?(igor)
Updated•15 years ago
|
Attachment #342280 -
Flags: review?(igor) → review+
Comment 15•15 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•15 years ago
|
||
Updated•15 years ago
|
Attachment #342283 -
Flags: review?(igor) → review+
Comment 17•15 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•15 years ago
|
Attachment #342280 -
Flags: approval1.9.0.4?
Assignee | ||
Updated•15 years ago
|
Attachment #342283 -
Flags: approval1.8.1.18?
Comment 18•15 years ago
|
||
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•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.18+
Assignee | ||
Comment 19•15 years ago
|
||
changeset: 20487:dcf70800258a
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 20•15 years ago
|
||
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•15 years ago
|
||
js> default xml namespace = '---"---&---\'---'; js> uneval(<foo/>) <foo xmlns="---"---&---'---"/> Looks good to me.
Comment 22•15 years ago
|
||
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 23•15 years ago
|
||
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•15 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•15 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
Comment 26•15 years ago
|
||
(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•15 years ago
|
||
The testcase from comment #21 is pretty easy.
Comment 28•15 years ago
|
||
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•15 years ago
|
||
http://www.squarefree.com/shell/ <--- should work fine.
Assignee | ||
Comment 30•15 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.
Comment 31•15 years ago
|
||
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•15 years ago
|
||
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
Comment 33•15 years ago
|
||
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
Comment 34•15 years ago
|
||
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•15 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•15 years ago
|
||
based on attachment 342283 [details] [diff] [review]; just some context adjustments to apply cleanly.
Updated•15 years ago
|
Alias: CVE-2008-5024
Updated•15 years ago
|
Group: core-security
Comment 37•15 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-
Comment 38•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/395be2ab5c6b
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Updated•15 years ago
|
Keywords: verified1.9.1
Updated•15 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•