Closed Bug 964078 Opened 6 years ago Closed 6 years ago
global-buffer-overflow (read) at CJKIdeographic
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.
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.
Attachment #8366228 - Flags: review?(roc) → review+
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.
Please nominate for Aurora uplift, Mats :)
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?
... 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!
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.
You need to log in before you can comment on or make changes to this bug.