Bug 1201183 (CVE-2015-7203)

Buffer overflow on OOM in DirectWriteFontInfo::LoadFontFamilyData

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: jtd)

Tracking

({csectype-bounds, sec-moderate})

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected, firefox42 wontfix, firefox43 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [b2g-adv-main2.5-][post-critsmash-triage][adv-main43+])

Attachments

(1 attachment)

Reporter

Description

4 years ago
In bug 968520 q1@lastland.net posted:

(In reply to q1 from comment #137)
> There is now an array-overrun bug in DirectWriteFontInfo::LoadFontFamilyData
> (gfx/thebes/gfxDWriteFontList.cpp):
> 
>     // lookup the family
>      nsAutoTArray<wchar_t, 32> famName;
>  
>      uint32_t len = aFamilyName.Length();
> -    famName.SetLength(len + 1);
> +    famName.SetLength(len + 1, fallible);
>      memcpy(famName.Elements(), aFamilyName.BeginReading(), len *
> sizeof(char16_t));
>      famName[len] = 0;
> 
> SetLength used to be infallible, but the code now uses the fallible version
> without a status check. So on OOM the memcpy will overwrite some amount of
> unowned memory. Since this is Aurora (?) code, I've filed the bug here
> rather than in a separate report.

Thanks for the bug report, but please file security issues as security bugs that affect code we're shipping on any channel, including Nightly and Aurora.
Reporter

Updated

4 years ago
Keywords: csectype-bounds

Comment 1

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #0)
> In bug 968520 q1@lastland.net posted:
> 
> (In reply to q1 from comment #137)
> > There is now an array-overrun bug in DirectWriteFontInfo::LoadFontFamilyData
> > (gfx/thebes/gfxDWriteFontList.cpp):
> > 
> >     // lookup the family
> >      nsAutoTArray<wchar_t, 32> famName;
> >  
> >      uint32_t len = aFamilyName.Length();
> > -    famName.SetLength(len + 1);
> > +    famName.SetLength(len + 1, fallible);
> >      memcpy(famName.Elements(), aFamilyName.BeginReading(), len *
> > sizeof(char16_t));
> >      famName[len] = 0;
> > 
> > SetLength used to be infallible, but the code now uses the fallible version
> > without a status check. So on OOM the memcpy will overwrite some amount of
> > unowned memory. Since this is Aurora (?) code, I've filed the bug here
> > rather than in a separate report.
> 
> Thanks for the bug report, but please file security issues as security bugs
> that affect code we're shipping on any channel, including Nightly and Aurora.

I'm sorry about that, and will do as you ask. I didn't notice that that bug was public until after I posted my reply.
Reporter

Comment 2

4 years ago
(In reply to q1 from comment #1)
> I'm sorry about that, and will do as you ask. I didn't notice that that bug
> was public until after I posted my reply.

Its okay, it happens.

Nathan, could you look into this? Also, do you know why the MOZ_WARN_UNUSED_RESULT for this and other nsTArray methods is commented out?
Flags: needinfo?(nfroyd)
Reporter

Comment 3

4 years ago
I've started looking at enabling MOZ_WARN_UNUSED_RESULT for more nsTArray methods. Unfortunately, this particular file only gets built on Windows, and MOZ_WARN_UNUSED_RESULT doesn't do anything on MSVC, so that would not have caught this. Maybe the efforts to get clang working on Windows would have noticed this.
Group: core-security → gfx-core-security
(In reply to Andrew McCreight [:mccr8] from comment #2)
> (In reply to q1 from comment #1)
> > I'm sorry about that, and will do as you ask. I didn't notice that that bug
> > was public until after I posted my reply.
> 
> Its okay, it happens.
> 
> Nathan, could you look into this? Also, do you know why the
> MOZ_WARN_UNUSED_RESULT for this and other nsTArray methods is commented out?

It looks like Birunthan landed things this way in bug 968520 comment 76 due to "build failures".  The patches as reviewed had MOZ_WARN_UNUSED_RESULT uncommented, AFAICS.

So I guess it's not just this callsite, but all the other ones affected by that push (and others?) that call the MOZ_WARN_UNUSED_RESULT variants and don't check the result.

(In reply to Andrew McCreight [:mccr8] from comment #3)
> I've started looking at enabling MOZ_WARN_UNUSED_RESULT for more nsTArray
> methods. Unfortunately, this particular file only gets built on Windows, and
> MOZ_WARN_UNUSED_RESULT doesn't do anything on MSVC, so that would not have
> caught this. Maybe the efforts to get clang working on Windows would have
> noticed this.

I don't know whether clang-cl supports non-MSVC attributes on functions.  Perhaps it shoehorns them in via the C++11 attribute syntax.
Flags: needinfo?(nfroyd)
Reporter

Comment 5

4 years ago
John, can you look into this? Thanks.
Flags: needinfo?(jdaggett)
Reporter

Comment 6

4 years ago
I'm going to rate this moderate, because while it is a write, it is only one off the end, and I'm not sure how easy it would be to trigger an OOM and this particular point in code.
Keywords: sec-moderate

Comment 7

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #6)
> I'm going to rate this moderate, because while it is a write, it is only one
> off the end....

>     // lookup the family
>      nsAutoTArray<wchar_t, 32> famName;
>  
>      uint32_t len = aFamilyName.Length();

So |len| is from the source array |aFamilyName|.

The destination array |famName| has 0 length until:

> -    famName.SetLength(len + 1);
> +    famName.SetLength(len + 1, fallible);

which can fail, leaving it still at 0 length. Then

>      memcpy(famName.Elements(), aFamilyName.BeginReading(), len *
> sizeof(char16_t));

Copies all the source array's data into the destination array and then appends a 0 at the end:

>      famName[len] = 0;

This has the effect of writing the entire source array plus the appended 0 into the loaded Firefox image's data segment, immediately following &nsTArray_base<Alloc, Copy>::Header::sEmptyHdr.
Assignee

Comment 8

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #6)
> I'm going to rate this moderate, because while it is a write, it is only one
> off the end, and I'm not sure how easy it would be to trigger an OOM and
> this particular point in code.

This code is reading the name table from fonts. For downloadable fonts, we always sanitize the name table so any large length values likely to trigger an out-of-memory condition would be rejected at that stage.
Assignee: nobody → jdaggett
Flags: needinfo?(jdaggett)
Assignee

Comment 9

4 years ago
(In reply to John Daggett (:jtd) from comment #8)
> (In reply to Andrew McCreight [:mccr8] from comment #6)
> > I'm going to rate this moderate, because while it is a write, it is only one
> > off the end, and I'm not sure how easy it would be to trigger an OOM and
> > this particular point in code.
> 
> This code is reading the name table from fonts. For downloadable fonts, we
> always sanitize the name table so any large length values likely to trigger
> an out-of-memory condition would be rejected at that stage.

Argh, should read the code more carefully. The 'familyName' parameter here is coming from the font family name of *installed* fonts, this method is never used for downloadable fonts. So you'd somehow need a malicious font installed on the platform for it to be even possible to hit an OOM here.
Assignee

Comment 10

4 years ago
Attachment #8659014 - Flags: review?(nfroyd)
Attachment #8659014 - Flags: review?(nfroyd) → review+

Comment 11

4 years ago
> Argh, should read the code more carefully. The 'familyName' parameter here
> is coming from the font family name of *installed* fonts, this method is
> never used for downloadable fonts. So you'd somehow need a malicious font
> installed on the platform for it to be even possible to hit an OOM here.

If FF is nearly OOM for some other reason (e.g., a malicious page), OOM could be hit here.
https://hg.mozilla.org/mozilla-central/rev/e1bcc04808cc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Group: gfx-core-security → core-security-release
Whiteboard: [b2g-adv-main2.5-]
Whiteboard: [b2g-adv-main2.5-] → [b2g-adv-main2.5-][post-critsmash-triage]
Whiteboard: [b2g-adv-main2.5-][post-critsmash-triage] → [b2g-adv-main2.5-][post-critsmash-triage][adv-main43+]
Alias: CVE-2015-7203
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.