618643 - No integer overflow check at nsScannerBufferList::AllocBufferFromString

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Alex Miller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: 

At http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsScannerString.cpp#52 , a string of at least 2^31 - 5 bytes will cause an overflow, and result in an incorrect amount of memory being allocated.

Reproducible: Didn't try




Because 32-bit windows by default can only keep track of 2 GB of memory addresses, this should not be exploitable on that platform.

Comment 1

[Jonathan Kew [:jfkthame]]

I'm not sure whether it's actually possible to construct a scenario where such a huge string will reach this code, but it certainly looks fragile/risky. Maybe you'd need a 64-bit system in order to reach this without hitting an OOM failure earlier? But I'm just speculating, really.

While we're here, there's a similar unchecked calculation in the following nsScannerBufferList::AllocBuffer() function; if someone can cause this to be called with a "capacity" argument of almost 2^31, the calculation will overflow.
http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsScannerString.cpp#74

Comment 2

[Daniel Veditz [:dveditz]]

Alex: adding your own "[sg:" marking to the whiteboard bypasses the security bug triage process and can counter-productively sideline your bug: the triage team won't see it and there's no assignee to take care of it. Eventually it will become a "critical bug not touched in a while" (assuming you don't poke at it yourself) and then get noticed.

Comment 4

[Daniel Veditz [:dveditz]]

Confirming by inspection, not convinced this is exploitable in practice. To start with you'd already have to have just shy of 4G memory tied up in the string passed to AllocBufferFromString(), not counting the memory tied up with the document we parsed that string out of.

Comment 5

[Alex Miller]

(In reply to comments #2 & #3)

Sorry, and as much as I hate playing the blame game Bas said it was okay. Also, this seems exploitable on 64-bit systems with a good deal of memory, especially if an attacker triggers the overflow using a text/html data: URL.

Comment 6

[Daniel Veditz [:dveditz]]

If you can construct a testcase that crashes a consumer-grade PC due to this bug we can re-evaluate, but at the moment it doesn't seem to be a pressing issue. We should fix it anyway, of course.

Comment 7

[Jesse Ruderman]

I don't understand the downgrade to sg:moderate. Many Linux distros are already 64-bit, and we're about to ship Firefox 4 as 64-bit for Mac OS X 10.6.

Does this bug exist on trunk?

Comment 8

[Alex Miller]

(In reply to comment #7)
> I don't understand the downgrade to sg:moderate. Many Linux distros are already
> 64-bit, and we're about to ship Firefox 4 as 64-bit for Mac OS X 10.6.
> 
> Does this bug exist on trunk?

It should, as this bug is in trunk code.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Fix.

Comment 10

[(no longer active)]

Johnny, did you mean to ask for somebody's review? :-)

Comment 11

[Johnny Stenback (:jst)]

I did! But I failed because that person was not on the cc list, and this bug is restricted etc...

Comment 12

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f7fdf1512b

Comment 13

[Johnny Stenback (:jst)]

Looks like this got merged to m-c.

http://hg.mozilla.org/mozilla-central/rev/22f7fdf1512b