Closed Bug 611778 Opened 9 years ago Closed 9 years ago

DOMParser errors, data from other pages returned in error

Categories

(Core :: XML, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: agilmore, Assigned: bent.mozilla)

Details

(Keywords: regression, Whiteboard: [sg:moderate] fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: 4.0b7

An XML document is downloaded async by a worker thread, the XML is then passed via postMessage() and parsed via DOMParser.parseFromString().

This causes an error, and appears to be accessing memory outside this page (Im not a C guy so my terminology may be off).  Ive had it return data/content from other tabs, the firefox installation, history, but mostly gibberish.

Reproducible: Sometimes

Steps to Reproduce:
1.Create a Worker()
2.Have worker download XML via xmlhttprequest async
3.pass downloaded XML back via postMessage
4.parse XML via DOMParser.parseFromString()
5.Check Error Console for errors
6.May need to be repeated several times to trigger bug, but will normally happen within the first 3 attempts
Actual Results:  
Throws a 'no well-formed' error, indicates the parsing error occurred on data that isn't in the XML.

Expected Results:  
It should parse the XML document without error.

This bug is accessing memory it shouldn't including content on other tabs.  I suspect there might be potential that someone with more C skills then myself could turn it into a remote execution exploit, so checking it as 'security' even tho its beta.  I'm not a big buzilla person, if this is incorrect use of the 'security' checkbox, apologies.
blocking2.0: --- → ?
Perfect use of the security checkbox -- thanks.

If it's just reading wrong/random memory into the response text it's unlikely to be turned into a remote execution exploit, but it certainly could potentially leak private information (internal urls? passwords?) if an attacker wanted to sift through it.
Attachment #490180 - Attachment mime type: application/zip → application/java-archive
rats, I was hoping we could run that without having to download if we changed the content-type to application/java-archive. Unfortunately the web worker won't load :-(

I think what's happening is the page principal should be the innermost URI, but the CheckMayLoad() comparison explicitly doesn't want to de-nest jar: URLs

Same-origin check here:
http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerScriptLoader.cpp#479

CheckMayLoad() comment about not de-nesting here:
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#406

Originally CheckMayLoad() was only used for file:// loading checks. Its use has expanded and may not be entirely consistent. If I got the worker to load I wonder if I'd have a similar same-origin failure trying to XHR the local xml files -- XHR uses CheckMayLoad() also.

I suppose this whole comment is off-topic for this bug, except to say
...that testers will have to download the .zip and expand it locally to test.
(sorry for the premature submission)
I've got a patch for this ready, uploading soon.
Assignee: nobody → bent.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I can't reproduce on the 1.9.2 branch, but can (easily) on minefield.
Keywords: regression
Basically the structured clone stuff introduced with JS compartments copy non-null-terminated strings. Our parser assumes null-terminated strings only, and keeps walking until it hits a two-byte null in the buffer. Most code uses the length of the nsString properly without doing a strlen equivalent so I'm not sure it's really a problem for anything other than calling DOMParser.parseFromString.

Not sure how to judge sg:moderate vs. sg:high. The patch is nearly done, fwiw.
Attached patch Patch, v1 (obsolete) — Splinter Review
I believe this should fix everything.
Attachment #490439 - Flags: review?(jorendorff)
Attached patch Patch, v1.1 (obsolete) — Splinter Review
Found another abuser in jsregexp.cpp
Attachment #490439 - Attachment is obsolete: true
Attachment #490944 - Flags: review?(jorendorff)
Attachment #490439 - Flags: review?(jorendorff)
Whiteboard: [sg:moderate] maybe sg:high?
I'd love to see this fix landed in beta8, but I'm not going to block beta8 on this.
blocking2.0: ? → betaN+
For the sake of continuing to develop XUL apps using the beta, is there an easy workaround in the meantime?  Some way to null terminate the string prior to calling DOMParser.parseFromString?
Comment on attachment 490944 [details] [diff] [review]
Patch, v1.1

In jsatom.cpp:
> JSAtom *
> js_AtomizeChars(JSContext *cx, const jschar *chars, size_t length, uintN flags)
> {
>     JSString str;
> 
>     CHECK_REQUEST(cx);
>-    str.initFlat((jschar *)chars, length);
>+    str.initFlatNotTerminated((jschar *)chars, length);

I forget -- Why is it OK for this not to be null-terminated?

In jsregexp.cpp:
>+        if (!newChars.reserve(len + 1))
>+            return NULL;
>+        newChars.append('\0');

Instead, just

          if (!newChars.append('\0'))
              return NULL;

In jsstr.h, initFlatExtensible:
>+        JS_ASSERT(chars[length] == (jschar)0);

Micro-nit: Please say either jschar(0) or just 0.

r=me with those addressed.
Attachment #490944 - Flags: review?(jorendorff) → review+
Whiteboard: [sg:moderate] maybe sg:high? → [sg:moderate]
Attached patch Patch, v1.2Splinter Review
Fixed the things you commented about.

As I understand it all atoms are copied, and null terminated. When we make temporary JSString objects on the stack to pass as args to the js_AtomizeFoo functions we're never going to hang on to the actual chars passed. If an atom exists then we'll hand out the null terminated string, and if it doesn't then we copy, null terminate, and then hand that one out. I think the key is looking at the ATOM_TMPSTR flag.
Attachment #490944 - Attachment is obsolete: true
Attachment #496151 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/4c18087c8bda
Whiteboard: [sg:moderate] → [sg:moderate] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/4c18087c8bda
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.