329677 - [FIX]Persist seems to let any page set any persist value

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looking at the code in nsXULContentUtils::MakeElementID it seems like it allows any page to set any persist attribute. I.e. it can even set persist values for other pages, including chrome pages.

Something like
<hbox id="chrome://browser/content/browser.xul#content" persist="onnewtab"
      onnewtab="alert('insert evilness here');BrowserOpenTab();">

will set the onnewtab attribute in the <browser> of browser.xul. I couldn't get this to work exactly like this, but I do get a

<RDF:Description RDF:about="chrome://browser/content/browser.xul#content"

entry in my localstore. It might be that it's only possible to affect attributes that are already persisted, which would be bad enough.

The offending code is nsXULContentUtils::MakeElementURI which treats ids containing an ':' as an already resolved uri.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So i think it's not that bad that it lets you set any attribute on any element. "Only" attributes on elements that have any other attributes persisted.

This is of course bad enough.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, <window> is a great tag to do this to, since it persists size/position and has a convenient onload attribute that you can inject script into.  :(

Proof-of-concept testcase coming up.  To reproduce, load the XUL file in Firefox (since the chrome URI is Firefox-specific), then open a new window.  Or just quit and restart the browser.  Code runs with chrome privileges, of course.

It sounds to me like we should fix MakeElementURI _and_ add a same-origin check somwhere in here...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Exploit

Don't forget to clean up your localstore.rdf for repeated testing!

Comment 4

[Mike Shaver (:shaver emeritus)]

01:10 <shaver> if we fix MakeElementURI, do we need a same-origin check?
01:11 <shaver> seems like this is a case of a name collision
01:11 <shaver> we don't want to let the content specify _any_ resolved URI, 
               regardless of the domain

Comment 5

[Mike Shaver (:shaver emeritus)]

01:12 <bz_sleep> yes, exactly
01:12 <bz_sleep> but I still feel that after we've got a built URI, if it's not 
                 the same origin as the node we should bail
01:12 <bz_sleep> belt-n-braces
01:13 <shaver> ah
01:13 <bz_sleep> or assert it.  Or something.
01:13 <shaver> yeah, I'd buy that, I think

Comment 6

[[e-mail]]

Might this have been part of some pre-outliner RDF persistence hack?

Comment 7

[Axel Hecht]

I tried to find out why the code does that, and it predates to some ancient
version of nsRDFContentUtils, which doesn't have that call path back then. The
code was added like that in some other context, it seems, and reused later on.

I didn't investigate further back in time and space.

Comment 8

[Boris Zbarsky [:bzbarsky]]

fwiw, you don't have to delete all of localstore.rdf to recover; just the one line this exploit adds... ;)

I'm going to post a proposed patch.  Stay tuned!  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to fix it

Will this not break anything in the sort service?  That uses this stuff too...

Comment 10

[[e-mail]]

>+    // Whoa.  Why the "id" attribute?  What if it's not even a XUL
>+    // element?  This is totally bogus!
Persisting a non-XUL element is totally bogus anyway ;-)

