Closed Bug 1035438 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Graphics: Text, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected

People

(Reporter: gkw, Assigned: jfkthame)

References

(Blocks 1 open bug, )

Details

(Keywords: reproducible, sec-low, Whiteboard: [adv-main31+])

Attachments

(2 files)

Attached file 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.
Flags: needinfo?(jfkthame)
Flags: needinfo?(jdaggett)
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.
Flags: needinfo?(jfkthame)
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.
Attachment #8452165 - Flags: review?(jdaggett)
Attachment #8452165 - Flags: review?(jdaggett) → review+
Flags: needinfo?(jdaggett)
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.
Keywords: sec-criticalsec-low
https://hg.mozilla.org/integration/mozilla-inbound/rev/373ec3f226af
Assignee: nobody → jfkthame
Severity: critical → normal
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/373ec3f226af

Does this need to get to Aurora or Beta?
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jfkthame)
Resolution: --- → FIXED
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.
Flags: needinfo?(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.
Attachment #8452165 - Flags: approval-mozilla-beta?
Attachment #8452165 - Flags: approval-mozilla-aurora?
Is ESR affected?
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
Attachment #8452165 - Flags: approval-mozilla-beta?
Attachment #8452165 - Flags: approval-mozilla-beta+
Attachment #8452165 - Flags: approval-mozilla-aurora?
Attachment #8452165 - Flags: approval-mozilla-aurora+
(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.
Whiteboard: [adv-main31+]
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.
(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.
Status: RESOLVED → VERIFIED
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.