443288 - (CVE-2008-0016) Investigate CVE 2008-0016: crash [@ nsACString_internal::SetLength]

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Brandon Sterne (:bsterne)]

Attachment: IBM advisory

Justin Schuh and Tom Cross of the IBM X-Force and Peter Williams of IBM Watson Labs reported this vulnerability to security@m.o.

I have attached the full advisory they provided.  I can confirm that this crashes Firefox 2.0.0.15 but not 3.0.  I will attach a sample backtrace from the crash I'm seeing which for me appears to be a null deref with a small offset.  This differs quite a bit from the advisory notes, so it would be great if we can get some additional eyes looking at this to figure out what's going on.

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: Backtrace from branch crash

Comment 2

[Brandon Sterne (:bsterne)]

Attachment: testcase crashes branch

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe fixed by the patches to one of bug 394275, bug 395651, bug 377360?  Or maybe even bug 282083?

Comment 4

[Al Billings [:abillings - ex-MoCo]]

I checked this with 2.0.0.16 and it cashes... :-)

Comment 5

[Daniel Veditz [:dveditz]]

In a debug build I get the following assertions:
WARNING: NS_ENSURE_TRUE(IsUTF8(input)) failed, file c:/dev/ff2/mozilla/netwerk/dns/src/nsIDNService.cpp, line 267
###!!! ASSERTION: Not a UTF-8 string. This code should only be used for converting from known UTF-8 strings.: 'Error', file ../../../dist/include/string\nsUTF8Utils.h, line 264
###!!! ASSERTION: not a UTF8 string: 'Error', file ../../dist/include/string\nsUTF8Utils.h, line 151
###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file c:/dev/ff2/mozilla/xpcom/string/src/nsReadableUtils.cpp, line 262

And then an access violation in nsTAString_CharT::Length() where "this" is suspiciously equal to 0x00410041 ("AA") and of course contains garbage.

String code out of control, need to assume the worst.

Should the string code be more robust in the face of non-UTF8 (but thereby slower), or is it the responsibility of the callers (e.g. IDN in this case) to keep non-UTF8 out? It looks like we're taking the latter tack but that seems dangerous.

Comment 6

[Justin Schuh]

The demo you were provided has hard-coded offsets for a locally opened file in Firefox 2.0.0.14 on Windows; the shellcode just pops up a command shell and kills the browser. If you need it, I can see if IBM/ISS will let me provide you a proof-of-concept updated for 2.0.0.16. It's easy to do technically, but I don't see the point since even the version with 2.0.0.14 offsets overwrites EIP with the "AA" filler string from the URL.

Also, I just checked the current code to see if the bug is still there. The surrogate pair handling in ConvertUTF16toUTF8::write() has changed a bit between versions, invalidating the proof-of-concept you were provided. However, an exploit may still be possible by landing the last byte of the input on a valid high-surrogate. I haven't looked at the code in a while, so I'm not sure if that vector is feasible from the improperly cracked URL.

Comment 7

[Justin Schuh]

Sorry, by "I just checked the current code" I meant to say Firefox 3.0, which I guess is core 1.9.0.

Comment 8

[Daniel Veditz [:dveditz]]

Simon: does this fall into your baliwick?

Comment 9

[Simon Montagu :smontagu]

(In reply to comment #5)
> Should the string code be more robust in the face of non-UTF8 (but thereby
> slower), or is it the responsibility of the callers (e.g. IDN in this case) to
> keep non-UTF8 out? It looks like we're taking the latter tack but that seems
> dangerous.

I agree. What is causing this crash is that nsIDNService checks for invalid UTF-8 in Normalize(), but not in ConvertUTF8toACE(). We can patch that specific case (and maybe that's all we should do for the 1.8 branch), but in general I think we should make the string code handle invalid input better.

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch to nsIDNService::ConvertUTF8toACE

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, but where does the invalid UTF-8 come from?

Comment 12

[Justin Schuh]

(In reply to comment #11)
> Hm, but where does the invalid UTF-8 come from?
> 

I provided a detailed explanation of the entire vulnerable code path in the advisory attached to this bug. However, the short answer is that you're handling whitespace inconsistently inside nsBaseURLParser::ParseURL(). So, when it cracks the URL the end of the string lands inside a multibyte character.

Comment 13

[Simon Montagu :smontagu]

I think it makes sense to split off the issues described in the advisory in attachment 327885 [details] into separate bugs. I've filed bug 415613, bug 415615 and bug 415617.

Comment 14

[Brandon Sterne (:bsterne)]

(In reply to comment #13)
> I think it makes sense to split off the issues described in the advisory in
> attachment 327885 [details] into separate bugs. I've filed bug 415613, bug 
> 415615 and bug 415617.

I believe you meant bug 451613, bug 451615, and bug 451617.

Comment 15

[Simon Montagu :smontagu]

Sorry, yes (as in the "Depends on" field)

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

sr=dveditz

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

> However, the short answer is that you're
> handling whitespace inconsistently inside nsBaseURLParser::ParseURL(). So, when
> it cracks the URL the end of the string lands inside a multibyte character.

So why shouldn't we fix ParseURL instead?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

I guess doing the same here as in Normalize isn't too bad

Comment 19

[Samuel Sidler (old account; do not CC)]

Dan said he might have time to check this in today since Simon is out.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

Approved for 1.8.1.17, a=dveditz for release-drivers.

Comment 21

[Daniel Veditz [:dveditz]]

Fix checked into the 1.8 branch

Comment 22

[Daniel Veditz [:dveditz]]

--> FIXED since this is a 1.8-only bug.

Comment 23

[Justin Schuh]

Could someone please add me to bugs 451613, 451613, and 451617 so I can track the issue for our reporting purposes?

Comment 24

[Justin Schuh]

(In reply to comment #23)
> Could someone please add me to bugs 451613, 451613, and 451617 so I can track
> the issue for our reporting purposes?
> 

Sorry, that should have included bug 451615.

Comment 25

[Brandon Sterne (:bsterne)]

(In reply to comment #24)

I added Justin to the 3 bugs he requested access to.

Comment 26

[Simon Montagu :smontagu]

(In reply to comment #17)

> So why shouldn't we fix ParseURL instead?

s/instead/as well/ => bug 451613

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17, where I *CAN'T* reproduce the crash.

I *CAN* reproduce the crash using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.16) Gecko/2008070205 Firefox/2.0.0.16, with testcase: https://bugzilla.mozilla.org/attachment.cgi?id=327888.

Replacing fixed1.8.1.17 keyword with verified1.8.1.17.

Comment 28

[Alexander Sack]

Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

a=asac for 1.8.0.15

taking for distros