451617 - ConvertUTF8toUTF16 with incomplete multi-byte character sequence can cause overrun

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simon Montagu :smontagu]

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]

Bug 314465 fixed this on trunk.

Comment 2

[Simon Montagu :smontagu]

Attachment: Check for end of input in mid-sequence

Comment 3

[Benjamin Smedberg]

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-4, no longer working on Mozilla)]

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]

(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]

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]]

(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]]

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]

Checked in to 1.8 branch.

Comment 10

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

Is there a QA-verifiable fix here, or is it more like bullet-proofing?  Thanks!

Comment 11

[Simon Montagu :smontagu]

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]

ok,but how to download the attachment 327885 [details] ??

Comment 13

[Alexander Sack]

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)