Last Comment Bug 319847 - CVE-2006-0296 XULDocument.persist() allows an attacker to inject bogus RDF data into localstore.rdf
: CVE-2006-0296 XULDocument.persist() allows an attacker to inject bogus RDF da...
Status: VERIFIED FIXED
[sg:critical]
: fixed1.7.13, verified1.8.0.1, verified1.8.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on: CVE-2008-5505
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-10 22:46 PST by moz_bug_r_a4
Modified: 2007-04-01 15:32 PDT (History)
9 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8.0.1+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix the serializer, v1 (1.78 KB, patch)
2005-12-14 00:40 PST, Blake Kaplan (:mrbkap)
benjamin: review+
axel: review+
Details | Diff | Splinter Review
Protect the serializer from itself (3.23 KB, patch)
2005-12-15 17:52 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Enforce the 2nd argument (1.17 KB, patch)
2005-12-16 13:09 PST, Blake Kaplan (:mrbkap)
axel: review-
Details | Diff | Splinter Review
No namespace qualifiers (2.23 KB, patch)
2005-12-16 14:41 PST, Blake Kaplan (:mrbkap)
axel: review+
benjamin: review+
jst: superreview+
mtschrep: approval1.8.0.1+
Details | Diff | Splinter Review
Backport to 1.0.x (2.07 KB, patch)
2006-01-29 17:22 PST, Christopher Aillon (sabbatical, not receiving bugmail)
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2005-12-10 22:46:45 PST
XULDocument.persist() function doesn't verify whether the second argument is
a valid attribute name.  Thus, an attacker can inject bogus RDF data into
localstore.rdf in order to run arbitrary code.

Steps to Reproduce:
1. Load testcase, and click "Click me!" button.
2. Shutdown the browser.
3. (See the content of localstore.rdf.)
4. Start the browser.

An alert dialog that shows Components.stack will appear.


This affects:
Firefox 1.0.7, 1.5 and trunk
Mozilla 1.7.12
Comment 1 moz_bug_r_a4 2005-12-10 22:48:19 PST
Created attachment 205523 [details]
testcase 1 - exploit for Firefox 1.0.7, 1.5 and trunk
Comment 2 moz_bug_r_a4 2005-12-10 22:49:24 PST
Created attachment 205524 [details]
testcase 2 - exploit for Mozilla 1.7.12
Comment 3 Daniel Veditz [:dveditz] 2005-12-12 01:14:18 PST
I can reproduce on 1.0.7 but not in 1.5. Will play more tomorrow, I can't think of any changes that would "fix" this in 1.5.
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-12-12 05:23:37 PST
I think Axel worked on this in the RDF serializer layer.
Comment 5 moz_bug_r_a4 2005-12-12 07:18:02 PST
When any of the following condition is satisfied, you can't reproduce on 1.5
and trunk.

1) localstore.rdf contains many data.

In this case, JS console shows many "junk after document element" error and one
"[nsIRDFService.GetDataSource]" error on startup.

Error: junk after document element
Source File: file:///C:/Profiles/Firefox/fx-1.5/localstore.rdf
Line: 11, Column: 1
Source Code:
<RDF:RDF a="#"^

Error: uncaught exception: [Exception... "Component returned failure code: 
0x8000ffff (NS_ERROR_UNEXPECTED) [nsIRDFService.GetDataSource]"  nsresult: 
"0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: 
chrome://browser/content/search.xml :: initialize :: line 94"  data: no]


2) xmlns:NS1 already exists.

(due to the RDF serializer issue: Bug 307558 comment 17)

I will attach new testcase that could deal with 2).
Comment 6 moz_bug_r_a4 2005-12-12 07:23:29 PST
Created attachment 205629 [details]
testcase 3

This could deal with the RDF serializer issue.
Comment 7 Blake Kaplan (:mrbkap) 2005-12-12 12:51:19 PST
Would it be possible and feasable for nsXULDocument::Persist to escape its input here?
Comment 8 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-12-12 13:14:30 PST
The escaping should be performed by the serializer, not by the client code.
Comment 9 moz_bug_r_a4 2005-12-12 23:09:58 PST
Created attachment 205698 [details]
testcase 4

