600974 - crash (nsUTF8ToUnicode overruns buffer by 2 bytes outputting surrogate pair) on :first-letter punctuation test in CSS 2.1 test suite

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Loading the following test in the CSS 2.1 test suite (which is derived from a test I wrote):
crashes Firefox intermittently.

Steps to reproduce:
 1. load http://test.csswg.org/suites/css2.1/20100917/html4/first-letter-punct-before-036.htm
 2. reload the page a few times

Actual results: crash, generally after less than 10 reloads

I've tested that this occurs on both 4.0b6 and on the 2010-09-30-03-mozilla-central nightly, in both cases on 64-bit Linux.


In a debug build, loading the page reliably triggers the assertions:

###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file /home/dbaron/builds/mozilla-central/mozilla/parser/html/nsHtml5StreamParser.cpp, line 495

A buffer overrun causing a crash may well be exploitable (it's harder, but still possible, if it's a heap buffer overrun), so marking security-sensitive.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect this is related to incorrect handling of characters that are represented in UTF-16 as surrogate pairs, i.e., non-BMP characters.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, the bug may well be that nsUTF8ToUnicode::Convert will happily write one PRUnichar past the end of the buffer when it's outputting a surrogate pair.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

These steps to reproduce are specific to the HTML5 parser, which isn't on old branches, but the bug is on those old branches and it's likely possible to exploit it there.

I think it only allows overrunning the buffer by only two bytes, though.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I think this is probably the safest approach to fixing it.

I started to write a different approach on the assumption that nobody would be silly enough to repeatedly call convert with a 1 char output buffer... but then (a) again, it's possible that somebody could be and (b) fixing it by backing up introduces significant complexity that I wasn't confident I could get right.

