Last Comment Bug 329677 - [FIX]Persist seems to let any page set any persist value
: [FIX]Persist seems to let any page set any persist value
Status: RESOLVED FIXED
[sg:critical] Exploit in comment 3; o...
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P1 blocker (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Neil Deakin
Mentors:
Depends on: 335142 335175 337841 500045
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-07 14:41 PST by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2010-02-11 14:48 PST (History)
12 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Exploit (385 bytes, application/vnd.mozilla.xul+xml)
2006-03-08 22:09 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
This seems to fix it (7.81 KB, patch)
2006-03-09 12:26 PST, Boris Zbarsky [:bz] (still a bit busy)
axel: review+
neil: superreview+
Details | Diff | Splinter Review
XUL sort service fixes (2.76 KB, patch)
2006-03-11 08:58 PST, neil@parkwaycc.co.uk
enndeakin: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Combined trunk patch (10.56 KB, patch)
2006-04-21 11:01 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
1.8.0 branch patch (9.59 KB, patch)
2006-04-21 11:11 PDT, Boris Zbarsky [:bz] (still a bit busy)
neil: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review
1.8 branch patch (9.67 KB, patch)
2006-04-21 13:32 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
1.8 branch patch (9.61 KB, patch)
2006-04-21 13:43 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
1.0.x branch (7.71 KB, patch)
2006-06-13 14:53 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-07 14:41:45 PST
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 2006-03-07 16:25:39 PST
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 [:bz] (still a bit busy) 2006-03-08 22:08:45 PST
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 [:bz] (still a bit busy) 2006-03-08 22:09:25 PST
Created attachment 214520 [details]
Exploit

Don't forget to clean up your localstore.rdf for repeated testing!
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-03-08 22:12:44 PST
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 -- probably not reading bugmail closely) 2006-03-08 22:13:52 PST
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 neil@parkwaycc.co.uk 2006-03-09 05:56:30 PST
Might this have been part of some pre-outliner RDF persistence hack?
Comment 7 Axel Hecht 2006-03-09 06:06:11 PST
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 [:bz] (still a bit busy) 2006-03-09 10:42:26 PST
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 [:bz] (still a bit busy) 2006-03-09 12:26:14 PST
Created attachment 214593 [details] [diff] [review]
This seems to fix it

Will this not break anything in the sort service?  That uses this stuff too...
Comment 10 neil@parkwaycc.co.uk 2006-03-09 13:27:05 PST
>+    // 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 [:bz] (still a bit busy) 2006-03-09 19:07:47 PST
So does that mean that we do or do not need to update those callsites?
Comment 12 neil@parkwaycc.co.uk 2006-03-10 03:02:54 PST
(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 2006-03-10 03:56:11 PST
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 -- probably not reading bugmail closely) 2006-03-10 09:09:59 PST
(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 2006-03-10 09:47:04 PST
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 neil@parkwaycc.co.uk 2006-03-10 13:12:57 PST
(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 neil@parkwaycc.co.uk 2006-03-11 08:58:38 PST
Created attachment 214784 [details] [diff] [review]
XUL sort service fixes
Comment 18 Axel Hecht 2006-04-10 09:34:35 PDT
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 2006-04-10 10:09:42 PDT
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 2006-04-10 10:26:53 PDT
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 2006-04-10 10:48:46 PDT
(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 2006-04-12 09:57:25 PDT
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 2006-04-17 15:46:51 PDT
(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 neil@parkwaycc.co.uk 2006-04-17 15:55:18 PDT
(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 2006-04-17 18:22:16 PDT
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 2006-04-21 04:07:54 PDT
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 [:bz] (still a bit busy) 2006-04-21 11:01:46 PDT
Created attachment 219329 [details] [diff] [review]
Combined trunk patch
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 11:11:37 PDT
Created attachment 219331 [details] [diff] [review]
1.8.0 branch patch
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 11:12:08 PDT
Fixed on trunk.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 13:32:49 PDT
Created attachment 219350 [details] [diff] [review]
1.8 branch patch
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 13:41:11 PDT
Comment on attachment 219350 [details] [diff] [review]
1.8 branch patch

Or not.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 13:43:40 PDT
Created attachment 219354 [details] [diff] [review]
1.8 branch patch
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2006-04-21 13:55:14 PDT
Fixed on 1.8 branch.  Followup bug 335000 filed.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2006-04-23 23:00:39 PDT
This caused bug 335142
Comment 35 Scott MacGregor 2006-04-24 11:29:25 PDT
This also caused Bug #335175.

I've confirmed that the fix in 335142 fixes this regression to. 
Comment 36 Daniel Veditz [:dveditz] 2006-04-24 11:38:20 PDT
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 [:bz] (still a bit busy) 2006-04-24 12:10:25 PDT
Fixed on 1.8.0 branch.
Comment 38 Tracy Walker [:tracy] 2006-05-04 08:27:04 PDT
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 2006-06-13 14:53:19 PDT
Created attachment 225480 [details] [diff] [review]
1.0.x branch
Comment 40 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-11 14:48:21 PST
Clearing stale requests.

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