Crash in nsTreeColumns::RestoreNaturalOrder()

RESOLVED FIXED in mozilla23

Status

()

Core
XUL
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: abillings, Assigned: mats)

Tracking

({crash, csectype-nullptr, testcase})

1.9.2 Branch
mozilla23
crash, csectype-nullptr, testcase
Points:
---

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 wontfix)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 600519 [details]
PoC #1

This is derived from bug 708186. The reporter offered three additional PoCs that only affected 1.9.2. Since they were not SVG related, it was decided to open a new bug for the issue.

The first PoC crashes with bp-979112cd-d44b-45a3-81c1-aeb9c2120224 on my Mac.

The second PoC crashes at bp-1465ec42-6ccb-4703-91b4-569c72120224.

The third PoC crashes at bp-e0564e71-4740-407a-aa24-34eb22120224.
(Reporter)

Comment 1

6 years ago
Created attachment 600520 [details]
PoC #2
(Reporter)

Comment 2

6 years ago
Created attachment 600521 [details]
PoC #3
(Reporter)

Updated

6 years ago
blocking1.9.2: --- → ?
for PoC 1 and 2 I get
bp-b709db30-881f-4f46-9bbd-e25d12120224

For PoC 3 I get a crash at a slightly different place, nsTreeContentView::SetTree(nsITreeBoxObject*)
bp-6d2ee95e-24ed-443c-9a62-9c5ac2120224

In all three cases it's dying on a null tree, held by a nsCOMPtr so this is an unexploitable crash. Null means it's either never set or--as in this case--correctly cleaned up and isn't going to be some random pointer to something freed.
Crash Signature: [@ nsTreeColumns::RestoreNaturalOrder() ] [@ nsTreeContentView::SetTree(nsITreeBoxObject*) ]
Group: core-security
If you allow remote XUL (magic per-site permission setting) it still crashes nightly. bp-fe50ac32-c776-4f30-8688-c4f8d2120224

Not so much a user concern (no remote XUL in Firefox 4+) but means an add-on or Firefox code might end up with a stability problem.

Updated

6 years ago
Severity: normal → critical
Keywords: crash
PoC 1 and 2 can be fixed by programming defensively.

PoC 3 passes the javascript object {} to idl defined function setTree defined as

void setTree(in nsITreeBoxObject tree);

in layout/xul/base/src/tree/public/nsITreeView.idl. {} obviously doesn't implement the nsITreeBoxObject interface, but what is the proper way of determining that? Should this even be allowed at all?
(In reply to Timothy Nikkel (:tn) from comment #5)
> PoC 3 passes the javascript object {} to idl defined function setTree
> defined as
> 
> void setTree(in nsITreeBoxObject tree);
> 
> in layout/xul/base/src/tree/public/nsITreeView.idl. {} obviously doesn't
> implement the nsITreeBoxObject interface, but what is the proper way of
> determining that? Should this even be allowed at all?

Ah, but it does.  XPConnect is just insane here ... but I'm not sure that we can fix that.
Unless it's sane for nsITreeBoxObject to be implemented by JS, we should mark it builtinclass which will fix #3.
I don't think we need to worry about this one on the dying branch
blocking1.9.2: ? → -
status1.9.2: --- → wontfix
(Assignee)

Comment 9

5 years ago
Created attachment 734246 [details] [diff] [review]
Fix+tests for #1 and #2
Assignee: nobody → matspal
Attachment #734246 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Keywords: csec-nullptr, testcase
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:dos null deref]
Comment on attachment 734247 [details] [diff] [review]
Fix+test for #3

>@@ -471,16 +471,20 @@ nsTreeContentView::SetTree(nsITreeBoxObj
>     nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mBoxObject);
>+    if (!boxObject) {
>+      mBoxObject = nullptr;
>+      return NS_ERROR_INVALID_ARG;
>+    }

This will ensure that it is at least a nsIBoxObject, but is it possible for the passed in object to be a nsIBoxObject but not a nsITreeBoxObject? I don't know the xpconnect magic that allows this fake nsITreeBoxObject so I'm not sure.
Attachment #734246 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #11)
> Comment on attachment 734247 [details] [diff] [review]
> Fix+test for #3
> 
> >@@ -471,16 +471,20 @@ nsTreeContentView::SetTree(nsITreeBoxObj
> >     nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mBoxObject);
> >+    if (!boxObject) {
> >+      mBoxObject = nullptr;
> >+      return NS_ERROR_INVALID_ARG;
> >+    }
> 
> This will ensure that it is at least a nsIBoxObject, but is it possible for
> the passed in object to be a nsIBoxObject but not a nsITreeBoxObject? I
> don't know the xpconnect magic that allows this fake nsITreeBoxObject so I'm
> not sure.

QI'ing to nsISupports and then back nsITreeBoxObject should be enough to ensure we have a valid object, so we can do that if we don't get a better answer.
(Assignee)

Comment 13

5 years ago
NS_IMETHODIMP
nsTreeContentView::SetTree(nsITreeBoxObject* aTree)

Hmm, I thought that the signature would guarantee that.
It's really a bug in the caller if a non-null aTree doesn't QI
to nsITreeBoxObject.

Anyway, I tested QI aTree to nsITreeBoxObject, both directly and via
nsISupports and both results in a pointer value === aTree.
I don't understand the XPConnect magic that makes that work either
but it seems like that extra check is pointless.
You're right. I thought QI'ing to nsISupports and then back nsITreeBoxObject wouldn't work.

The question is then what should we do about this?

The other thing I noticed is that nsITreeBoxObject doesn't inherirt from nsIBoxObject. So maybe we should set mBoxObject to null if aTree doesn't QI to nsIBoxObject?
(Assignee)

Comment 15

5 years ago
The patch does set mBoxObject to null in that case (ClearRows() always
nulls out mRoot unconditionally); I don't think we can do more in SetTree.
This stuff isn't exposed to unprivileged content so I think we're fine.

I'll remove the !mRoot from the if-condition since it's always true.
Ok, that sounds good then.
Created attachment 740430 [details] [diff] [review]
Fix+test for #3

This just does what comment 15 says. I got impatient. :)
Attachment #734247 - Attachment is obsolete: true
Attachment #734247 - Flags: review?(tnikkel)
Attachment #740430 - Flags: review?(matspal)
(Assignee)

Comment 18

5 years ago
Comment on attachment 740430 [details] [diff] [review]
Fix+test for #3

Thanks!
Attachment #740430 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/352ceffb0d9e
https://hg.mozilla.org/mozilla-central/rev/a6639d6743db
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.