This patch should apply all the way back to 1.9.0, with fuzz.  (I actually accidentally wrote it in my 1.9.0 tree while looking to see how far back the bug went.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I should probably also add a comment here:

+            if (out >= outend) {
+              mRemainingSurrogate = lowSurr;

noting that out >= outend means this is our last iteration through the loop, but we don't want to |break| because we need the assignments below.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, maybe this isn't the best approach, since it seems like the other decoders don't split surrogate pairs across output blocks.

Comment 8

[Simon Montagu :smontagu]

Crashtest?

Comment 9

[Simon Montagu :smontagu]

(In reply to comment #7)
> Actually, maybe this isn't the best approach, since it seems like the other
> decoders don't split surrogate pairs across output blocks.

I was wondering about that too. Do you think splitting the surrogate pair will cause problems?

Comment 10

[Simon Montagu :smontagu]

FWIW, nsUTF32ToUnicode and nsGBKToUnicode seem to have the same bug.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsUTF32ToUnicode doesn't look like it overruns the buffer; it just drops a character on the floor, I think.

ucvlatin/nsUCS2BEToUnicode.cpp also has correctness bug when a surrogate pair
ought to cross a block boundary in the destination buffer (it outputs a replacement char instead of the surrogates it ought to)
nsGBKToUnicode::ConvertNoBuff does indeed look broken; I hadn't yet gotten to any of the ConvertNoBuff implementations, or for that matter even figured out how nsBufferDecoderSupport::Convert worked or who derived from it.

We probably ought to have a more general audit here...

(In reply to comment #9)
> I was wondering about that too. Do you think splitting the surrogate pair will
> cause problems?

It depends what the callers do with the result.  If they're just pasting the chunks back together then they should be fine.  But if they look into them before doing so, there could be problems.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to comment #9)
> (In reply to comment #7)
> > Actually, maybe this isn't the best approach, since it seems like the other
> > decoders don't split surrogate pairs across output blocks.
> 
> I was wondering about that too. Do you think splitting the surrogate pair will
> cause problems?

The decode loop in the HTML5 parser assumes that if the result is NS_PARTIAL_MORE_OUTPUT the decoder hasn't consumed all input bytes. This is wrong, of course, if the decoder consumed a 4-byte sequence but had room to write only the first surrogate.
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp#536
My bad. :-(

In any case, the decoder interface contract should be documented to cover this case so that users of the API know what to do.

If the decode loop in the HTML5 parser were fixed to take into account the case mentioned above, the worst that could happen with the high and low surrogates going into different output buffers would be a discretionary flush happening in the middle of a surrogate pair and the first surrogate got notified to layout before the second is available. However, if the decoder doesn't emit the high surrogate until it has consumed the entire 4-bytes UTF-8 sequence, AFAICT, there's no way for a discretionary flush to take place in the middle of a surrogate pair.

So the HTML5 parser should be OK once the decode loop itself has been fixed--assuming that the new API contract makes it possible to grab the second surrogate out of the decoder without pushing new bytes to it so that the parser can always obtain the complete surrogate pair inside the same runnable task that pushed the 4-byte sequence into the decoder.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 479989 [details] [diff] [review]
patch

From the comments sounds like we don't need a separate branch patch. We'll wait for trunk landing (or at least blocking) first.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #12)
> If the decode loop in the HTML5 parser were fixed to take into account the case
> mentioned above, the worst that could happen with the high and low surrogates
> going into different output buffers would be a discretionary flush happening in
> the middle of a surrogate pair and the first surrogate got notified to layout
> before the second is available.

I think we don't want that to happen.

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 479989 [details] [diff] [review]
patch

So the consensus seems to be that we don't want to do it this way. David, do you want to make a new patch, or would you rather punt to me (in view of what you said in comment 4)?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd be happy if you wrote the new patch.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a bunch of other bugs (bug 597849, bug 596874, bug 599600) at least some of the occurrences of which are probably this bug... and they're public.

Do you know when you'll have a chance to work on this?  Should I take it back?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are also public comments in bug 564008.

Comment 20

[Simon Montagu :smontagu]

This is about 90% done, but I got distracted by other bugs.

Comment 21

[Simon Montagu :smontagu]

Attachment: Patch

Comment 22

[Simon Montagu :smontagu]

Attachment: Test

Reftest rather than crashtest in order to test for correct decoding.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Did you forget to request a reviewer? This needs to get on someones review queue.

Comment 24

[Simon Montagu :smontagu]

(In reply to comment #23)
> Did you forget to request a reviewer? This needs to get on someones review
> queue.

Bug 372539 must die!

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 482587 [details] [diff] [review]
Patch

r=dbaron

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 482587 [details] [diff] [review]
Patch

I couldn't run reftests (even without patches), but the patch looks fine.

Comment 27

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/4c146ed860b3
http://hg.mozilla.org/mozilla-central/rev/3a24c22ddd4f

Keeping open for other decoders per comment 10 and 11.

I'm not so convinced that we need this on branches without STR, and if we do take it we should probably take fixes for other buffer overruns that were exposed by the HTML5 parser. I remember bug 563618, and I think there were others.

Comment 28

[Simon Montagu :smontagu]

Both UTF-32 and GB18030 trigger the assertions. Since HTML5 recommends against supporting UTF-32, maybe the time has come to remove it rather than invest time in fixing it. I opened bug 604317 on that.

Comment 29

[Henri Sivonen (:hsivonen)]

(In reply to comment #12)
> The decode loop in the HTML5 parser assumes that if the result is
> NS_PARTIAL_MORE_OUTPUT the decoder hasn't consumed all input bytes. This is
> wrong, of course, if the decoder consumed a 4-byte sequence but had room to
> write only the first surrogate.
> http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp#536
> My bad. :-(
> 
> In any case, the decoder interface contract should be documented to cover this
> case so that users of the API know what to do.

What's the new API contract? How should I change nsHtml5StreamParser.cpp?

Comment 30

[Masatoshi Kimura [:emk]]

Sayre, we need to fix the UTF-32 decoder unless we remove the encoder itself.

Comment 31

[Simon Montagu :smontagu]

Attachment: UTF-32 testcase

UTF-32 testcase (asserts and crashes)

Comment 32

[Simon Montagu :smontagu]

umm, actually the UTF-32 testcase has different assertions and doesn't seem to crash, so maybe it's not such high priority after all.

###!!! ASSERTION: The Unicode decoder consumed the wrong number of bytes.: 'totalByteCount == (PRInt32)aCount', file /home/smontagu/mozwork/hgtree/mozilla/parser/html/nsHtml5StreamParser.cpp, line 540
###!!! ASSERTION: Wrong number of stream bytes written/sniffed.: 'writeCount == aLength', file /home/smontagu/mozwork/hgtree/mozilla/parser/html/nsHtml5StreamParser.cpp, line 682

Comment 33

[Simon Montagu :smontagu]

(In reply to comment #29)
> What's the new API contract? How should I change nsHtml5StreamParser.cpp?

I don't think there's any change, except for the assertion that you linked to before.

   * Unless there is not enough output space, this method must consume all the
   * available input data! 

This doesn't necessarily imply the converse, i.e. that where there isn't enough output space, not all input data will be consumed. I'll add a sentence to clarify that.

Comment 34

[Simon Montagu :smontagu]

(In reply to comment #29)
> What's the new API contract? How should I change nsHtml5StreamParser.cpp?

The assertion 
    NS_ASSERTION(byteCount > 0 || NS_FAILED(convResult),
        "The decoder consumed too few bytes but did not signal an error.");
will also need to change to cover the case where the decoder returns NS_PARTIAL_MORE_OUTPUT at the very end of the input, and the caller has to call Convert() again with no input.

Comment 35

[Simon Montagu :smontagu]

Attachment: Patch for GB18030 and UTF-16

Comment 36

[Simon Montagu :smontagu]

Attachment: Tests for GB18030 and UTF-16

Comment 37

[Masatoshi Kimura [:emk]]

Comment on attachment 484264 [details] [diff] [review]
Patch for GB18030 and UTF-16

Please set a language and a font-family to the <html> element explicitly. Otherwise reftests will fail on non-English locales because those locales may select a non-Western or sans-serif font for UTF-8 documents. At least the tests don't pass on Japanese locale unless I add lang="en" style="font-family: serif" to the <html> element of 600974-1.html and 600974-3.html.

Otherwise looks good.

Comment 38

[Simon Montagu :smontagu]

(In reply to comment #37)
> At least the tests
> don't pass on Japanese locale unless I add lang="en" style="font-family: serif"
> to the <html> element of 600974-1.html and 600974-3.html.

I originally did this in 600974-2.html, because it didn't pass on en-US locale, but even with the language specified it still fails. I couldn't find a solution, so I ended up giving up and making 2 reference versions, 600974-1-ref.html and 600974-2-ref.html, which are identical except for the charset. Do you have any idea how to avoid this? I'm not sure if it's a bug in font selection or expected behaviour.

Comment 39

[Masatoshi Kimura [:emk]]

Did you also specify font-family: serif? On Japanese and Chinese locale, the default font-family is sans-serif which seems to affect a font selection for comma characters (U+002C). I could match 600974-2-ref.html rednering to 600974-1-ref.html by adding style="font-family: serif".

Comment 40

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/7c6637b28e51
http://hg.mozilla.org/mozilla-central/rev/a7f4a58793c0

font-family: serif wasn't enough, but with font-size as well I was able to remove the duplicate reference rendering.

Comment 41

[Simon Montagu :smontagu]

wrt branches, see comment 27.

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 482587 [details] [diff] [review]
Patch

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 484264 [details] [diff] [review]
Patch for GB18030 and UTF-16

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers

Comment 44

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e7fe73c04be2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e27a7abd54e2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1f9aed09ee37
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/270a9fcb9711

Comment 45

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5ec393c57c79
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e0f5ccd3934f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cf9f758ef187
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d4f4337cbe8f

Comment 46

[Al Billings [:abillings - ex-MoCo]]

Have we seen this crash on 1.9.2 or 1.9.1? Using 1.9.2.13 and 1.9.1.16 released builds on XP with either the original URL or the testcase attached, I don't get any crashes.

Comment 47

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 version

Comment 48

[Henri Sivonen (:hsivonen)]

(In reply to comment #34)
> (In reply to comment #29)
> > What's the new API contract? How should I change nsHtml5StreamParser.cpp?
> 
> The assertion 
>     NS_ASSERTION(byteCount > 0 || NS_FAILED(convResult),
>         "The decoder consumed too few bytes but did not signal an error.");
> will also need to change to cover the case where the decoder returns
> NS_PARTIAL_MORE_OUTPUT at the very end of the input, and the caller has to call
> Convert() again with no input.

Filed bug 634262 as a follow-up.