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

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Edgar Alves]

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).

Comment 1

[Edgar Alves]

Attachment: Test case

Comment 2

[Edgar Alves]

Comment on attachment 160234 [details]
Test case

This was originally set as text/html. Sorry. :)

Comment 3

[chris hofmann]

not sure this bug should be marked security closed...  I'm I missing something?

Comment 4

[Edgar Alves]

(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.

Comment 5

[Daniel Veditz [:dveditz]]

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)

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: 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.

Comment 7

[Boris Zbarsky [: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....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

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).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with the InsertElementsAt() change that dveditz caught

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught

a=dveditz for 1.7 branch

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 160436 [details] [diff] [review]
Patch with the InsertElementsAt() change that dveditz caught

a=asa for aviary checkin.