634556 - browser crash in gfxScriptItemizer::Next with broken stack related to unicode-bidi

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Aki Helin]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.98 Safari/534.13
Build Identifier: Firefox 4.0b11

Current Firefox 4.0 beta crashes when opening a page with the content "<style> p{ direction:rtl; unicode-bidi:bidi-override; } </style> <p>a if".

Reproducible: Always

Steps to Reproduce:
1. $ echo "<style> p{ direction:rtl; unicode-bidi:bidi-override; } </style> <p>a if" > repro.html
2. ...
3. $ firefox repro.html
Actual Results:  
Firefox is sorry it crashed.

Expected Results:  
Firefox should not need to be sorry it crashed.

The crash state has a broken frame pointer, which can be moved by changing the content of the page. This makes this look like a probably exploitable crash, so reporting as a critical security issue.

Tested with 4.0b10 and 4.0b11 on 64-bit Linux. Does not affect 3.x on the same machine.

From 4.0b10:
 Program received signal SIGSEGV, Segmentation fault.
 0xf78480c8 in ?? () from libxul.so
 (gdb) bt
 #0  0xf78480c8 in ?? () from libxul.so
 Cannot access memory at address 0x8
 (gdb) x/i $eip
 => 0xf78480c8:  movzwl 0x0(%ebp,%eax,2),%esi
 (gdb) p $ebp
 $1 = (void *) 0x4

Comment 1

[Aki Helin]

Crash report at:
  https://crash-stats.mozilla.com/report/index/bp-fbb7784c-8d80-4157-b251-bc27c2110216

Comment 2

[Simon Montagu :smontagu]

Attachment: crash stack

This is the stack I get on crash after opening data:text/html,<style>
p{ direction:rtl; unicode-bidi:bidi-override; } </style> <p>a if

Comment 3

[Simon Montagu :smontagu]

regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=95452499f3d6&tochange=11e328a49e0a

The crash only occurs with gfx.font_rendering.harfbuzz.level > 0

Maybe bug 617231?

Comment 4

[Simon Montagu :smontagu]

Attachment: Reporter's testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

When I crash things look like this:

#0  0x00007ffff6e97bd5 in gfxScriptItemizer::Next (this=0x7fffffff4540, aRunStart=
    @0x7fffffff46dc, aRunLimit=@0x7fffffff46d8, aRunScript=@0x7fffffff46d4)
    at ../../../mozilla/gfx/thebes/gfxScriptItemizer.cpp:262
262             ch = textPtr[scriptLimit];
(gdb) p scriptLimit
$1 = 0
(gdb) p textPtr
$2 = (const PRUnichar *) 0x4

The 0x4 comes from the fact that in TextRunWordCache::FinishTextRun source->GetTextUnicode() is null and sourceOffset is 2.  |source == aNewRun| in this case, because the deferred words don't point to a textrun.

aNewRun in this case has TEXT_IS_8BIT set, which is why GetTextUnicode returns null.

Should that bit not be set here?  Or should the code in the word cache, whatever it's doing, not assume that all textruns can hand out Unicode text?

Comment 6

[Karl Tomlinson (:karlt)]

TextRunWordCache::FinishTextRun should not be using GetTextUnicode() on a source with TEXT_IS_8BIT set.

Comment 7

[Simon Montagu :smontagu]

The crash also doesn't occur with different text, so this is clearly being triggered by the "if" forming a fi ligature when reversed.

Comment 8

[Karl Tomlinson (:karlt)]

There seems something else funny that gets us into this situation too, as wordStartsInsideLigature probably shouldn't be set.

Comment 9

[Simon Montagu :smontagu]

I think the text reversing *is* the something funny, because the first (logical) character in the word is the second character of the ligature.

Perhaps we just shouldn't let reversed LTR text form ligatures?

Comment 10

[Simon Montagu :smontagu]

Attachment: Limit hb_ensure_native_direction to reversed RTL text

Like so?

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 512955 [details] [diff] [review]
Limit hb_ensure_native_direction to reversed RTL text

No, that's ridiculous

Comment 12

[Karl Tomlinson (:karlt)]

I wonder how the font it told the text direction when the fi ligature is found.
i.e. do ligature look-ups use logical order + direction or just visual order?

Comment 13

[Karl Tomlinson (:karlt)]

It does seem that the issue is in HarfBuzz.

In gfxHarfBuzzShaper::SetGlyphsFromRun, for the (RTL) text "a if",

(gdb) p numGlyphs
$3 = 3

(gdb) p ginfo[0].cluster
$9 = 0
(gdb) p ginfo[1].cluster
$10 = 1
(gdb) p ginfo[2].cluster
$11 = 3

The expected value of ginfo[/* glyph number = */ 2].cluster is 2 to indicate that character 2 (base 0) "i" is the start of the cluster.