This should work even when localstore.rdf contains many data.
Comment 10 Blake Kaplan (:mrbkap) 2005-12-14 00:40:58 PST
Created attachment 205810 [details] [diff] [review]
Fix the serializer, v1

I need to pick out bsmedberg or axel on IRC to explain to me exactly how the second argument to persist ends up being a _namespace_ in the serialized RDF document, and we need to make sure bugs are filed on the fact that the RDF content sink will gladly accept a malformed XML file as input, but this patch fixes the exploit by making sure that the argument to persist is not able to take over the rest of the file by escaping it (do I need to actually do that string copy?).
Comment 11 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-12-14 05:20:53 PST
Comment on attachment 205810 [details] [diff] [review]
Fix the serializer, v1

doh! /me wonders why we didn't do this before...

I'd like Pike to look at this, but I don't know what his travel situation is; let's get this landed on trunk and get him to ex-post-facto review this before putting it on branch.
Comment 12 Axel Hecht 2005-12-14 06:13:08 PST
Comment on attachment 205810 [details] [diff] [review]
Fix the serializer, v1

Looks good to me.
Comment 13 Blake Kaplan (:mrbkap) 2005-12-14 11:02:16 PST
Fix checked into trunk.
Comment 14 moz_bug_r_a4 2005-12-15 10:03:36 PST
It's still exploitable.

