Closed Bug 862309 Opened 7 years ago Closed 7 years ago

"ASSERTION: Trying to compute style without PresShell" with editor, cross-document selection

Categories

(Core :: Editor, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 + verified
firefox23 + fixed
firefox24 --- fixed
firefox-esr17 22+ verified
b2g18 22+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [adv-main22+][adv-esr1707+])

Attachments

(5 files)

Attached file testcase
###!!! ASSERTION: Trying to compute style without PresShell: 'presShell', file /Users/jruderman/trees/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.cpp, line 592

###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file /Users/jruderman/trees/mozilla-central/layout/base/nsPresShell.cpp, line 6582

The "wrong shell" assertion was added in a security bug (bug 635852).

I'm guessing this will be fixed by not allowing selections to be in other documents.  That's part of the plan in bug 671152 and bug 768756, right?
Attached file stacks (gdb)
(In reply to Jesse Ruderman from comment #0)

> I'm guessing this will be fixed by not allowing selections to be in other
> documents.  That's part of the plan in bug 671152 and bug 768756, right?

Both unassigned :/
Yeah we need somebody to finish them...
Flags: needinfo?(bugs)
Mats can you help figure out the way forward here?
Flags: needinfo?(matspal)
Group: layout-core-security
Group: layout-core-security
Sorry, I don't have time to work on this.  BTW, Core/Editor security bugs
(and Range/Selection too) belongs to the DOM security group, not Layout.
Flags: needinfo?(matspal)
Assignee: nobody → bugs
Olli, how bad do you think this is?
Flags: needinfo?(bugs)
This
Summary: "ASSERTION: wrong shell" with editor, cross-iframe selection → "ASSERTION: wrong shell" with editor, cross-document selection
This is bad. sg-crit
(In reply to Jesse Ruderman from comment #0)> 
> ###!!! ASSERTION: Trying to compute style without PresShell: 'presShell',
> file
> /Users/jruderman/trees/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.
> cpp, line 592
In other words, this is bogus 

> 
> ###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file
> /Users/jruderman/trees/mozilla-central/layout/base/nsPresShell.cpp, line 6582
But this is bad
Attached patch patchSplinter Review
Bug 671152 should be fixed, but that isn't something for branches.
So perhaps we should just add a simple if check for this case.
Mats, what do you think?
Attachment #746610 - Flags: review?(matspal)
Comment on attachment 746610 [details] [diff] [review]
patch

Yes, this is a good precaution.  It's still caused by bug(s) in our code
though so I'd like to keep the assertions in PresShell, so please add the
NS_ENSURE_STATE after the DEBUG block.  r=mats
Attachment #746610 - Flags: review?(matspal) → review+
Attached patch wipSplinter Review
Fwiw, I wonder if we could something like this to reject nodes not in
the same document as the Selection owner.  It would throw ERROR_FAILURE
which is probably wrong -- WRONG_DOCUMENT seems more appropriate.

BTW, is this the authoritative spec for selectAllChildren ?
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections
Attached patch patchSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No really

Which older supported branches are affected by this flaw?
All, and looks like  Bug 635852 landed everywhere so this patch could also.
The bug itself is old, not a recent regression.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe, but not risk free. Some testing is needed.
Attachment #746657 - Flags: sec-approval?
Attachment #746657 - Flags: approval-mozilla-esr17?
Attachment #746657 - Flags: approval-mozilla-beta?
Attachment #746657 - Flags: approval-mozilla-b2g18?
Attachment #746657 - Flags: approval-mozilla-aurora?
(In reply to Mats Palmgren [:mats] from comment #12) 
> Fwiw, I wonder if we could something like this to reject nodes not in
> the same document as the Selection owner.  It would throw ERROR_FAILURE
> which is probably wrong -- WRONG_DOCUMENT seems more appropriate.
I was thinking something similar but I think we just need bug 671152 fixed, and make sure the
spec is sane.


> BTW, is this the authoritative spec for selectAllChildren ?
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections
Kind of
Comment on attachment 746657 [details] [diff] [review]
patch

sec-approval+ (for m-c) after 5/14. Please make and nominate patches for affected branches.
Attachment #746657 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin after 5/14]
(In reply to Mats Palmgren [:mats] from comment #12)
> BTW, is this the authoritative spec for selectAllChildren ?
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections

It's the only spec I know of, but I sure wouldn't view it as authoritative.  More like, some research went into it and it's a good starting point for discussion.
Attachment #746657 - Flags: approval-mozilla-esr17?
Attachment #746657 - Flags: approval-mozilla-esr17+
Attachment #746657 - Flags: approval-mozilla-beta?
Attachment #746657 - Flags: approval-mozilla-beta+
Attachment #746657 - Flags: approval-mozilla-b2g18?
Attachment #746657 - Flags: approval-mozilla-b2g18+
Attachment #746657 - Flags: approval-mozilla-aurora?
Attachment #746657 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a8f45cb70707
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I tested this fix on Mac OSX 10.7 with Firefox 22 beta 2 debug and the testcase in this bug.

The first assertion in comment 0 has disappeared, but the second one is still there, just on another line:

###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file ../../../layout/base/nsPresShell.cpp, line 6604.

Would you rather I filed a followup bug for that, or just reopen this one?
That second one is there on purpose. See comment 11.
But we'll need to fix the code to not trigger that assertion.
Filed bug 876826.  Thanks for keeping that from slipping through the cracks, Ioana.
Summary: "ASSERTION: wrong shell" with editor, cross-document selection → "ASSERTION: Trying to compute style without PresShell" with editor, cross-document selection
Whiteboard: [Unhide with bug 876826]
Whiteboard: [Unhide with bug 876826] → [Unhide with bug 876826][adv-main22+][adv-esr1707+]
Verified as fixed on Firefox 17.0.7 ESR with the same results as in comment 21. Marking as verified...
Group: core-security
Whiteboard: [Unhide with bug 876826][adv-main22+][adv-esr1707+] → [adv-main22+][adv-esr1707+]
You need to log in before you can comment on or make changes to this bug.