643649 - Incorrect length passed to RegEnumValueW for value name buffer size in gfxGDIFontList::GetFontSubstitutes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/file/a7346f028fd6/gfx/thebes/gfxGDIFontList.cpp#l629

lenAlias is passed to RegEnumValueW to specify the length of aliasName. RegEnumValueW expects the length in characters, not bytes AIUI, so using sizeof makes us say the buffer is twice as big as it actually is.

From MSDN http://msdn.microsoft.com/en-us/library/ms724865%28v=vs.85%29.aspx :
lpcchValueName [in, out]
    A pointer to a variable that specifies the size of the buffer pointed to by the lpValueName parameter, in characters.

Not sure if overrunning this buffer is an actual security problem so I'm marking it as one.

Comment 1

[Jonathan Kew [:jfkthame]]

The same issue is present in the gfxDWriteFontList.cpp version of GetFontSubstitutes, too.

To actually encounter an overrun here would require a the registry to contain a FontSubstitutes name of at least 512 characters, which seems pretty far-fetched; and if a potential attacker is able to insert such malicious entries into the registry, I suspect things are already in a bad state.

Nevertheless, this does look like a bug and we should definitely fix it!

Comment 2

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

On the assumption that no one's stuffing malicious font names into the registry we're probably OK. We don't save names we find in downloadable fonts, right? In general, though, having windows calls overwrite buffers is exploitable.

Comment 3

[(no longer active)]

A better solution would be switching to the nsIWindowsRegKey API.  Calling the registry APIs directly is a bad choice, because of bugs like this.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[(no longer active)]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=8aae8f26aae7

Comment 6

[(no longer active)]

Try run was successful.

Comment 7

[John Daggett (:jtd)]

Attachment: patch, use array length instead of sizeof

simple fix for my screwup

Comment 8

[John Daggett (:jtd)]

Attachment: process monitor font substitutes profiling, patch

Comment 9

[John Daggett (:jtd)]

Attachment: process monitor font substitutes profiling, trunk

(In reply to comment #3)
> A better solution would be switching to the nsIWindowsRegKey API.  Calling the
> registry APIs directly is a bad choice, because of bugs like this.

In fact, I would argue precisely the opposite because using an API like
nsIWindowsRegKey obscures how the underlying APIs function and wastes time
because the code isn't structured optimally.  If you look back at the
previous versions of this code you'll see that it was using
nsIWindowsRegKey. I rewrote the code to not use it because when profiling
registry calls on cold startup, the previous code was calling RegQueryValue
many more times than is necessary.  Your build reflects that inefficiency,
it takes 1.3ms on cold startup to run through this loop as opposed to 0.5ms
with the current trunk code.  The MSDN docs explicitly state "While using
RegEnumValue, an application should not call any registry functions that
might change the key being queried."

Attached are the Process Manager event logs for trunk vs. the try build
with nsIWindowsRegKey.  Note all the extra RegQueryValue calls in the try
build case.

Comment 10

[John Daggett (:jtd)]

(In reply to comment #2)
> On the assumption that no one's stuffing malicious font names into the registry
> we're probably OK. We don't save names we find in downloadable fonts, right? In
> general, though, having windows calls overwrite buffers is exploitable.

This code is reading from specific registry entries, downloadable fonts have no impact at all on this code.

Comment 11

[John Daggett (:jtd)]

> Attached are the Process Manager event logs...

s/Process Manager/Process Monitor/

http://technet.microsoft.com/en-us/sysinternals/bb896645

Comment 12

[(no longer active)]

Comment on attachment 522252 [details] [diff] [review]
Patch (v1)

OK, fair enough.

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 522317 [details] [diff] [review]
patch, use array length instead of sizeof

diff -U 8 would have been better :)

Comment 14

[John Daggett (:jtd)]

Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/5e69b22218e9

Comment 15

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 522317 [details] [diff] [review]
patch, use array length instead of sizeof

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Please land this for the Tumucumaque Macaw release.

Comment 16

[Jonathan Kew [:jfkthame]]

I pushed this to mozilla-2.0:
http://hg.mozilla.org/releases/mozilla-2.0/rev/087fc58e6622

Comment 17

[LegNeato]

Marking as unaffected for branches as the file doesn't exist and it doesn't look to appear elsewhere. Please double check.

Comment 18

[Jonathan Kew [:jfkthame]]

Correct; the equivalent code used to be in gfxWindowsPlatform.cpp, but this issue was not present in the old (1.9.2 and earlier) version. It was rewritten for 2.0 (see comment #9) and the bug dates from then.