182751 - Mozilla accepsts malformed UTF-8 byte sequences

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Alexey Chernyak]

OS: Win2K
Build: 2002111904 (Trunk)

http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt

This URL contains UTF-8 decoder capability and stress test. Mozilla seem to fail
a maximum overlong sequences test 4.2.3 and all tests in section 5 - illegal
code positions.

The document also briefly explains security implications of accepting overlong
and malformed sequences.

You can use this tool for testing wellformedness and round trip of UTF-8:
http://www.macchiato.com/unicode/convert.html

Also the following bugs seem to be related:

Bug 86411
Bug 172701
Bug 174351

Comment 1

[sub9]

'Me Too' - Status update. Testing against the UTF-8 stress test with mozilla-1.3a
(Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021213), 4.2.3 looks
to be 'fixed' (at least, working), but 2.1.3 looks broken, as do 2.3.x and all
of the 5.x tests.

This is a pretty important security issue and needs to be fixed before the code
gets into an enormous installed base (we hope) who never upgrade...

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Over to Simon - we should run these tests and act on the results.

Comment 3

[Simon Montagu :smontagu]

Accepting.

Comment 4

[Simon Montagu :smontagu]

There are two problems: a typo at
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsUTF8ToUnicode.cpp#185

            // from Unicode 3.1, non-shortest form is illegal
            if(((2==mBytes) && (mUcs4 < 0x0080)) ||
               ((3==mBytes) && (mUcs4 < 0x0800)) ||
               ((4==mBytes) && (mUcs4 < 0x1000)) ||
//                                      ^^^^^^ -- should be 0x10000
                (5==mBytes) ||
                (6==mBytes))

and because this whole block is in the |else| of |if(mUcs4 >= 0x00010000)| at
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsUTF8ToUnicode.cpp#173
some cases are never getting tested.

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch

Fix the problems described in the last comment, add tests for UTF-16 encoded as
UTF-8 and other illegal characters, and general cleanup.

Comment 6

[Simon Montagu :smontagu]

Attachment: The same, diff -uw

Comment 7

[Charles Rosendahl]

plussing per edt triage 

Comment 8

[Simon Montagu :smontagu]

Attachment: New patch

Removed the test for U+FFFE and U+FFFF per bug 172701 comment 6, cleaned up a
bit more and expanded comments.

Comment 9

[Simon Montagu :smontagu]

Attachment: The last patch, diff -uw

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 115576 [details] [diff] [review]
The same, diff -uw

transferring review request

Comment 11

[Frank Tang]

Comment on attachment 116491 [details] [diff] [review]
The last patch, diff -uw

looks accroding to the new definitation unicode 3.2

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 116491 [details] [diff] [review]
The last patch, diff -uw

I don't claim to understand this Unicode checking code, so my sr is purely a
syntax/style-based sr. Please test with every testcase you can get your hands
on...

The only thing I could notice that needs fixing is the indentation,
inconsistent space around operators & tab usage. Please replace all tabs in the
file with 2 spaces, and make sure indentation still looks ok.

Comment 13

[Simon Montagu :smontagu]

The indentation appears wonky in the diff -uw because it's wonky in the original
file, but it looks all OK to me in my locak file and attachment 116490 [details] [diff] [review].

Comment 14

[Simon Montagu :smontagu]

I have been testing with
http://jazz.mcom.com/users/smontagu/publish/dynamicutf8test.html, which is a
version of the URL given here. I'm now looking for some more testcases to run
before I check in.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I can see even from the diff that there are tabs!

Comment 16

[Simon Montagu :smontagu]

Also tested by correlating http://www.iur.ut.ee/juridica/utf8-3.htm with the
table of legal UTF-8 sequences in http://www.unicode.org/reports/tr28/, and
looking at lots of utf-8 pages in different languages to see that they are still
displayed OK. I think we are good to go with this.

Comment 17

[Simon Montagu :smontagu]

Fix checked in.

Comment 18

[Daniel Veditz [:dveditz]]

Clearing security flag: fix was checked in months ago and there was never a
demonstrated exploit of the potential to abuse over-long UTF8 encodings.