ConvertUTF8toUTF16 with incomplete multi-byte character sequence can cause overrun

RESOLVED FIXED

Status

()

Core
String
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

({fixed1.8.1.17})

1.8 Branch
fixed1.8.1.17
Points:
---
Bug Flags:
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Bug 314465 fixed this on trunk.
Version: Trunk → 1.8 Branch
(Assignee)

Comment 2

9 years ago
Created attachment 334949 [details] [diff] [review]
Check for end of input in mid-sequence
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+
(Assignee)

Comment 5

9 years ago
(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...
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 9

9 years ago
Checked in to 1.8 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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!
(Assignee)

Comment 11

9 years ago
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.

Updated

9 years ago
Flags: blocking1.8.0.15+
Group: core-security

Comment 12

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

Comment 13

9 years ago
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.