[FIX]BoxObject's InvalidatePresentationStuff needs ability to tell document to remove it from its boxobject table

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
P1
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: bz)

Tracking

(Depends on: 1 bug, 4 keywords)

Trunk
mozilla1.9alpha1
fixed-aviary1.0.8, fixed1.7.13, fixed1.8.1, verified1.8.0.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8b3 -
blocking1.8b5 -
blocking-aviary1.5 -
blocking1.8.0.2 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] [no l10n impact][rft-dl])

Attachments

(3 attachments, 1 obsolete attachment)

We can get into situations where an element's box object becomes useless because
of some sequence of events like this: frames are destroyed (e.g. in response to
opacity changes (?)), InvalidatePresentationStuff is called on the box object in
question, but the document still holds it in its boxobject table, so that when
script attempts to subsequently access boxobject methods, the busted boxobject
with null mPresShell etc is used, and none of the methods work. 

This was noted in earlier testing versions of my new prefwindow where I had a
listbox and was attempting to access properties on it after the pane was faded in. 

The solution appears to be that in some cases when InvalidatePresentationStuff
is called, we want to tell the document to take it out of the boxobject table,
which is what this patch does. See also the patch in 282103, which updates the
call sites in nsXULDocument.
Created attachment 174197 [details] [diff] [review]
patch
Blocks: 274712
Status: NEW → ASSIGNED
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta1
*** Bug 282104 has been marked as a duplicate of this bug. ***
Depends on: 282103
Attachment #174197 - Flags: superreview?(jst)
Attachment #174197 - Flags: review?(bzbarsky)
Comment on attachment 174197 [details] [diff] [review]
patch

So... the only caller with PR_TRUE will be nsListBoxBodyFrame::Destroy, right? 
In that case, may I ask why this is nuking the box object in the first place?

It seems to me like the listbox is just abusing the
InvalidatePresentationStuff() api here; while it should be setting the
listboxbody property to null when the listbox frame is destroyed, it should NOT
be nulling out the presshell.  It seems that it wants a separate method it can
call on the nsListBoxObject that will do that.
Attachment #174197 - Flags: review?(bzbarsky) → review-
Flags: blocking1.8b3?
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Whiteboard: [bs] patch has r-, needs work
Whiteboard: [bs] patch has r-, needs work → [no l10n impact] patch has r-, needs work
No longer blocking anything. 
Flags: blocking1.8b4-
Flags: blocking1.8b4+
Flags: blocking-aviary1.1-
Flags: blocking-aviary1.1+
Might I ask why not?  I can see that it might not be blocking Firefox 1.1, but
why isn't it blocking toolkit/gecko 1.8?
Blocks: 322513
This is causing toolkit users problems, apparently.  See bug 322513.  I do think we should fix this...
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Blocks: 325045
OK, this also causes a crash that's most likely an exploitable security hole (bug 325045).  Further, in debugging that I figured out that the way this is set up means that the 1.7 branch is also exploitable in the same way (since content can call setPropertyAsSupports).

I guess I'll take this and see what I can do here.  :(  Does rather make me wish someone had answered my question from comment 5....
Assignee: bugs → bzbarsky
Group: security
Status: ASSIGNED → NEW

Updated

12 years ago
Whiteboard: [no l10n impact] patch has r-, needs work → [sg:critical] [no l10n impact] patch has r-, needs work
Target Milestone: mozilla1.8beta1 → ---
Created attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....
Attachment #174197 - Attachment is obsolete: true
Attachment #174197 - Flags: superreview?(jst)
Attachment #210093 - Flags: superreview?(bugs)
Attachment #210093 - Flags: review?(neil)
Created attachment 210109 [details] [diff] [review]
Same for 1.8 branch
Attachment #210109 - Flags: superreview?(bugs)
Attachment #210109 - Flags: review?(neil)
Created attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

So what these patches do is make sure that content JS

1)  Can't get hold of the list box body frame via XPConnect.
2)  Can't mess with our boxobject's internal frame pointers.

Both are accomplished by having some nonscriptable private interfaces.
Attachment #210110 - Flags: superreview?(bugs)
Attachment #210110 - Flags: review?(neil)
Attachment #210110 - Flags: approval1.7.13?
Attachment #210110 - Flags: approval-aviary1.0.8?
Flags: blocking1.8.0.2?
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:critical] [no l10n impact] patch has r-, needs work → [sg:critical] [no l10n impact]
Target Milestone: --- → mozilla1.9alpha
Attachment #210109 - Flags: approval1.8.1?
Attachment #210109 - Flags: approval1.8.0.2?
Summary: BoxObject's InvalidatePresentationStuff needs ability to tell document to remove it from its boxobject table → [FIX]BoxObject's InvalidatePresentationStuff needs ability to tell document to remove it from its boxobject table

Comment 11

12 years ago
Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

>+  // listboxBody is always null.  It's only here to avoid changing the
>+  // interface.
Surely we can just this completely on trunk?