http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFXMLSerializer.cpp#228
228 nsresult
229 nsRDFXMLSerializer::RegisterQName(nsIRDFResource* aResource)
230 {
231     nsCAutoString uri, qname;
232     aResource->GetValueUTF8(uri);
233 
234     nsNameSpaceMap::const_iterator iter = mNameSpaces.GetNameSpaceOf(uri);
235     if (iter != mNameSpaces.last()) {
236         NS_ENSURE_TRUE(iter->mPrefix, NS_ERROR_UNEXPECTED);
237         iter->mPrefix->ToUTF8String(qname);
238         qname.Append(':');
239         qname += StringTail(uri, uri.Length() - iter->mURI.Length());
240         return mQNames.Put(aResource, qname) ? NS_OK : NS_ERROR_FAILURE;
241     }

When the second argument to persist is a string beginning with an existing
namespace URI, the substring following the namespace URI part ends up being
treated as the local part of the qualified name.  Thus, an attacker can still
inject any string without being escaped.

Is it possible to do nsContentUtils::CheckQName for |aAttr| at 
nsXULDocument::Persist?
Comment 15 moz_bug_r_a4 2005-12-15 10:06:15 PST
Created attachment 205981 [details]
testcase 5

injects bogus RDF data into the local part of the qualified name.
Comment 16 Blake Kaplan (:mrbkap) 2005-12-15 10:28:26 PST
Damnit, I was looking at the QName stuff when I found the namespace problem, but managed to get myself turned around by all of the Gets and Sets.
Comment 17 Blake Kaplan (:mrbkap) 2005-12-15 17:52:20 PST
Created attachment 206041 [details] [diff] [review]
Protect the serializer from itself

bsmedberg and I talked about this on IRC, and we concluded that the easiest way to deal with the case of a malformed QName is to simply stop its setting in the XUL document. I'll open another bug on making the serializer itself deal with malformed URIs and QNames.

This patch is slightly ugly because I had to copy the logic to find the QName from the RDF serializer. We might want to consider putting a function to do that on the RDF service to avoid this sort of code duplication. It also would have been nice to have a RFindCharInReadable in nsReadableUtils (I'll look into adding one).
Comment 18 Axel Hecht 2005-12-16 03:31:43 PST
Comment on attachment 206041 [details] [diff] [review]
Protect the serializer from itself

+    // XXX Why does this use RFindChar for the hash mark?

Is that question answered by "RDF vocabularies favor http://someurl/for/vocab#item for resources, with QName being vocab:item"?
Comment 19 Blake Kaplan (:mrbkap) 2005-12-16 10:17:40 PST
(In reply to comment #18)
> Is that question answered by "RDF vocabularies favor
> http://someurl/for/vocab#item for resources, with QName being vocab:item"?

Not quite. Given http://someurl/for/vocab#item#foo, we'll currently use a namespace of "http://someurl/for/vocab#item#" and a name of "foo", whereas most of the times I've seen URL parsers do this, they've treated the _first_ hash as the end of the URL (and beginning of the resource name). I'm actually tempted to also throw an error (in nsXULDocument::Persist) if there is more than one # mark.
Comment 20 Axel Hecht 2005-12-16 11:07:31 PST
I don't really have the relevant specs at my finger tip, but both the URL and
the XML QName specs are involved here. Is foo:bar#item a valid QName? A pound in
a namespace is surely OK, so I'd have no problem with keeping it this way.

I won't look that up in the specs today, though.
Comment 21 Blake Kaplan (:mrbkap) 2005-12-16 11:09:17 PST
No that isn't a valid qname, but I'm not sure if my example is a valid URL either (I'm not even sure which spec covers URLs).
Comment 22 Axel Hecht 2005-12-16 12:29:28 PST
Actually, persist is completely broken with any namespaced attribute (or rather,
working ones are just coincidence), so nsXULDocument::Persist should bail on 
anything that is not an NCName, http://www.w3.org/TR/xml-names11/#NT-NCName.

The right fix for namespaces and persist is to store the localname and the
namespace separatly, no way around it. Just try to part "urn:x-foo:bar" "item"
after making one string out of it.
Comment 23 Blake Kaplan (:mrbkap) 2005-12-16 13:09:29 PST
Created attachment 206125 [details] [diff] [review]
Enforce the 2nd argument

The second argument to persist should be a QName, so we should verify that. We might as well let a namespace specifier get in (it can't hurt if content can say NC:foo, right?). This patch stops all testcases here cold.
Comment 24 Axel Hecht 2005-12-16 13:24:34 PST
Comment on attachment 206125 [details] [diff] [review]
Enforce the 2nd argument

QNames are not supported. They just don't work.

See
http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULDocument.cpp#1147
http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULDocument.cpp#2310

It may be sensible to throw a NS_ERROR_NOT_IMPLEMENTED in the case of a non-NCName QName, though, instead of an INVALID_ARG
Comment 25 Blake Kaplan (:mrbkap) 2005-12-16 14:41:30 PST
Created attachment 206132 [details] [diff] [review]
No namespace qualifiers
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-16 16:47:14 PST
Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

sr=jst
Comment 27 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-12-19 11:27:32 PST
Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

Our namespace handling in xul:persist is so screwed up :-(
Comment 28 Blake Kaplan (:mrbkap) 2005-12-19 13:18:28 PST
I've checked the last fix into the trunk. Hopefully this plugs nsXULDocument::Persist for good.
Comment 29 Mike Schroepfer 2006-01-11 11:38:58 PST
Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

a=schrep for Drivers.  This bug should have landed before the freeze since it was nominated as a blocker.
Comment 30 Blake Kaplan (:mrbkap) 2006-01-11 11:49:36 PST
I checked attachment 206132 [details] [diff] [review] into the branches.
Comment 31 Marcia Knous [:marcia - use ni] 2006-01-12 15:56:44 PST
I am trying to verify this on the branch, and I followed the initial STR and and ran testcase 1 - I did not get the alert dialog - is this enough to verify? 
Comment 32 Blake Kaplan (:mrbkap) 2006-01-12 16:07:59 PST
You should get an error in your javascript console, as well.
Comment 33 Bob Clary [:bc:] 2006-01-13 00:35:25 PST
reproduced testcase 5 with vanilla firefox 1.5 but not with 1.8.0.1, 1.8.1 1.9a1 on winxp from 20060112 where it generated:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMXULDocument.persist]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=205981&action=view :: a :: line 19"  data: no]
Comment 34 Christopher Aillon (sabbatical, not receiving bugmail) 2006-01-29 17:22:35 PST
Created attachment 210095 [details] [diff] [review]
Backport to 1.0.x

For reference, the trunk patch mostly applied to 1.0.x; there was also a build error because nsContentUtils::GetParserService() did not exist in 1.0.x -- the line needed to be changed to reference nsContentUtils::GetParserServiceWeakRef().
Comment 35 Daniel Veditz [:dveditz] 2006-01-31 07:32:31 PST
Checked into aviary101/moz17 branches
Comment 36 Jay Patel [:jay] 2006-02-09 14:49:42 PST
v.fixed on 1.0.8 Aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20060209 Firefox/1.0.7 with the various testcases in this bug.

Note You need to log in before you can comment on or make changes to this bug.