Closed Bug 1018234 (CVE-2014-1551) Opened 5 years ago Closed 5 years ago

Crash in ~FontTableRec() when called by :~gfxMathTable()

Categories

(Core :: MathML, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

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

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [reporter-external][adv-main31+][adv-esr24.7+])

Attachments

(2 files, 6 obsolete files)

Attached patch testcase.diff (obsolete) — Splinter Review
Apply the attached patch and run the MathML reftests.  The crash occurs sometime after the added reftest, when the test specific fonts expire.

The crash is similar to that recorded in bug 961365, but it appears different stuff happens during the call to ReleaseFontTable()
@Jonathan: any idea?
Flags: needinfo?(jfkthame)
Attached patch testcase (obsolete) — Splinter Review
Oops, wrong testcase attached.

https://tbpl.mozilla.org/?tree=Try&rev=26553ed6bf70
Try run.  

Unfortunately the actual crash occurs in closed source DirectWrite code, which I am unable to debug.

scripts-3 has a different mFontFace to the stretchy font loaded in the previous test (both reference the same file).  The crash occurs at the first call to ~FontTableRec, which is deconstructing the same object as that created by gfxDWriteFontEntry::GetFontTable(HB_TAG('M','A','T','H')).   

(The pointers to mFontFace are identical, but I don't have the symbols to determine whether there are any internal changes to mFontFace between construction and destruction).  mContext is unchanged and appears to always be 0x0.
Attachment #8431592 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Within gfxDWriteFontEntry mFontFace is reference counted, yet it is passed to FontTableRecord which has no reference counting and seems to live independently of the gfxDWriteFontEntry which created it.  

Stepping through with a debugger, ~gfxDWriteFontEntry() occurs before ~FontTableRec() so it is possible that mFontFace is disposed of before FontTableRec is finished with it.

The attached patch seems to fix the crash for me, however I suggest you look closely to verify that it does what it is supposed to.
Attachment #8432315 - Attachment is obsolete: true
Attachment #8432330 - Flags: review?(jfkthame)
I suspect this to be a use-after-free, but I do not know if it is exploitable.
Group: core-security
Comment on attachment 8432330 [details] [diff] [review]
patch

Review of attachment 8432330 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks James. Can you move the crash test to layout/mathml/crashtests/?

Also, it seems that you didn't include any changes to the code.
Attachment #8432330 - Flags: feedback+
Attached patch patch (obsolete) — Splinter Review
It would help if I could attach the correct patch.
Attachment #8432330 - Attachment is obsolete: true
Attachment #8432330 - Flags: review?(jfkthame)
Attachment #8432334 - Flags: review?(jfkthame)
Flags: needinfo?(jfkthame)
Attached patch patch (obsolete) — Splinter Review
Test is now a crashtest.

See comment 3.

Please double check that I got the reference counting correct.
Attachment #8432334 - Attachment is obsolete: true
Attachment #8432334 - Flags: review?(jfkthame)
Attachment #8432349 - Flags: review?(jfkthame)
Attached patch patch (obsolete) — Splinter Review
This now includes the problematic font
Attachment #8432349 - Attachment is obsolete: true
Attachment #8432349 - Flags: review?(jfkthame)
Attachment #8432350 - Flags: review?(jfkthame)
Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

The font still seems to be missing in this patch.

I let Jonathan check the refcounted change as I'm not familiar with this part of the code.

::: layout/mathml/crashtests/1018234-1.html
@@ +37,5 @@
> +
> +    <p>
> +      <math style="font-family: scripts-3;">
> +        <msqrt>
> +        <mspace/ width="5em">

the slash should be at then end of the tag.
I also wonder whether the crash is only reproducible with a MATH font or if it could already happen with some testcase calling the SVGGlyphs code instead... That is if we need to take this patch for Gecko >= 31 only or for older versions too...
https://crash-stats.mozilla.com/report/index/0b79c1a3-4a46-46cc-91e9-2e9452140203

Crash from Firefox 26 with a very similar stack signature (apart from not the gfxMathTable destructor).  The code in question dates back to Fx24.
Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

It's clear the problem here is a bug in the gfxDWrite font backend's management of the font resources and tables, but I'm not yet sure whether this is the correct fix; I'm concerned it might be setting up a circular reference that will mean certain resources are never freed. I'll try to do some tracing and code-reading this afternoon to figure out what's going on and whether this is the right place to fix it.

Leaving r? pending for now, while I poke around...

::: layout/mathml/crashtests/1018234-1.html
@@ +16,5 @@
> +        src: url(./stretchy.otf);
> +      }
> +    </style>
> +    <script type="text/javascript">
> +    

nit: trailing whitespace

