Last Comment Bug 443288 - (CVE-2008-0016) Investigate CVE 2008-0016: crash [@ nsACString_internal::SetLength]
(CVE-2008-0016)
: Investigate CVE 2008-0016: crash [@ nsACString_internal::SetLength]
Status: VERIFIED FIXED
[sg:critical?]
: testcase, verified1.8.1.17
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 451615 451613 451617
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-02 16:47 PDT by Brandon Sterne (:bsterne)
Modified: 2009-06-09 12:57 PDT (History)
16 users (show)
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Backtrace from branch crash (23.49 KB, text/plain)
2008-07-02 16:49 PDT, Brandon Sterne (:bsterne)
no flags Details
testcase crashes branch (815 bytes, text/html)
2008-07-02 16:49 PDT, Brandon Sterne (:bsterne)
no flags Details
Patch to nsIDNService::ConvertUTF8toACE (916 bytes, patch)
2008-08-18 14:29 PDT, Simon Montagu :smontagu
cbiesinger: review+
dveditz: superreview+
dveditz: approval1.8.1.17+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2008-07-02 16:47:07 PDT
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.
Comment 1 Brandon Sterne (:bsterne) 2008-07-02 16:49:18 PDT
Created attachment 327886 [details]
Backtrace from branch crash
Comment 2 Brandon Sterne (:bsterne) 2008-07-02 16:49:47 PDT
Created attachment 327888 [details]
testcase crashes branch
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-02 17:30:03 PDT
Maybe fixed by the patches to one of bug 394275, bug 395651, bug 377360?  Or maybe even bug 282083?
Comment 4 Al Billings [:abillings] 2008-07-02 17:37:03 PDT
I checked this with 2.0.0.16 and it cashes... :-)
Comment 5 Daniel Veditz [:dveditz] 2008-08-14 16:14:25 PDT
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 2008-08-15 08:15:13 PDT
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 2008-08-15 08:22:57 PDT
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] 2008-08-18 11:50:02 PDT
Simon: does this fall into your baliwick?
Comment 9 Simon Montagu :smontagu 2008-08-18 14:26:09 PDT
(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 2008-08-18 14:29:39 PDT
Created attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2008-08-19 02:20:16 PDT
Hm, but where does the invalid UTF-8 come from?
Comment 12 Justin Schuh 2008-08-19 05:52:26 PDT
(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 2008-08-21 12:51:59 PDT
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) 2008-08-21 14:09:38 PDT
(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 2008-08-21 14:14:07 PDT
Sorry, yes (as in the "Depends on" field)
Comment 16 Daniel Veditz [:dveditz] 2008-08-22 11:23:13 PDT
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) 2008-08-26 14:50:37 PDT
> 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) 2008-08-26 14:52:44 PDT
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) 2008-08-26 14:54:23 PDT
Dan said he might have time to check this in today since Simon is out.
Comment 20 Daniel Veditz [:dveditz] 2008-08-26 14:57:09 PDT
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] 2008-08-27 00:37:08 PDT
Fix checked into the 1.8 branch
Comment 22 Daniel Veditz [:dveditz] 2008-08-27 00:39:23 PDT
--> FIXED since this is a 1.8-only bug.
Comment 23 Justin Schuh 2008-08-27 14:19:46 PDT
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 2008-08-27 14:20:39 PDT
(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) 2008-08-27 14:47:15 PDT
(In reply to comment #24)

I added Justin to the 3 bugs he requested access to.
Comment 26 Simon Montagu :smontagu 2008-08-28 07:27:03 PDT
(In reply to comment #17)

> So why shouldn't we fix ParseURL instead?

s/instead/as well/ => bug 451613
Comment 27 Stephen Donner [:stephend] 2008-08-31 16:10:52 PDT
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 2008-09-18 02:12:12 PDT
Comment on attachment 334342 [details] [diff] [review]
Patch to nsIDNService::ConvertUTF8toACE

a=asac for 1.8.0.15

taking for distros

Note You need to log in before you can comment on or make changes to this bug.