Bug 443288 (CVE-2008-0016)

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

VERIFIED FIXED

Status

()

Core
XPCOM
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: bsterne, Assigned: smontagu)

Tracking

(Depends on: 1 bug, {testcase, verified1.8.1.17})

1.8 Branch
testcase, verified1.8.1.17
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
Created attachment 327885 [details]
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.
(Reporter)

Comment 1

9 years ago
Created attachment 327886 [details]
Backtrace from branch crash
(Reporter)

Comment 2

9 years ago
Created attachment 327888 [details]
testcase crashes branch
Maybe fixed by the patches to one of bug 394275, bug 395651, bug 377360?  Or maybe even bug 282083?
I checked this with 2.0.0.16 and it cashes... :-)
(Reporter)

Updated

9 years ago
Whiteboard: [sg:investigate]
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
Flags: blocking1.8.0.15?
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: [sg:investigate] → [sg:critical?]
Version: Trunk → 1.8 Branch

Comment 6

9 years ago
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

9 years ago
Sorry, by "I just checked the current code" I meant to say Firefox 3.0, which I guess is core 1.9.0.
Simon: does this fall into your baliwick?
Assignee: nobody → smontagu
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Comment 10

9 years ago
Created attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE
Attachment #334342 - Flags: superreview?(dveditz)
Attachment #334342 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #334342 - Flags: review? → review?(cbiesinger)
Hm, but where does the invalid UTF-8 come from?

Comment 12

9 years ago
(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.
Flags: blocking1.8.1.17? → blocking1.8.1.17+
Keywords: testcase
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.17, fixed1.9.0.2
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.17, fixed1.9.0.2
(Assignee)

Updated

9 years ago
Depends on: 451613
(Assignee)

Updated

9 years ago
Depends on: 451615
(Assignee)

Updated

9 years ago
Depends on: 451617
(Assignee)

Comment 13

9 years ago
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.
(Reporter)

Comment 14

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
Sorry, yes (as in the "Depends on" field)
Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

sr=dveditz
Attachment #334342 - Flags: superreview?(dveditz) → superreview+
Whiteboard: [sg:critical?] → [sg:critical?][needs r=biesi]
> 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 on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

I guess doing the same here as in Normalize isn't too bad
Attachment #334342 - Flags: review?(cbiesinger) → review+
Dan said he might have time to check this in today since Simon is out.
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs r=biesi] → [sg:critical?][needs checkin on 1.9.0]
Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

Approved for 1.8.1.17, a=dveditz for release-drivers.
Attachment #334342 - Flags: approval1.8.1.17+
Whiteboard: [sg:critical?][needs checkin on 1.9.0] → [sg:critical?][needs checkin on 1.8]
Fix checked into the 1.8 branch
Keywords: checkin-needed → fixed1.8.1.17
Whiteboard: [sg:critical?][needs checkin on 1.8] → [sg:critical?]
--> FIXED since this is a 1.8-only bug.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 23

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

Comment 24

9 years ago
(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.
(Reporter)

Comment 25

9 years ago
(In reply to comment #24)

I added Justin to the 3 bugs he requested access to.
(Assignee)

Comment 26

9 years ago
(In reply to comment #17)

> So why shouldn't we fix ParseURL instead?

s/instead/as well/ => bug 451613
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.17 → verified1.8.1.17

Updated

9 years ago
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Comment 28

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

a=asac for 1.8.0.15

taking for distros
Attachment #334342 - Flags: approval1.8.0.15+
Group: core-security
You need to log in before you can comment on or make changes to this bug.