@@ +27,5 @@
> +
> +    <p>
> +      <math style="font-family: scripts-1;">
> +        <msub>
> +          <mspace height="2em" width="2em" mathbackground="blue"/> 

and here

::: layout/mathml/crashtests/crashtests.list
@@ +1,1 @@
> +load 1018234-1.html

I think it'd be preferable to put this at the end of the list - i.e. let's use numerical order rather than lexical.
(And yes, it's very likely the same issue could affect the SVGGlyphs code, if the same SVG-in-OT font is instantiated multiple times simultaneously.)
Comment on attachment 8432350 [details] [diff] [review]
patch

Review of attachment 8432350 [details] [diff] [review]:
-----------------------------------------------------------------

OK, looks good to me - let's do it.
Attachment #8432350 - Flags: review?(jfkthame) → review+
Attached patch patchSplinter Review
Assignee: nobody → jkitch.bug
Status: NEW → ASSIGNED
Attachment #8432350 - Attachment is obsolete: true
No sec rating and affects multiple branches = needs security approval to land. Especially with Fx30 going to RC today.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Comment on attachment 8433299 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I believe this can lead to a use-after-free of a DirectWrite font-face object, but as the DW code is closed-source, it's not clear to me how readily that might translate to an exploit rather than merely a crash.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The actual code change doesn't offer much clue, IMO.

The testcase shows how to trigger a crash; whether/how that could be extended to a more serious exploit is unknown. Still, we might want to land only the code patch at first, and defer landing the crashtest.

(The crashtest here depends on MathML OpenType support landed for mozilla-31. I suspect, though, that equivalent examples could probably be constructed for older Gecko versions using non-MathML codepaths.)


Which older supported branches are affected by this flaw? All (on Windows only)


If not all supported branches, which bug introduced the flaw?


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same code change should apply to all; minimal risk.


How likely is this patch to cause regressions; how much testing does it need? Seems unlikely. The only potential issue would be that DirectWrite font-face objects may live a bit longer, but AFAICT the effect is not significant - and anyway, ensuring these objects are not prematurely deleted is the whole point of the patch!
Attachment #8433299 - Flags: sec-approval?
Sorry about that.

The attached patch is for versions before v31.  The other patch won't apply cleanly as the font it copies is not present.  This patch removes the test (which won't detect the problem on those versions anyway).

> I believe this can lead to a use-after-free of a DirectWrite font-face
> object, but as the DW code is closed-source, it's not clear to me how
> readily that might translate to an exploit rather than merely a crash.

mFontFace->ReleaseFontTable() is a virtual method, so I suppose someone could overwrite the vtable.  No idea how feasible that is.

https://crash-stats.mozilla.com/report/index/2d9e99ac-808f-4dc7-967b-d13432140131
Here's a crash with reason EXCEPTION_ILLEGAL_INSTRUCTION.  Examples can also be found for EXCEPTION_ACCESS_VIOLATION_READ/WRITE/EXEC.  I don't have the experience to draw any conclusions from that though.
We aren't going to be able to take this on 30. The release builds are either made or in process. So this needs to go to 31 and later. 

We need to figure out a security rating though. A UAF is usually critical but given the lack of confidence in what is actually going on, it isn't clear that is appropriate as a rating.
Calling a virtual function on a deleted object is usually sec-critical
(unless IDWriteFontFace objects at least safely poison their memory on delete and the memory is only ever reallocated for objects with a similar vtable)
http://hg.mozilla.org/releases/mozilla-release/annotate/39faf812aaec/gfx/thebes/gfxDWriteFontList.cpp#l350
bp-0b79c1a3-4a46-46cc-91e9-2e9452140203

(In reply to James Kitchener (:jkitch) from comment #16)
> Green try:

We usually avoid using try or other public media for security bugs, or if necessary obfuscate the patch with other changes and a different bug number, and test without crashtests.  Fortunately, the particular proof-of-concept in the crashtest is only effective on Aurora.
Keywords: sec-critical
Once again I apologise.
Flags: sec-bounty?
Whiteboard: [reporter-external]
Duplicate of this bug: 1020835
I'm giving sec-approal+ to check this in on June 24 or later (a couple of weeks into the next cycle).
Whiteboard: [reporter-external] → [reporter-external][checkin on 6/24 or later]
Attachment #8433299 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #24)
> I'm giving sec-approal+ to check this in on June 24 or later (a couple of
> weeks into the next cycle).

