1035438 - AddressSanitizer: stack-buffer-overflow with gfxUserFontSet::OTSMessage on the stack

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

+++ This bug was initially created as a clone of Bug #1030667 +++

+++ This bug was initially created as a clone of Bug #1023758 +++

+++ This bug was initially created as a clone of Bug #1021612 +++

I tested Steven's asan build ( http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg ) from Jul 02 (bug 1030667 comment 30) and when I went to http://spectrum.ieee.org/static/interactive-the-top-programming-languages , the asan build crashed with a stack buffer overflow with gfxUserFontSet::OTSMessage on the stack.

Steven Michaud could reproduce this on 10.8.5 and I reproduced this on 10.9.4. He advises that this might be a font issue, so cc'ing John Daggett and Jonathan Kew.

Setting s-s and sec-critical until we know what is going on. Please feel free to re-rate this as necessary. Setting needinfo? from John and Jonathan.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

By the way, someone should test with a Linux ASan build:
https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer#Public_Builds

Comment 2

[Jonathan Kew [:jfkthame]]

Looks like this is an OTS bug; it (consistently) has things like

    return OTS_FAILURE_MSG("Failed to read script header for table %4s", (char *)&tag);

where I believe it should have

    return OTS_FAILURE_MSG("Failed to read script header for table %4.4s", (char *)&tag);

to ensure that it doesn't attempt to write more than 4 chars for the (32-bit) tag.

I'll report this upstream, and work up a patch shortly.

I don't think this is too dangerous, AFAICS. It means we'll potentially include some garbage bytes (whatever happens to be on the stack adjacent to the tag being printed) in the console messages OTS logs for downloadable font errors, but those messages are purely informational. We won't overrun the buffer where the message is being generated (thus trampling other stack contents), as that's done with vsnprintf, passing sizeof(buf) as a safe length limit.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: ensure tags are limited to 4 chars in OTS error messages.

This should prevent the ASan errors due to trying to read tags as strings. Also submitted as PR to upstream: https://github.com/khaledhosny/ots/pull/30.

Comment 4

[Jonathan Kew [:jfkthame]]

Downgrading this to sec-low, as I don't think anything sinister can actually be done here... just possibly, it might lead to a crash if we try to read an inaccessible location, but even that seems unlikely; in practice, the only effect I expect is a garbage byte or two within the text of a console-log message.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/373ec3f226af

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/373ec3f226af

Does this need to get to Aurora or Beta?

Comment 7

[Jonathan Kew [:jfkthame]]

I don't think it's essential, since AFAICS there's nothing really dangerous here; but OTOH it's clearly a code bug with a simple and safe fix, and cleaning this up will improve things for anyone who tries to run ASan builds.

So I'll nominate the patch, and see what drivers decide.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8452165 [details] [diff] [review]
ensure tags are limited to 4 chars in OTS error messages.

Approval Request Comment
[Feature/regressing bug #]:
Bug in upstream OTS (now fixed there).

[User impact if declined]:
Minor: potential garbage character(s) in certain console log warnings for downloadable fonts.
Possible crash for developers/testers who run ASan or similar strict-memory-checking tools; risk of a crash in a standard release build is probably minimal.

[Describe test coverage new/current, TBPL]:
Landed in m-c; tested locally. We don't have specific unit tests for the precise content of the warning messages; that would be a pain to maintain in the face of upstream changes.

[Risks and why]: 
No risk, just enforces correct field width in printf strings.

[String/UUID change made/needed]:
None.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Is ESR affected?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8452165 [details] [diff] [review]
ensure tags are limited to 4 chars in OTS error messages.

Low risk and 31 is an ESR. Taking it

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Is ESR affected?

ESR24 is not affected; the bug arrived with the OTS update we took in bug 941019, which landed for mozilla-29.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/15c397ebfd14
https://hg.mozilla.org/releases/mozilla-beta/rev/edd37821774c

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

Not able to reproduce with smichaud's ASan build from comment 0 or with my own ASan build of Fx31 from 2014-07-01.

Gary, if you have the ability to confirm that it no longer crashes for you, that would be great. Otherwise, I will mark it [qa-] for verification purposes. Thank you.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Matt Wobensmith from comment #13)
> Not able to reproduce with smichaud's ASan build from comment 0 or with my
> own ASan build of Fx31 from 2014-07-01.

Matt, that's because Steven has updated his ASan build earlier today at the same address, so you're likely testing a recent build with the fix. (I'm not sure about your build though)

> 
> Gary, if you have the ability to confirm that it no longer crashes for you,
> that would be great. Otherwise, I will mark it [qa-] for verification
> purposes. Thank you.

Yes, I've confirmed that it no longer crashes for me.