Closed Bug 964078 Opened 6 years ago Closed 6 years ago

global-buffer-overflow (read) at CJKIdeographicToText

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: aki.helin, Assigned: mats)

References

Details

(5 keywords, Whiteboard: [asan])

Attachments

(3 files)

Attached file 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.
Attached patch fixSplinter Review
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.
Assignee: nobody → matspal
Attachment #8366228 - Flags: review?(roc)
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.
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [asan]
Keywords: checkin-needed
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.
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1a7b4c02f2f8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Please nominate for Aurora uplift, Mats :)
Flags: needinfo?(matspal)
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:
Attachment #8366228 - Flags: approval-mozilla-aurora?
Flags: needinfo?(matspal)
... low risk, no UUID changes.
Attachment #8366228 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/41a58e12473f

So I guess the test is good to land since all affected branches are fixed?
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...
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.
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.
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!
Depends on: 1135426
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.
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.
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.