(In reply to comment #9)
>Will this not break anything in the sort service?  That uses this stuff too...
Well, the call in InsertContainerNode looks as if it could simply use nsIRDFService::GetUnicodeResource directly, while the call in SortContainer looks as if it could be using nsXULElement::GetResource instead.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So does that mean that we do or do not need to update those callsites?

Comment 12

[[e-mail]]

(In reply to comment #11)
>So does that mean that we do or do not need to update those callsites?
If I can get my build to complete I'll let you know.

Comment 13

[Axel Hecht]

As we're passing the value of a ref attribute to MakeElementResource in
XULSortServiceImpl::InsertContainerNode, that needs to be updated, AFAICT.
If there's no ref, it tries the id, which may be inside the generated content
and have the id set to a URL of the resource, but I don't know that, leaving
that up to Neil. As with SortContainer.

In terms of persistance, those changes look good, though. I wonder if we should
kill the sharing there and leave templates alone. The correspondence of ID to
RDF resource seems to be fundamentally different in xul sort and xul persist.

Comment 14

[Mike Shaver (:shaver emeritus)]

(In reply to comment #10)
> >+    // Whoa.  Why the "id" attribute?  What if it's not even a XUL
> >+    // element?  This is totally bogus!
> Persisting a non-XUL element is totally bogus anyway ;-)

I disagree, in the general case.  We should be encouraging people to use HTML instead of XUL in their chrome UI where it better suits their needs, and to help us reduce the confusing and bloat-inducing overlap between different text-container types, etc.  I don't see why it's bogus to persist on non-XUL, and would say that we should fix whatever might render it unwise or unworkable today.

Comment 15

[Axel Hecht]

The reusability of our persist vocabulary inside localstore is pretty limited,
though, that is, we can't persist namespaced attributes, either.
I'd say we can safely leave anything beyond "works as designed" as a mental note
for the time when we migrate that data out of localstore.rdf anyway.

Comment 16

[[e-mail]]

(In reply to comment #10)
>the call in InsertContainerNode looks as if it could simply use
>nsIRDFService::GetUnicodeResource directly
Verified using Bookmarks

>the call in SortContainer looks as if it could be using
>nsXULElement::GetResource instead.
Verified using Chatzilla

Comment 17

[[e-mail]]

Attachment: XUL sort service fixes

Comment 18

[Axel Hecht]

Comment on attachment 214784 [details] [diff] [review]
XUL sort service fixes

I think the first chunk is wrong (the behaviour changes if the template just adds a ref for fun, right?), and the second chunk could actually use xulElement->GetResource. Asking the other Neil on this one.

Comment 19

[Axel Hecht]

Comment on attachment 214593 [details] [diff] [review]
This seems to fix it

Once the sort service knows what it's doing, nsXULContentUtils should be stripped.
GetElementResource is a candidate for death, then. Exposing all these undocumented methods proves to be fragile.

Depending on which branches this should land on, I'd like either a follow-up patch or a new one.

Postponing review, I'd like to see the call sites resolve first.

Comment 20

[Neil Deakin]

Comment on attachment 214784 [details] [diff] [review]
XUL sort service fixes

I'd think that the other patch is OK, then this sort service one isn't necessary. Or am I missing something?

Comment 21

[Axel Hecht]

(In reply to comment #20)
> Or am I missing something?

Attachement 214593 changes MakeElementURI to not return the ID as URL if it
contains a ':', but does SetRef unconditionally. Thus, absolute URLs as IDs
don't work through this codepath, which the xul sort service relies on.

The part of the xul sort service I'm puzzled about mostly is that it sometimes
uses just the id attribute, and sometimes uses ref and id (I even think in
changing order, not sure about that one, though). 

Comment 22

[Neil Deakin]

Comment on attachment 214784 [details] [diff] [review]
XUL sort service fixes


>-        nsXULContentUtils::GetElementResource(child, getter_AddRefs(resource));
>+        nsCOMPtr<nsIDOMXULElement> xulElement(do_QueryInterface(child));
>+        xulElement->GetResource(getter_AddRefs(resource));

Axel is right about this changing behaviour in one case, however unlikely:

<menuitem id="some_resource" ref="?thing"/>

Comment 23

[Axel Hecht]

(In reply to comment #22)
> Axel is right about this changing behaviour in one case, however unlikely:

Enn, would that be an r- or an r+ on that patch?

Comment 24

[[e-mail]]

(In reply to comment #22)
><menuitem id="some_resource" ref="?thing"/>
These are template generated elements, so they won't have a ref, will they?.

Comment 25

[Neil Deakin]

Comment on attachment 214784 [details] [diff] [review]
XUL sort service fixes

It changes behaviour, but in a way which is unlikely to break any existing code. I'm ok with a r+ if no other fix presents itself.

Comment 26

[Axel Hecht]

Comment on attachment 214593 [details] [diff] [review]
This seems to fix it

Ok, the three mails asking me to review this for 1.8.0.3 answer comment #19.

I do want a follow-up bug on the cleanup of nsXULContentUtils, though.
Sidenote, I'm not convinced that changing undocumented parts of templates is good, even if they're terribly obscure.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Combined trunk patch

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.0 branch patch

Comment 29

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 219350 [details] [diff] [review]
1.8 branch patch

Or not.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

Comment 33

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.  Followup bug 335000 filed.

Comment 34

[Boris Zbarsky [:bzbarsky]]

This caused bug 335142

Comment 35

[Scott MacGregor]

This also caused Bug #335175.

I've confirmed that the fix in 335142 fixes this regression to. 

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 219331 [details] [diff] [review]
1.8.0 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 37

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch.

Comment 38

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060504 Firefox/1.5.0.4

Comment 39

[Alexander Sack]

Attachment: 1.0.x branch

Comment 40

[Johnny Stenback (:jst)]

Clearing stale requests.