1201183 - (CVE-2015-7203) Buffer overflow on OOM in DirectWriteFontInfo::LoadFontFamilyData

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew McCreight [:mccr8]]

In bug 968520 [e-mail] 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.

Comment 1

[q1]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> In bug 968520 [e-mail] 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.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Nathan Froyd [:froydnj]]

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

Comment 5

[Andrew McCreight [:mccr8]]

John, can you look into this? Thanks.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[q1]

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

Comment 8

[John Daggett (:jtd)]

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

Comment 9

[John Daggett (:jtd)]

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

Comment 10

[John Daggett (:jtd)]

Attachment: patch, handle font family name OOM error

Comment 11

[q1]

> 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.

Comment 12

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bcc04808cc

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1bcc04808cc