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

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: jfkthame)

Tracking

({reproducible, sec-low})

Trunk
mozilla33
x86_64
macOS
Points:
---

Firefox Tracking Flags

(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)

Details

(Whiteboard: [adv-main31+], )

Attachments

(2 attachments)

Reporter

Description

5 years ago
Posted 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)
Assignee

Comment 2

5 years ago
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)
Assignee

Comment 3

5 years ago
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)

Updated

5 years ago
Attachment #8452165 - Flags: review?(jdaggett) → review+
Flags: needinfo?(jdaggett)
Assignee

Comment 4

5 years ago
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
Assignee

Comment 5

5 years ago
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
Last Resolved: 5 years ago
Flags: needinfo?(jfkthame)
Resolution: --- → FIXED
Assignee

Comment 7

5 years ago
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)
Assignee

Comment 8

5 years ago
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+
Assignee

Comment 11

5 years ago
(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.
Reporter

Comment 14

5 years ago
(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

Updated

4 years ago
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.