Closed Bug 451617 Opened 11 years ago Closed 11 years ago

ConvertUTF8toUTF16 with incomplete multi-byte character sequence can cause overrun

Categories

(Core :: String, defect)

1.8 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smontagu, Assigned: smontagu)

References

Details

(Keywords: fixed1.8.1.17, Whiteboard: [sg:critical])

Attachments

(1 file)

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.
Bug 314465 fixed this on trunk.
Version: Trunk → 1.8 Branch
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
Attachment #334949 - Flags: superreview?(dbaron)
Attachment #334949 - Flags: review?(dbaron)
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 on attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence

r+sr=dbaron, although I agree with Benjamin's concerns.
Attachment #334949 - Flags: superreview?(dbaron)
Attachment #334949 - Flags: superreview+
Attachment #334949 - Flags: review?(dbaron)
Attachment #334949 - Flags: review+
(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...
Attachment #334949 - Flags: approval1.8.1.17?
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.
(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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17+
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.
Attachment #334949 - Flags: approval1.8.1.17? → approval1.8.1.17+
Checked in to 1.8 branch.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: fixed1.8.1.17
Resolution: --- → FIXED
Whiteboard: [sg:critical]
Is there a QA-verifiable fix here, or is it more like bullet-proofing?  Thanks!
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.
Flags: blocking1.8.0.15+
Group: core-security
ok,but how to download the attachment 327885 [details] ??
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)
Attachment #334949 - Flags: approval1.8.0.next+
You need to log in before you can comment on or make changes to this bug.