If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 22

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 2 bugs, {assertion, sec-critical, testcase})

Trunk
mozilla24
x86_64
Mac OS X
assertion, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ verified, firefox23+ fixed, firefox24 fixed, firefox-esr1722+ verified, b2g1822+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [adv-main22+][adv-esr1707+])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 737937 [details]
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?
(Reporter)

Comment 1

5 years ago
Created attachment 737938 [details]
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)

Updated

5 years ago
Assignee: nobody → bugs
Olli, how bad do you think this is?
Flags: needinfo?(bugs)
(Assignee)

Comment 7

4 years ago
This
Summary: "ASSERTION: wrong shell" with editor, cross-iframe selection → "ASSERTION: wrong shell" with editor, cross-document selection
(Assignee)

Comment 8

4 years ago
This is bad. sg-crit
Keywords: sec-critical
(Assignee)

Comment 9

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

Comment 10

4 years ago
Created attachment 746610 [details] [diff] [review]
patch

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+
Created attachment 746656 [details] [diff] [review]
wip

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

Comment 13

4 years ago
Created attachment 746657 [details] [diff] [review]
patch

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

Comment 14

4 years ago
(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.
status-b2g18: --- → affected
status-firefox21: --- → wontfix
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox-esr17: --- → ?

Updated

4 years ago
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox-esr17: ? → +

Updated

4 years ago
tracking-firefox-esr17: + → ?

Updated

4 years ago
tracking-b2g18: ? → 22+
tracking-firefox-esr17: ? → 22+
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f45cb70707

Updated

4 years ago
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
Last Resolved: 4 years ago
status-b2g18-v1.0.1: --- → affected
status-firefox24: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddef037b226b
https://hg.mozilla.org/releases/mozilla-esr17/rev/ef7a8f39085e
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-firefox-esr17: affected → fixed
Whiteboard: [checkin after 5/14]
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef0168b5cf1c
https://hg.mozilla.org/releases/mozilla-beta/rev/5a4e14b433bc
status-firefox22: affected → fixed
status-firefox23: affected → fixed

Comment 21

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

Comment 22

4 years ago
That second one is there on purpose. See comment 11.
But we'll need to fix the code to not trigger that assertion.

Updated

4 years ago
status-firefox22: fixed → verified
(Reporter)

Comment 23

4 years ago
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+]

Comment 24

4 years ago
Verified as fixed on Firefox 17.0.7 ESR with the same results as in comment 21. Marking as verified...
status-firefox-esr17: fixed → verified
Group: core-security
(Reporter)

Updated

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