319847 - CVE-2006-0296 XULDocument.persist() allows an attacker to inject bogus RDF data into localstore.rdf

Status: VERIFIED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

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]

Attachment: testcase 1 - exploit for Firefox 1.0.7, 1.5 and trunk

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - exploit for Mozilla 1.7.12

Comment 3

[Daniel Veditz [:dveditz]]

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]

I think Axel worked on this in the RDF serializer layer.

Comment 5

[moz_bug_r_a4]

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]

Attachment: testcase 3

This could deal with the RDF serializer issue.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Would it be possible and feasable for nsXULDocument::Persist to escape its input here?

Comment 8

[Benjamin Smedberg]

The escaping should be performed by the serializer, not by the client code.

Comment 9

[moz_bug_r_a4]

Attachment: testcase 4

This should work even when localstore.rdf contains many data.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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

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]

Comment on attachment 205810 [details] [diff] [review]
Fix the serializer, v1

Looks good to me.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 14

[moz_bug_r_a4]

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]

Attachment: testcase 5

injects bogus RDF data into the local part of the qualified name.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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

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

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

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

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

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]

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

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

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

Attachment: No namespace qualifiers

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

sr=jst

Comment 27

[Benjamin Smedberg]

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

I've checked the last fix into the trunk. Hopefully this plugs nsXULDocument::Persist for good.

Comment 29

[Mike Schroepfer]

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

I checked attachment 206132 [details] [diff] [review] into the branches.

Comment 31

[Marcia Knous [:marcia]]

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

You should get an error in your javascript console, as well.

Comment 33

[Bob Clary [:bc] (inactive)]

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

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

Checked into aviary101/moz17 branches

Comment 36

[Jay Patel [:jay]]

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.