Closed
Bug 241440
Opened 20 years ago
Closed 19 years ago
memory overflow in UTF8ToNewUnicode
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: wind.li, Assigned: caillon)
References
Details
(Keywords: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6, Whiteboard: [sg:fix])
Attachments
(3 files)
639 bytes,
patch
|
dbaron
:
review+
dveditz
:
superreview+
dveditz
:
approval-aviary1.0.1+
dveditz
:
approval1.7.6+
dveditz
:
approval1.8b+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
dbaron
:
review-
|
Details | Diff | Splinter Review |
850 bytes,
patch
|
dveditz
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113 in UTF8ToNewUnicode >copy_string(aSource.BeginReading(start), aSource.EndReading(end), > calculator); if aSource include none-UTF8 chachacter such as 0xFC 0xDF calculator.Length() == 0 >PRUnichar *result = NS_STATIC_CAST(PRUnichar*, > nsMemory::Alloc(sizeof(PRUnichar) * (calculator.Length() + 1))); length of result = 2 > > ConvertUTF8toUTF16 converter(result); > copy_string(aSource.BeginReading(start), aSource.EndReading(end), converter).write_terminator(); if aSource is like "something(0xFC 0xDF)" result will be "something". Danger. A walk through is to remove the mLength = 0; in xpcom/string/public/nsUTF8Utils.h:256 Reproducible: Always Steps to Reproduce: 1. 2. 3.
why would non utf8 characters be in a class clearly labeled utf8?
Component: XPCOM → String
Comment 2•20 years ago
|
||
(In reply to comment #1) > why would non utf8 characters be in a class clearly labeled utf8? It happens ;-) See bug 236941 for a recent example.
Comment 3•20 years ago
|
||
that sounds more like a bug in the caller
Attachment #147104 -
Flags: review?(scc)
Comment 5•20 years ago
|
||
Confirming bug. Exploitability would depend on the ability of an attacker to get bad UTF8 into this section of code, but there are enough places where we parse UTF8 off the web that there's probably some easy ways. I'm not sure I like the proposed fix. Instead of returning a known error value (0) it would return a partial length. Then we have to hope ConvertUTF8toUTF16 fails no later than CalculateUTF8Length. It seems better for UTF8toNewUnicode to check for a zero length before allocating and deal with the error at that level.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:fix] heap overrun
Comment 6•20 years ago
|
||
I'd expect UTF-8 off the web to go to a different place, namely intl's nsUTF8ToUnicode...
Updated•20 years ago
|
Assignee: dougt → string
Updated•19 years ago
|
Flags: blocking1.8b?
Flags: blocking1.8b+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Updated•19 years ago
|
Flags: blocking-aviary1.0.1?
Comment 7•19 years ago
|
||
callion to try and work on this a bit. can use some help
Updated•19 years ago
|
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+
Updated•19 years ago
|
Assignee: string → caillon
Updated•19 years ago
|
Whiteboard: [sg:fix] heap overrun → [sg:fix] heap overrun - eta 2/14
Assignee | ||
Comment 8•19 years ago
|
||
Another fix for the issue, handling the zero-length given by the calculator. The question to ask is do we protect against this when we clearly state that UTF8ToNewUnicode takes a UTF-8 string? Sure, callers should get fixed, but we probably shouldn't stomp memory if they hand us garbage... I'm also wondering if the previously submitted patch (removing the mLength = 0 in the calculatr) is correct, since the converter will in fact write up to the invalid character and hand back a string of that length. It makes sense to have the converter and calculator match, I think, with their output length. I also took a look at ToNewUTF8String which I thought might have had the same issue, though I don't believe it does after reading through its respective Convert and Calculate classes.
Attachment #147104 -
Attachment is obsolete: true
Attachment #174183 -
Flags: review?(dbaron)
Updated•19 years ago
|
Whiteboard: [sg:fix] heap overrun - eta 2/14 → [sg:fix] heap overrun - eta 2/14 [need review dbaron]
Comment on attachment 147104 [details] [diff] [review] remove mLength = 0; The converter and the calculator should definitely match. It looks to me like this makes them do so, so r=dbaron on this patch. It would probably be good to add comments to the header of all 4 classes (the two converters and the two calculators) about that (i.e., that ConvertUTF8toUTF16 and CalculateUTF8Length should match and that ConvertUTF16toUTF8 and CalculateUTF8Size should match).
Attachment #147104 -
Attachment is obsolete: false
Attachment #147104 -
Flags: superreview?(darin)
Attachment #147104 -
Flags: review?(scc)
Attachment #147104 -
Flags: review+
Comment on attachment 174183 [details] [diff] [review] Fix attempt I prefer the other approach.
Attachment #174183 -
Flags: review?(dbaron) → review-
Comment 11•19 years ago
|
||
Comment on attachment 147104 [details] [diff] [review] remove mLength = 0; sr=dveditz
Attachment #147104 -
Flags: superreview?(darin) → superreview+
Comment 12•19 years ago
|
||
Comment on attachment 147104 [details] [diff] [review] remove mLength = 0; a=dveditz for landing everywhere
Attachment #147104 -
Flags: approval1.8b+
Attachment #147104 -
Flags: approval1.7.6+
Attachment #147104 -
Flags: approval-aviary1.0.1+
Updated•19 years ago
|
Whiteboard: [sg:fix] heap overrun - eta 2/14 [need review dbaron] → [sg:fix] need checkin
Comment 13•19 years ago
|
||
If I'm not mistaken, the convertor and calculator will still not match in the case of an incomplete multi-byte sequence (e.g. 110xxxxx not followed by 10xxxxxx, or 1110xxxx not followed by two of 10xxxxxx, etc.). If by "match" you just mean the convertor should never write more than the length returned by the calculator, that's fine.
Comment 14•19 years ago
|
||
(In reply to comment #9) > (From update of attachment 147104 [details] [diff] [review] [edit]) > The converter and the calculator should definitely match. It looks to me like > this makes them do so, so r=dbaron on this patch. It would probably be good to > add comments to the header of all 4 classes (the two converters and the two > calculators) about that (i.e., that ConvertUTF8toUTF16 and CalculateUTF8Length > should match and that ConvertUTF16toUTF8 and CalculateUTF8Size should match). It would also be good to have comments explaining which errors the converter considers fatal and which it considers recoverable by emitting a REPLACEMENT CHARACTER, and why. I assume that the distinction is between byte sequences that look like they are in another encoding (fatal) and byte sequences that look like UTF-8 with errors (recoverable), but that's only a guess.
Actually, though, there's another case where the computed length could be less -- which is if we actually get a 5-byte or 6-byte UTF-8 sequence. It looks like we'll attempt to fit it into a surrogate pair even though it doesn't fit.
Er, actually, never mind. We do check for that case.
Fix checked in to trunk, 2005-02-17 12:17 -0800. Fix checked in to MOZILLA_1_7_BRANCH, 2005-02-17 12:24 -0800. Fix checked in to AVIARY_1_0_1_20050124_BRANCH, 2005-02-17 12:29 -0800. Thanks for the patch.
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed-aviary1.0.1,
fixed1.7.6
Resolution: --- → FIXED
Whiteboard: [sg:fix] need checkin → [sg:fix]
Target Milestone: --- → mozilla1.8beta1
Comment 18•19 years ago
|
||
Does anyone have a testcase we can use to verify this fix?
Updated•19 years ago
|
Group: security
Comment 19•19 years ago
|
||
Attachment #187760 -
Flags: superreview?(dveditz)
Comment 20•19 years ago
|
||
Comment on attachment 187760 [details] [diff] [review] Patch for 1.4 branch sr=dveditz
Attachment #187760 -
Flags: superreview?(dveditz) → superreview+
Comment 21•19 years ago
|
||
Checking in nsUTF8Utils.h; /cvsroot/mozilla/xpcom/string/public/nsUTF8Utils.h,v <-- nsUTF8Utils.h new revision: 1.3.2.1; previous revision: 1.3 done
Keywords: fixed1.4.5
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•