Stack overflow involving CSS selectors

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: security, Assigned: dbaron)

Tracking

({crash, fixed1.8.1})

1.8 Branch
x86
Windows XP
crash, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.0.8 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos], URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

I have found a stack overflow in the latest version of Mozilla Firefox that seems to be caused by loading a large font array inside of a stylesheet element. The issue is not present on Mac OSX Intel, but does crash FF on every windows platform I have tested.

Reproducible: Always

Steps to Reproduce:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML>
<HEAD>
<TITLE> New Document </TITLE>
</HEAD>

<BODY>


<script>

var buffer = 'a,';

for (i=0; i < 85750; i++)
{
	buffer += 'g,';
}

document.write('<style>.'+buffer+' { font-family: '+buffer+'; }</style>');

</script>



</BODY>
</HTML>

Actual Results:  
The example above will crash the browser
(Reporter)

Comment 1

13 years ago
"seems to be caused by loading a large font array inside of a stylesheet element"

After looking at this issue for more than a few minutes I realize the problem is with the large number of "tags" defined in the style sheet element and has nothing to do with the font array. But, I am sure you already see that :)

Comment 2

13 years ago
Created attachment 218174 [details]
winxp trunk stack

looks like Bug 279678
> winxp trunk stack
> 
> looks like Bug 279678

Really? I get a stack overflow (as described) with a repeating bunch of nsCSSSelectorList routines on the stack. Quite unlike the js-engine stuff in 279678
Assignee: nobody → dbaron
Status: UNCONFIRMED → NEW
Component: Security → Style System (CSS)
Ever confirmed: true
Product: Firefox → Core
Whiteboard: [sg:dos]
Version: unspecified → 1.8 Branch
(Reporter)

Comment 4

13 years ago
There seems to be a few unusual things going on here. It seems like different results can be achieved by specifying different formats of selectors and selector classes references. For example, in my tests the following script will not crash the browser immediately, but will upon refreshing the page.

<script language="JavaScript" type="text/javascript">

var buffer = 'a';

for (i=0; i < 100000; i++)
{
	buffer += ',a';
}

document.write('<style>'+buffer+' {}</style>');

</script>

This kind of behavior almost makes me think that there are possibly two different, but very similar bugs present? Has anyone found anything yet in the source tree that seems to be the cause?
(Reporter)

Comment 5

13 years ago
Also Confirmed in the latest version of Mozilla Thunderbird. Of course the large selector would have to be written out, and not generated with Javascript.
Created attachment 219718 [details] [diff] [review]
get rid of recursive operations on CSS selector structures

This also gets rid of a good bit of unused code.
Attachment #219718 - Flags: superreview?(bzbarsky)
Attachment #219718 - Flags: review?(bzbarsky)
Attachment #219718 - Flags: approval-branch-1.8.1?
Comment on attachment 219718 [details] [diff] [review]
get rid of recursive operations on CSS selector structures

>Index: nsICSSStyleRule.h
> struct nsAtomList {
>+  /**
>+   * Do a deep clone.  Should be used only on the first in the linked list.
>+   *
>+   * aDeep == PR_FALSE is for internal use only.
>+   */
>+  nsAtomList* Clone(PRBool aDeep = PR_TRUE) const;

I think I'd prefer something like:

  public:
    nsAtomList* Clone() const {
      return Clone(PR_TRUE);
    }
  protected:
   nsAtomList* Clone(PRBool aDeep) const;

instead of relying purely on comments for the "internal use only" thing, if possible.  Same elsewhere.

r+sr=bzbarsky with that.
Attachment #219718 - Flags: superreview?(bzbarsky)
Attachment #219718 - Flags: superreview+
Attachment #219718 - Flags: review?(bzbarsky)
Attachment #219718 - Flags: review+
Attachment #219718 - Flags: approval-branch-1.8.1?
Attachment #219718 - Flags: approval-branch-1.8.1+
Created attachment 219795 [details] [diff] [review]
get rid of recursive operations on CSS selector structures

Patch addressing bz's comments.
Attachment #219718 - Attachment is obsolete: true
Checked in to trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
This patch leaks with chains of negations because moving the cloning of negations outside of the aDeep check means they get double-cloned.
Created attachment 220142 [details] [diff] [review]
fix leak in previous patch
Attachment #220142 - Flags: superreview?(bzbarsky)
Attachment #220142 - Flags: review?(bzbarsky)
Attachment #220142 - Flags: approval-branch-1.8.1?(bzbarsky)
Comment on attachment 220142 [details] [diff] [review]
fix leak in previous patch

>Index: nsICSSStyleRule.h
>   // No need to worry about multiple levels of recursion (or about copying
>   // mNegations->mNext) since an mNegations can't have an mNext.

Is this comment still relevant?

r+sr+a=bzbarsky
Attachment #220142 - Flags: superreview?(bzbarsky)
Attachment #220142 - Flags: superreview+
Attachment #220142 - Flags: review?(bzbarsky)
Attachment #220142 - Flags: review+
Attachment #220142 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #220142 - Flags: approval-branch-1.8.1+
(In reply to comment #12)
> Is this comment still relevant?

Yes, except for the parenthetical.
Leak fix checked in to trunk and MOZILLA_1_8_BRANCH.

Comment 15

13 years ago
Should this remain security-sensitive despite just being a crash?  (e.g. because it affects Thunderbird?)
Severity: normal → critical
Keywords: crash

Updated

13 years ago
Summary: Firefox Stack Overflow → Stack overflow involving CSS selectors
Flags: blocking1.8.0.8?
Group: security
Flags: blocking1.8.0.8? → blocking1.8.0.8-
You need to log in before you can comment on or make changes to this bug.