964078 - global-buffer-overflow (read) at CJKIdeographicToText

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aki Helin]

Attachment: ff-gbofr.html

ASan spots a global buffer overflow when the attached page is opened at least in current Aurora channel.

=================================================================
==4778==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f0f66261278 at pc 0x7f0f60bb30c9 bp 0x7fff0e4df210 sp 0x7fff0e4df208
READ of size 2 at 0x7f0f66261278 thread T0
    #0 0x7f0f60bb30c8 in CJKIdeographicToText(int, nsString&, CJKIdeographicData const&) /home/aki/src/mozilla-aurora/layout/generic/nsBulletFrame.cpp:937
    #1 0x7f0f60bb14a3 in nsBulletFrame::AppendCounterText(int, int, nsString&, bool&) /home/aki/src/mozilla-aurora/layout/generic/nsBulletFrame.cpp:1270
    #2 0x7f0f60acfbe8 in nsCounterUseNode::GetText(nsString&) /home/aki/src/mozilla-aurora/layout/base/nsCounterManager.cpp:89
    #3 0x7f0f60acf65f in nsCounterUseNode::InitTextFrame(nsGenConList*, nsIFrame*, nsIFrame*) /home/aki/src/mozilla-aurora/layout/base/nsCounterManager.cpp:29
    #4 0x7f0f60a6b874 in nsCSSFrameConstructor::ConstructTextFrame(nsCSSFrameConstructor::FrameConstructionData const*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsStyleContext*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:3188
    #5 0x7f0f60a7439b in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:5561
    #6 0x7f0f60a71a33 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:9193
    #7 0x7f0f60a6ce68 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:3571
    #8 0x7f0f60a7453c in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:5591
    #9 0x7f0f60a61563 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) /home/aki/src/mozilla-aurora/layout/base/nsCSSFrameConstructor.cpp:9193
[...]

Filing as an unlikely but potential security issue based on bug type.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

The passed in value is INT_MIN so we need an uint32_t to hold
the absolute value.  I looked through the other functions
that handles ordinals and they are OK.

Comment 2

[Mats Palmgren (inactive)]

Attachment: crashtest (land when bug is public)

Comment 3

[Mats Palmgren (inactive)]

This looks completely safe, we just read some static data ("8 bytes to the left
of global variable 'gDataKoreanHanjaFormal'" according to Asan) into the text
representation and then exit the loop.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7b4c02f2f8

Setting Firefox 28 status to affected based on comment 0. Guessing this would be a wontfix on any other branches, but we'll probably want it there at least.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a7b4c02f2f8

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate for Aurora uplift, Mats :)

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8366228 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

Comment 8

[Mats Palmgren (inactive)]

... low risk, no UUID changes.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41a58e12473f

So I guess the test is good to land since all affected branches are fixed?

Comment 10

[Mats Palmgren (inactive)]

Only if we're ready to make the bug public at the same time.
It's usually good practice to let these bugs remain hidden for a while,
in case we missed something in the analysis.  In this case we might
also want to check the tree for other occurrences of the pattern
"x = -x" that has the same problem.  Just a quick "grep" shows that
it's quite common...

Comment 11

[Aki Helin]

I assume the text representation isn't retrievable, since this is low-risk? This kind of bugs can be security risks if some builds happen to have pointers next to the global variable, because they are useful for defeating ASLR.

Comment 12

[Mats Palmgren (inactive)]

gDataKoreanHanjaFormal and other tables nearby are all "static const":
http://hg.mozilla.org/mozilla-central/annotate/8840c133115a/layout/generic/nsBulletFrame.cpp#l839
so they end up in read-only memory, at least in the local Linux build I checked:
00000000038a7010 l     O .data.rel.ro.local     0000000000000028   _ZL22gDataKoreanHanjaFormal

At the offending line 936:
http://hg.mozilla.org/mozilla-central/annotate/8840c133115a/layout/generic/nsBulletFrame.cpp#l936
'cur' is guaranteed to be in the 0 to -9 range (inclusive), for the attached test it's -8.
So we're reading a 2-byte value at _ZL22gDataKoreanHanjaFormal.digit[cur], which
is at the beginning of gDataKoreanHanjaFormal or end of the one before it,
gDataKoreanHanjaFormal, at least in my build.  We're guaranteed to exit the loop
after reading once (since 'ordinal' is negative).

The 2-byte value we read is renderer to screen as a number, so if we read say 42
you would see 42 on the screen, in some CJK font likely.  You can probably scrape
that number but I don't see how it's useful.  Even if some platform puts this data
in a read-write section together with other globals (which I doubt) you're still
only getting 2 bytes, and at most 10 different values for any input.

Comment 13

[Aki Helin]

Ok, so it's easily retrievable but bounded to useless static data and there's likely no chance of there being anything dangerous next to it. Thanks for checking!

Comment 14

[Mats Palmgren (inactive)]

I was just about to open this bug and land the test when I discovered that it triggers
an assertion in a trunk debug build:
###!!! ASSERTION: Only accept non-negative ordinal: 'aOrdinal >= 0', file /builds/slave/try-and-api-11-d-0000000000000/build/layout/style/CounterStyleManager.cpp, line 331

It appears bug 966166 part 5 reverted the fix here for some reason...
I filed bug 1135426 to sort this out.

Comment 15

[Mats Palmgren (inactive)]

The bug only occurs on the "Android 4.0 API11+" platform, due to bug 989718.
Xidorn is landing a wallpaper for now in bug 1135426.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d042516e3561

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d042516e3561