Closed Bug 634556 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- -
status1.9.2 --- ?
status1.9.1 --- ?

People

(Reporter: aki.helin, Assigned: karlt)

Details

(Keywords: regression, Whiteboard: [sg:dos?][has patch])

Attachments

(5 files, 1 obsolete file)

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
Attached file 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
Assignee: nobody → smontagu
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Keywords: regression
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?
TextRunWordCache::FinishTextRun should not be using GetTextUnicode() on a source with TEXT_IS_8BIT set.
The crash also doesn't occur with different text, so this is clearly being triggered by the "if" forming a fi ligature when reversed.
There seems something else funny that gets us into this situation too, as wordStartsInsideLigature probably shouldn't be set.
Component: Layout → Layout: Text
QA Contact: layout → layout.fonts-and-text
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 on attachment 512955 [details] [diff] [review]
Limit hb_ensure_native_direction to reversed RTL text

No, that's ridiculous
Attachment #512955 - Flags: review?(jfkthame)
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?
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.
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).
blocking2.0: ? → -
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.
Whiteboard: [sg:dos?]
At least in 4.0b11 over here ebp is 2n + 6, where n is the number of a's.
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.)
OS: Linux → All
Whiteboard: [sg:dos?] → [sg:critical?]
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.
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Hardware: x86_64 → All
This should only cause unexpected memory reads (not writes), and it would be hard to make use of the data.
Whiteboard: [sg:critical?] → [sg:dos?]
(Still working on a test that doesn't need the harfbuzz bug.)
Attachment #513318 - Flags: review?(roc)
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
status2.0: --- → ?
Summary: browser crash with broken stack related to unicode-bidi → browser crash in gfxScriptItemizer::Next with broken stack related to unicode-bidi
Assignee: smontagu → karlt
blocking2.0: - → ?
status1.9.1: ? → ---
status1.9.2: ? → ---
status2.0: ? → ---
OS: All → Linux
Hardware: All → x86_64
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
status1.9.1: --- → ?
status1.9.2: --- → ?
OS: Linux → All
Hardware: x86_64 → All
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.
blocking2.0: ? → final+
Whiteboard: [sg:dos?] → [sg:dos?][hardblocker]
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?
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.
Whiteboard: [sg:dos?][hardblocker] → [sg:dos?][hardblocker][can land]
Attachment #513373 - Attachment is private: false
Whiteboard: [sg:dos?][hardblocker][can land] → [sg:dos?][hardblocker][can land][has patch]
Whiteboard: [sg:dos?][hardblocker][can land][has patch] → [sg:dos?][hardblocker][has patch]
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.
Attachment #514136 - Flags: review?(roc)
sg:dos doesn't get hard blocking status, imo!
blocking2.0: final+ → -
Whiteboard: [sg:dos?][hardblocker][has patch] → [sg:dos?][has patch]
Attachment #513318 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/279792a400ff
http://hg.mozilla.org/mozilla-central/rev/35b6854b0159
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Group: core-security
You need to log in before you can comment on or make changes to this bug.