I think we should just land the fix, *not* the testcase, at that time.
Although the test was briefly exposed on Try, there's no reason to expose
it further.  The test should be landed at a later time when the bug
becomes public (as usual).
Ok. I agree with just landing the fix. We should hold the test until after release.
Flags: sec-bounty? → sec-bounty+
(In reply to Al Billings [:abillings] from comment #26)
> Ok. I agree with just landing the fix. We should hold the test until after
> release.

Should we upload a new patch for the fix only, or can attachment 8433352 [details] [diff] [review] be taken for the trunk?
Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

This should apply on trunk, as well as any branches where we decide to uplift this.
Attachment #8433352 - Attachment description: Patch for versions < 31 → code patch only (without test), for landing
Attachment #8433352 - Flags: review+
Pushed the fix to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb42fe85f9a
Target Milestone: --- → mozilla33
Whiteboard: [reporter-external][checkin on 6/24 or later] → [reporter-external]
https://hg.mozilla.org/mozilla-central/rev/cdb42fe85f9a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Please nominate this for Beta/Aurora/ESR24 uplift.
Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

Approval Request Comment

[Feature/regressing bug #]: longstanding bug in DirectWrite font code - may have been introduced by changes in bug 847344 (mozilla-24); I'm not sure if a similar flaw existed before then.

[User impact if declined]: Sporadic crash - generally occurs during deferred deletion of cached objects, so it'll appear rather random.

[Describe test coverage new/current, TBPL]: Existing unit tests exercise this font code heavily, but were not hitting the crash scenario, probably because it's too timing- and usage-pattern dependent. We now have a crashtest here, ready to land after the fix is pushed out. Crash-stats does not show any crashes with this signature since the fix landed on trunk, though the frequency was not really high enough on Nightly builds to make this definitive.

[Risks and why]: Low-risk patch - just adds a strong reference to avoid premature deletion of a DWrite object, no other change in behavior.

[String/UUID change made/needed]: none
Attachment #8433352 - Flags: approval-mozilla-esr24?
Attachment #8433352 - Flags: approval-mozilla-beta?
Attachment #8433352 - Flags: approval-mozilla-aurora?
Comment on attachment 8433352 [details] [diff] [review]
code patch only (without test), for landing

As Lukas said, we want this patch to land in all branches.
Attachment #8433352 - Flags: approval-mozilla-esr24?
Attachment #8433352 - Flags: approval-mozilla-esr24+
Attachment #8433352 - Flags: approval-mozilla-beta?
Attachment #8433352 - Flags: approval-mozilla-beta+
Attachment #8433352 - Flags: approval-mozilla-aurora?
Attachment #8433352 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1021262
Duplicate of this bug: 1021157
Whiteboard: [reporter-external] → [reporter-external][adv-main31+][adv-esr24.7+]
Alias: CVE-2014-1551
I couldn't reproduce the original issue from comment #0, I attempted the following:

Built m-c fx using changeset e86b84998b18 (June 23th, 2014):
- applied the patch from comment # 1
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes
- applied the patch from comment # 2
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes

Built aruora using changeset f118b45daada (June 23th, 2014):
- applied the patch from comment # 1
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes
- applied the patch from comment # 2
- ran "./mach reftest layout/reftests/mathml/reftest.list" and couldn't reproduce any crashes

I've also tried running the patch(s) against the latest changesets without any luck. James, am I missing something in the way I'm trying to reproduce? If so, could you please let me know how I can reproduce this? Maybe listing the original changeset that produced this issue?
Flags: needinfo?(jkitch.bug)
Changeset e86b84998b18 crashes for me with the comment 2 testcase applied.

.mozconfig used:
ac_add_options --enable-debug
ac_add_options --disable-optimize

Opt builds are much faster, so a slightly different codepath is invoked (deletion is triggered by shutdown rather than by timeout). However crash reports indicate that the shutdown path can also cause the crashes.

This crash only occurs when DirectWrite is used, so it is limited to Windows.  It does not occur for Windows XP, Vista without the platform update, or when hardware acceleration is disabled.  (I have only tested it with 32 bit builds on 64 bit Windows 7 and 64 bit 8.1, so the requirements may be somewhat stricter).
Flags: needinfo?(jkitch.bug)
Adding the in-testsuite? flag to remind us to land the test
Flags: in-testsuite?
Using the information from comment #39, I reproduced the crash consistently:

- using changeset e86b84998b18 without the patch completed the entire mathml reftest without any crashes
- using changeset e86b84998b18 and the patch from comment #2, the reftest crashes consistently (tried 5 different times with the same crash occurring)

Went through the following verification:

- fx33 [changeset: 7075808c3306] -> Passed without issues
- fx32 [changeset: dfeac3d29b5e] -> Passed without issues
- fx31 [changeset: 6befadcaa685] -> Passed without issues
- fx esr24 [changeset: eced3be3a20e] -> Passed without issues
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa!]
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.