The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
Security
P2
normal
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({fixed1.7.13, verified1.8.0.1, verified1.8.1})

Trunk
mozilla1.9alpha1
x86
Windows XP
fixed1.7.13, verified1.8.0.1, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8.0.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 205523 [details]
testcase 1 - exploit for Firefox 1.0.7, 1.5 and trunk
(Reporter)

Comment 2

12 years ago
Created attachment 205524 [details]
testcase 2 - exploit for Mozilla 1.7.12
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.
Depends on: 295994
Flags: blocking1.8.0.1+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical]
I think Axel worked on this in the RDF serializer layer.
(Reporter)

Comment 5

12 years ago
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).
(Reporter)

Comment 6

12 years ago
Created attachment 205629 [details]
testcase 3

This could deal with the RDF serializer issue.
(Assignee)

Comment 7

12 years ago
Would it be possible and feasable for nsXULDocument::Persist to escape its input here?
The escaping should be performed by the serializer, not by the client code.
(Reporter)

Comment 9

11 years ago
Created attachment 205698 [details]
testcase 4

This should work even when localstore.rdf contains many data.
(Assignee)

Comment 10

11 years ago
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?).
Assignee: dveditz → mrbkap
Status: NEW → ASSIGNED
Attachment #205810 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #205810 - Flags: review? → review?(benjamin)
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.
Attachment #205810 - Flags: review?(benjamin) → review+

Comment 12

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

Looks good to me.
Attachment #205810 - Flags: review+
(Assignee)

Comment 13

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Reporter)

Comment 14

11 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 15

11 years ago
Created attachment 205981 [details]
testcase 5

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

Comment 16

11 years ago
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.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 17

11 years ago
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).
Attachment #206041 - Flags: superreview?(jst)
Attachment #206041 - Flags: review?

Comment 18

11 years ago
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"?
(Assignee)

Comment 19

11 years ago
(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

11 years ago
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.
(Assignee)

Comment 21

11 years ago
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

11 years ago
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.
(Assignee)

Comment 23

11 years ago
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.
Attachment #206041 - Attachment is obsolete: true
Attachment #206125 - Flags: superreview?(jst)
Attachment #206125 - Flags: review?(axel)
Attachment #206041 - Flags: superreview?(jst)
Attachment #206041 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #206125 - Flags: review?(benjamin)

Comment 24

11 years ago
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
Attachment #206125 - Flags: review?(axel) → review-
(Assignee)

Comment 25

11 years ago
Created attachment 206132 [details] [diff] [review]
No namespace qualifiers
Attachment #206125 - Attachment is obsolete: true
Attachment #206132 - Flags: superreview?(jst)
Attachment #206132 - Flags: review?(axel)
Attachment #206125 - Flags: superreview?(jst)
Attachment #206125 - Flags: review?(benjamin)
(Assignee)

Updated

11 years ago
Attachment #206132 - Flags: review?(benjamin)
Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

sr=jst
Attachment #206132 - Flags: superreview?(jst) → superreview+
Comment on attachment 206132 [details] [diff] [review]
No namespace qualifiers

Our namespace handling in xul:persist is so screwed up :-(
Attachment #206132 - Flags: review?(benjamin) → review+
(Assignee)

Comment 28

11 years ago
I've checked the last fix into the trunk. Hopefully this plugs nsXULDocument::Persist for good.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #206132 - Flags: review?(axel) → review+

Comment 29

11 years ago
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.
Attachment #206132 - Flags: approval1.8.0.1+
(Assignee)

Comment 30

11 years ago
I checked attachment 206132 [details] [diff] [review] into the branches.
Keywords: fixed1.8.0.1, fixed1.8.1
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? 
(Assignee)

Comment 32

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

Comment 33

11 years ago
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]
Status: RESOLVED → VERIFIED
Flags: testcase?
Keywords: fixed1.8.0.1, fixed1.8.1 → verified1.8.0.1, verified1.8.1

Updated

11 years ago
Flags: testcase? → testcase+

Updated

11 years ago
Summary: 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 data into localstore.rdf
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().
Checked into aviary101/moz17 branches
Keywords: fixed-aviary1.0.8, fixed1.7.13
Attachment #210095 - Flags: approval1.7.13+
Attachment #210095 - Flags: approval-aviary1.0.8+

Comment 36

11 years ago
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.
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
Group: security

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.