>+                 "Calling SetPropertyAsSupports on a frame.  Prepare to crash "
>+                 "and be exploited any time some random website decides to "
>+                 "exploit you");
Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the following text.

>-  SetPropertyAsSupports(NS_LITERAL_STRING("listboxbody").get(), nsnull);
>+  ClearCachedListBoxBody();
Using a virtual call to set a member variable?
Attachment #210093 - Flags: review?(neil) → review+

Comment 12

12 years ago
Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

Some previous comments may be relevant.
Attachment #210109 - Flags: review?(neil) → review+

Updated

12 years ago
Attachment #210110 - Flags: review?(neil) → review+
> Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the
> following text.

The following text doesn't make sense without the last part, unless I'm missing something.  Care to suggest the wording you meant?

> Using a virtual call to set a member variable?

Yep.  It's cleaner in this case, and this is not perf-intensive code.
Oh, missed one comment:

> Surely we can just this completely on trunk?

I'd love to do that as a separate patch, but I wanted to minimize merging as much as possible.  As it was, it took me a good long while to get this stuff merged...
Attachment #210109 - Flags: approval1.8.1?

Comment 15

12 years ago
BZ please land on trunk!
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
...when you have the reviews you need.
Whiteboard: [sg:critical] [no l10n impact] → [sg:critical] [no l10n impact] needs sr=bengoodger
Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

> NS_IMETHODIMP
> nsListBoxObject::GetListboxBody(nsIListBoxObject * *aListboxBody)
> {
>-  *aListboxBody = GetListBoxBody();
>-  NS_IF_ADDREF(*aListboxBody);
>+  *aListboxBody = nsnull;
>   return NS_OK;
> }

Why does this return null now?
One of these days I'll learn to read scrollback. Ignore my previous question. 
Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

sr=ben@mozilla.org
Attachment #210093 - Flags: superreview?(bugs) → superreview+
Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

sr=ben@mozilla.org
Attachment #210109 - Flags: superreview?(bugs) → superreview+
Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

sr=ben@mozilla.org
Attachment #210110 - Flags: superreview?(bugs) → superreview+
Whiteboard: [sg:critical] [no l10n impact] needs sr=bengoodger → [sg:critical] [no l10n impact]
Attachment #210109 - Flags: branch-1.8.1?(neil)
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 23

12 years ago
This checkin caused the following on my Win32 cygwin MingW Thunderbird build :-

e:/mozilla/source/mozilla/layout/xul/base/src/nsListBoxObject.cpp:87: error: ext
ra `;'

I think it actually means line 70 (as that is what my Linux build complains about).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
jst fixed the build bustage and I'll make sure to fix it in the branch patches before landing.

For future reference, please just comment; don't reopen.  That doesn't send _nearly_ as much bugspam.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #210109 - Flags: branch-1.8.1?(neil) → branch-1.8.1+
Fixed on 1.8.1 branch.
Keywords: fixed1.8.1
Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

a=dveditz for drivers. Please land soon so we can shake this out.
Attachment #210109 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

a=dveditz for drivers, ditto
Attachment #210110 - Flags: approval1.7.13?
Attachment #210110 - Flags: approval1.7.13+
Attachment #210110 - Flags: approval-aviary1.0.8?
Attachment #210110 - Flags: approval-aviary1.0.8+
Fixed on MOZILLA_1_8_0_BRANCH, MOZILLA_1_7_BRANCH, AVIARY_1_0_1_20050124_BRANCH.
Keywords: fixed-aviary1.0.8, fixed1.7.13, fixed1.8.0.2

Updated

12 years ago
Depends on: 326834

Updated

12 years ago
Flags: testcase-
Boris: If you could provide a quick way for QA to verify the fix in the bug for 1.0.8/1.7.13, that would be very helpful.
Boris: Ignore my last comment, please. We won't get to verifying some of the non-critical bugs for this release.

(In reply to comment #29)
> Boris: If you could provide a quick way for QA to verify the fix in the bug for
> 1.0.8/1.7.13, that would be very helpful.
> 

Marcia, the bugs this bug blocks have testcases that should be verified (that's bug 322513 and bug 325045).  Note that one of them is critical but just didn't have the fixed-* keywords set, since they were set on this bug (which covered the main problem).  I'll try to set fixed-* keywords on all bugs a checkin fixes in the future, not just the bug where the work is happening.

Updated

12 years ago
Whiteboard: [sg:critical] [no l10n impact] → [sg:critical] [no l10n impact][rft-dl]

Comment 32

12 years ago
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, based on verifications of bug 322513 and bug 325045 (pre bz in comment #31)
Keywords: fixed1.8.0.2 → verified1.8.0.2
Group: security
Flags: blocking1.9a1?
Flags: blocking1.8.1?

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets

Updated

9 years ago
Blocks: 457353
You need to log in before you can comment on or make changes to this bug.