Closed Bug 329677 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: sicking, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:critical] Exploit in comment 3; only load it if you don't mind editing localstore.rdf)

Attachments

(5 files, 3 obsolete files)

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.
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.
Flags: blocking1.8.0.3?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
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...
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Attached file Exploit
Don't forget to clean up your localstore.rdf for repeated testing!
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
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
Whiteboard: [sg:critical] Exploit in comment 3; only load it if you don't mind deleting localstore.rdf
Might this have been part of some pre-outliner RDF persistence hack?
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.
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!  ;)
Whiteboard: [sg:critical] Exploit in comment 3; only load it if you don't mind deleting localstore.rdf → [sg:critical] Exploit in comment 3; only load it if you don't mind editing localstore.rdf
Attached patch This seems to fix it (obsolete) — Splinter Review
Will this not break anything in the sort service?  That uses this stuff too...
Attachment #214593 - Flags: superreview?(neil)
Attachment #214593 - Flags: review?(axel)
>+    // 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.
So does that mean that we do or do not need to update those callsites?
(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.
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.
(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.
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.
(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
Attached patch XUL sort service fixes (obsolete) — Splinter Review
Attachment #214784 - Flags: superreview?(bzbarsky)
Attachment #214784 - Flags: review?(axel)
Attachment #214784 - Flags: superreview?(bzbarsky) → superreview+
Attachment #214593 - Flags: superreview?(neil) → superreview+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: Persist seems to let any page set any persist value → [FIX]Persist seems to let any page set any persist value
Target Milestone: --- → mozilla1.9alpha
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.
Attachment #214784 - Flags: review?(axel) → review?(enndeakin)
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 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?
(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 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"/>
(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?
(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 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.
Attachment #214784 - Flags: review?(enndeakin) → review+
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.
Attachment #214593 - Flags: review?(axel) → review+
Attachment #214593 - Attachment is obsolete: true
Attachment #214784 - Attachment is obsolete: true
Attachment #219331 - Flags: approval1.8.0.3?
Attachment #219331 - Flags: approval-branch-1.8.1?(neil)
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #219331 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Attached patch 1.8 branch patch (obsolete) — Splinter Review
Attachment #219331 - Attachment description: 1.8(.0) branch patch → 1.8.0 branch patch
Comment on attachment 219350 [details] [diff] [review]
1.8 branch patch

Or not.
Attachment #219350 - Attachment is obsolete: true
Attached patch 1.8 branch patchSplinter Review
Fixed on 1.8 branch.  Followup bug 335000 filed.
Keywords: fixed1.8.1
Depends on: 335142
This caused bug 335142
This also caused Bug #335175.

I've confirmed that the fix in 335142 fixes this regression to. 
Depends on: 335175
Comment on attachment 219331 [details] [diff] [review]
1.8.0 branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #219331 - Flags: approval1.7.14?
Attachment #219331 - Flags: approval-aviary1.0.9?
Attachment #219331 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fixed on 1.8.0 branch.
Flags: blocking1.9a1?
Keywords: fixed1.8.0.3
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
Depends on: 337841
Attached patch 1.0.x branchSplinter Review
Attachment #225480 - Flags: review?(caillon)
Group: security
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Attachment #219331 - Flags: approval1.7.14?
Attachment #219331 - Flags: approval-aviary1.0.9?
Depends on: 500045
Clearing stale requests.
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
You need to log in before you can comment on or make changes to this bug.