Last Comment Bug 451617 - ConvertUTF8toUTF16 with incomplete multi-byte character sequence can cause overrun
: ConvertUTF8toUTF16 with incomplete multi-byte character sequence can cause ov...
Status: RESOLVED FIXED
[sg:critical]
: fixed1.8.1.17
Product: Core
Classification: Components
Component: String (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks: CVE-2008-0016
  Show dependency treegraph
 
Reported: 2008-08-21 12:48 PDT by Simon Montagu :smontagu
Modified: 2009-06-09 12:51 PDT (History)
12 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
Check for end of input in mid-sequence (1.24 KB, patch)
2008-08-21 13:51 PDT, Simon Montagu :smontagu
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.1.17+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Simon Montagu :smontagu 2008-08-21 12:48:56 PDT
Spun off from bug 443288 (see also bug 451613 and bug 451615)

Description from attachment 327885 [details]:

The invalid URL segment is eventually passed to ConvertUTF8toUTF16::write. This method copies the string in a loop, checking to see if the end is reached. However, the loop condition does not properly account for the scenario where the loop body passes the end of the string, which occurs when the source string ends inside a multi-byte character sequence. 

xpcom\string\public\nsUTF8Utils.h:88-90
        for ( ; p != end /* && *p */; )
          {
            char c = *p++;

This results in an unbounded copy into the previously allocated stack buffer.
Comment 1 Simon Montagu :smontagu 2008-08-21 13:26:23 PDT
Bug 314465 fixed this on trunk.
Comment 2 Simon Montagu :smontagu 2008-08-21 13:51:08 PDT
Created attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence
Comment 3 Benjamin Smedberg [:bsmedberg] 2008-08-22 06:10:14 PDT
Although this patch in particular seems ok, I'm worried we're approaching this a bit whack-a-mole.

It is a programming error to pass invalid UTF8/UTF16 to these conversion functions. We should be validating the possibly-invalid UTF8/UTF16 when it crosses some untrusted/trusted boundary.

Do we know where that boundary is? Can we perhaps use different string classes or annotations to statically enforce the boundary?
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-08-22 06:27:19 PDT
Comment on attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence

r+sr=dbaron, although I agree with Benjamin's concerns.
Comment 5 Simon Montagu :smontagu 2008-08-22 06:40:25 PDT
(In reply to comment #3)
> It is a programming error to pass invalid UTF8/UTF16 to these conversion
> functions. We should be validating the possibly-invalid UTF8/UTF16 when it
> crosses some untrusted/trusted boundary.

That's what I tried to do in attachment 334342 [details] [diff] [review] in bug 443288...
Comment 6 Benjamin Smedberg [:bsmedberg] 2008-08-22 07:13:41 PDT
Yes, I'm in particular trying to describe a programming practice where the boundary is explicit: where we use typedefs

typedef nsACString nsAUTF8String;
typedef nsCString nsUTF8String;
etc...

to mean "this string is guaranteed to be UTF8". And then use a static checker to enforce that you cannot do a CString->UTF8String assignment without verifying that the string is indeed UTF8.

We can spin this conversation off into a separate/public bug if you like.
Comment 7 Daniel Veditz [:dveditz] 2008-08-22 11:30:06 PDT
(In reply to comment #6)
> We can spin this conversation off into a separate/public bug if you like.

Please do, in the nearer term we need to patch the holes on the 1.8 branch without waiting for a better future.
Comment 8 Daniel Veditz [:dveditz] 2008-08-22 11:30:28 PDT
Comment on attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence

Approved for 1.8.1.17, a=dveditz for release-drivers.
Comment 9 Simon Montagu :smontagu 2008-08-23 13:55:47 PDT
Checked in to 1.8 branch.
Comment 10 Stephen Donner [:stephend] 2008-09-02 15:19:02 PDT
Is there a QA-verifiable fix here, or is it more like bullet-proofing?  Thanks!
Comment 11 Simon Montagu :smontagu 2008-09-03 00:06:54 PDT
Bullet-proofing (or belt-and-braces). Either one of the patches here and in bug 443288 would have been sufficient to fix the crash there, but with both of them checked in I don't know of a way to verify them independently.
Comment 12 dangguai27 2008-10-21 06:10:44 PDT
ok,but how to download the attachment 327885 [details] ??
Comment 13 Alexander Sack 2009-01-05 05:09:22 PST
Comment on attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence

a=asac for 1.8.0.next (distros already have this for a while)

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