crash if CSSStyleSheet::insertRule is used with an empty string

RESOLVED FIXED in mozilla1.8alpha5

Status

()

P1
critical
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: epa, Assigned: bzbarsky)

Tracking

({crash, fixed-aviary1.0, fixed1.7.5})

Trunk
mozilla1.8alpha5
x86
All
crash, fixed-aviary1.0, fixed1.7.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8
Build Identifier: http://ftp.mozilla.org/pub/mozilla.org/mozilla/releases/mozilla1.7.3/

When the method CSSStyleSheet::insertRule is called with an empty string as the
CSS rule, the browser crashes (tested with Mozilla 1.7.3 and Firefox 1.0PR).

Reproducible: Always
Steps to Reproduce:
1. Open the file attached to this bug report with either Mozilla or Firefox.

Actual Results:  
Crash.

Expected Results:  
The JavaScript console should display an error (DOMException SYNTAX_ERR).
(Reporter)

Comment 1

15 years ago
Created attachment 160234 [details]
Test case
(Reporter)

Comment 2

15 years ago
Comment on attachment 160234 [details]
Test case

This was originally set as text/html. Sorry. :)
Attachment #160234 - Attachment mime type: text/html → text/plain
Keywords: crash

Comment 3

15 years ago
not sure this bug should be marked security closed...  I'm I missing something?
(Reporter)

Comment 4

15 years ago
(In reply to comment #3)
> not sure this bug should be marked security closed...  I'm I missing something?

My reasoning was that since this bug is triggered by a string of a particular
size (0) maybe the code isn't robust enough to cope with some other strings.

Since it seemed plausible to me that such a vulnerability could also be used to
perform a buffer overflow attack I've marked the bug as security-sensitive.

Of course, I hope that I'm wrong and someone quickly finds that an empty string
is the only case where the code fails and removes the security-sensitive flag.
Confirming. I've spent some time poking at this and it's crashing on a null
dereference. Removing security-sensitive flag: an empty rule won't lead to a
buffer overflow, and non-empty illegal strings are handled OK.

Not nice to be able to crash someone at will, of course.

Crashing at line 2680 in nsCSSStyleSheet.cpp (aviary branch) dereferencing
firstRule. The obvious local fix would be an if (firstRule) check like the if
(nextRule) above, but I defer to bz in case he wants to fix this higher on the
food chain (this section last touched Sept 2001 fixing bug 93977 and bug 98358)
Assignee: general → dveditz
Group: security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 160375 [details] [diff] [review]
Aviary patch

This fixes the crash, but as mentioned earlier I don't know if this should have
been caught higher up.

The patch also includes a somewhat unrelated change from NS_ENSURE_SUCCESS() to
NS_ENSURE_TRUE() on inserting the rules. _SUCCESS() is a noop here, if the
check is useful (for performance?) we should make this change, otherwise drop
it. "unrelated" because in the case of this bug if we avoid the crash we
successfully insert 0 elements and call DidDirty() anyway.
Attachment #160375 - Flags: superreview?(dbaron)
Attachment #160375 - Flags: review?(bzbarsky)
(In reply to comment #5)
> but I defer to bz in case he wants to fix this higher on the
> food chain

I think I do, yes.  Patch coming up....
Created attachment 160435 [details] [diff] [review]
Patch
Attachment #160375 - Attachment is obsolete: true
Comment on attachment 160375 [details] [diff] [review]
Aviary patch

Marking r-, since this isn't quite the best fix (and misses similar code for
rule groups).
Attachment #160375 - Flags: superreview?(dbaron)
Attachment #160375 - Flags: superreview-
Attachment #160375 - Flags: review?(bzbarsky)
Attachment #160375 - Flags: review-
Created attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught
Attachment #160435 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #160436 - Flags: superreview?(dbaron)
Attachment #160436 - Flags: review?(dbaron)
(Assignee)

Updated

15 years ago
Assignee: dveditz → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha5
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught

This is a low-risk crash fix well-worth taking on the branches, in my opinion.
Attachment #160436 - Flags: approval1.7.x?
Attachment #160436 - Flags: approval-aviary?
Comment on attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught

a=dveditz for 1.7 branch
Attachment #160436 - Flags: approval1.7.x? → approval1.7.x+
Comment on attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught

a=asa for aviary checkin.
Attachment #160436 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Updated

15 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
You need to log in before you can comment on or make changes to this bug.