Aki Helin Reporter
	Description • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 1 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 3 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 4 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 10 years ago

Please nominate for Aurora uplift, Mats :)

	Mats Palmgren (inactive) Assignee
	Comment 7 • 10 years ago

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:

	Mats Palmgren (inactive) Assignee
	Comment 8 • 10 years ago

... low risk, no UUID changes.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

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

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

	Mats Palmgren (inactive) Assignee
	Comment 10 • 10 years ago

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...

	Aki Helin Reporter
	Comment 11 • 10 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 12 • 10 years ago

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.

	Aki Helin Reporter
	Comment 13 • 10 years ago

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!

	Mats Palmgren (inactive) Assignee
	Comment 14 • 9 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 9 years ago

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.

	Pulsebot
	Comment 16 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago
bugherder

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