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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
P1
blocker
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: sicking, Assigned: bz)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
mozilla1.9alpha1
fixed1.8.1, verified1.8.0.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 3 obsolete attachments)

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?
(Assignee)

Comment 2

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

Comment 3

12 years ago
Created attachment 214520 [details]
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

Updated

12 years ago
Whiteboard: [sg:critical] Exploit in comment 3; only load it if you don't mind deleting localstore.rdf

Comment 6

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

Comment 7

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

Comment 8

12 years ago
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!  ;)

Updated

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

Comment 9

12 years ago
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...
Attachment #214593 - Flags: superreview?(neil)
Attachment #214593 - Flags: review?(axel)

Comment 10

12 years ago
>+    // 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.
(Assignee)

Comment 11

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

Comment 12

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

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

Comment 15

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

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

12 years ago
Created attachment 214784 [details] [diff] [review]
XUL sort service fixes
Attachment #214784 - Flags: superreview?(bzbarsky)
Attachment #214784 - Flags: review?(axel)
(Assignee)

Updated

12 years ago
Attachment #214784 - Flags: superreview?(bzbarsky) → superreview+

Updated

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

Updated

12 years ago
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 18

12 years ago
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 19

12 years ago
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?

Comment 21

12 years ago
(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"/>

Comment 23

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

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

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

Comment 27

11 years ago
Created attachment 219329 [details] [diff] [review]
Combined trunk patch
Attachment #214593 - Attachment is obsolete: true
Attachment #214784 - Attachment is obsolete: true
(Assignee)

Comment 28

11 years ago
Created attachment 219331 [details] [diff] [review]
1.8.0 branch patch
Attachment #219331 - Flags: approval1.8.0.3?
Attachment #219331 - Flags: approval-branch-1.8.1?(neil)
(Assignee)

Comment 29

11 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #219331 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
(Assignee)

Comment 30

11 years ago
Created attachment 219350 [details] [diff] [review]
1.8 branch patch
(Assignee)

Updated

11 years ago
Attachment #219331 - Attachment description: 1.8(.0) branch patch → 1.8.0 branch patch
(Assignee)

Comment 31

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

Or not.
Attachment #219350 - Attachment is obsolete: true
(Assignee)

Comment 32

11 years ago
Created attachment 219354 [details] [diff] [review]
1.8 branch patch
(Assignee)

Comment 33

11 years ago
Fixed on 1.8 branch.  Followup bug 335000 filed.
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Depends on: 335142
(Assignee)

Comment 34

11 years ago
This caused bug 335142

Comment 35

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

Comment 37

11 years ago
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
Keywords: fixed1.8.0.4 → verified1.8.0.4
(Assignee)

Updated

11 years ago
Depends on: 337841

Comment 39

11 years ago
Created attachment 225480 [details] [diff] [review]
1.0.x branch

Updated

11 years ago
Attachment #225480 - Flags: review?(caillon)
Group: security

Updated

9 years ago
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?

Updated

8 years ago
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.