These values are interpreted to mean that glyph 1 (base 0) is a ligature for " i", and so wordStartsInsideLigature.

I assume we should not interpret hb_glyph_info_t differently for RTL runs, which leads me to believe that the hb_glyph_info_t have the wrong values.

Comment 14

[Johnny Stenback (:jst)]

Not blocking unless this turns out to be more critical than I think it is (looks like a null pointer deref, but needs more investigation).

Comment 15

[Daniel Veditz [:dveditz]]

The symptoms look like an unexploitable null-deref. is it coincidentally null? That is, could the attacker manipulate the value to be something else? If it's always null we can unhide the bug.

Comment 16

[Aki Helin]

At least in 4.0b11 over here ebp is 2n + 6, where n is the number of a's.

Comment 17

[Karl Tomlinson (:karlt)]

The sourceOffset (from NULL) can be as large as the textRun length.
I don't know our limit on textRun lengths, but I guess it's much bigger than 4096/2.

This is XP code so would affect all platforms.  (Possible differences would just be in whether the font selected had an "fi" ligature.)

Comment 18

[Karl Tomlinson (:karlt)]

There are two bugs here:

1) HarfBuzz is giving bad glyph-character associations.
   jfkthame is probably best to look into that.

2) gfxTextRun::FinishTextRun is incorrectly assuming that the text run is 16-bit.
   I can write a patch for that.

Comment 19

[Karl Tomlinson (:karlt)]

1.9.1 code has the same incorrect 16-bit assumption.

1.9.1 and 1.9.2 don't have HarfBuzz, but someone could construct a downloaded font so that platform shapers would form a ligature in such a way as to reproduce this.

We have been lucky that issue 2 has not shown up previously because 8-bit characters usually do not have unusual character-glyph associations.

Comment 20

[Karl Tomlinson (:karlt)]

This should only cause unexpected memory reads (not writes), and it would be hard to make use of the data.

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: don't assume 16-bit text when wordStartsInsideLigature

(Still working on a test that doesn't need the harfbuzz bug.)

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: downloaded font crashtest

Need to check whether the 1.9.2 crash is more serious.
(Haven't tried 1.9.1.)

https://crash-stats.mozilla.com/report/index/fc925852-ecc0-41e5-97a6-068512110217

> 0 	libxul.so 	nsCharSinkTraits<CalculateUTF8Size>::write 	nsUTF8Utils.h:669
> 1 	libxul.so 	CalculateUTF8Size& copy_string<nsReadingIterator<unsigned short>, CalculateUTF8Size> 	nsAlgorithm.h:93
> 2 	libxul.so 	AppendUTF16toUTF8 	nsReadableUtils.cpp:200
> 3 	libxul.so 	gfxPangoFontGroup::MakeTextRun 	gfxPangoFonts.cpp:2403
> 4 	libxul.so 	TextRunWordCache::FinishTextRun 	gfxTextRunWordCache.cpp:517
> 5 	libxul.so 	TextRunWordCache::MakeTextRun 	gfxTextRunWordCache.cpp:813

Comment 23

[Aki Helin]

I just had a new look and could only cause different harmless invalid reads with this. Comment 20 is likely right, making this a harmless DoS. I was a bit hasty to submit this, since 4.0 will probably be released soon and a controllable frame pointer usually means trouble. In this case it probably doesn't. I'll have a better look in the weekend and post here if there is any proof to the contrary.

Comment 24

[Asa Dotzler [:asa]]

If this does turn out to be a simple DOS, does that change the security status and should we remove it from the hardblocker list?

Comment 25

[Karl Tomlinson (:karlt)]

The invalid pointer is only used to pass as a "const PRUnichar *" parameter on m-c, 1.9.2 and 1.9.1, so I can't imagine anything beyond an invalid read on all branches.

Thank you for filing, Aki.  This is still something we should fix.

I don't know whether "direction:rtl; unicode-bidi:bidi-override;" is common enough that this should be a hardblocker, but it doesn't really matter as this can land.

Comment 26

[Karl Tomlinson (:karlt)]

Attachment: don't assume 16-bit text when wordStartsInsideLigature v1.1

This is a variation on the first patch that functions the same but code is
more clearly using the API safely.

When using gfxFontGroup::MakeTextRun(), the TEXT_IS_8BIT bit of the aFlags
parameter needs to match the particular MakeTextRun method selected.

In the previous patch source->GetFlags() was used to select the method, but
aNewRun->GetFlags() was passed as flags.
That was fine because source == aNewRun, because we don't put such words in
the cache, but that was not very clear.

I think it's clearer that we are passing appropriate an TEXT_IS_8BIT bit, if
we use |source| both times.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

sg:dos doesn't get hard blocking status, imo!

Comment 28

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/279792a400ff
http://hg.mozilla.org/mozilla-central/rev/35b6854b0159

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we open this?