Last Comment Bug 643649 - Incorrect length passed to RegEnumValueW for value name buffer size in gfxGDIFontList::GetFontSubstitutes
: Incorrect length passed to RegEnumValueW for value name buffer size in gfxGDI...
Status: RESOLVED FIXED
: [sg:moderate]
:
Product: Core
Classification: Components
Component: Graphics
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: John Daggett (:jtd)
: thebes
:
:
:
  Show dependency treegraph
 
Reported: 2011-03-21 17:35 PDT by Michael Wu [:mwu]
Modified: 2011-05-24 23:34 PDT (History)
6 users (show)
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  Macaw+
  .1-fixed
  ---
  unaffected
  ---
  unaffected


Attachments
Patch (v1) (5.32 KB, patch)
2011-03-27 15:16 PDT, Ehsan Akhgari [:ehsan]
jdaggett: review-
Details | Diff | Splinter Review
patch, use array length instead of sizeof (1.14 KB, patch)
2011-03-28 00:58 PDT, John Daggett (:jtd)
joe: review+
dveditz: approval2.0+
Details | Diff | Splinter Review
process monitor font substitutes profiling, patch (25.58 KB, text/plain)
2011-03-28 01:16 PDT, John Daggett (:jtd)
no flags Details
process monitor font substitutes profiling, trunk (9.72 KB, text/plain)
2011-03-28 01:17 PDT, John Daggett (:jtd)
no flags Details

Summon comment box

Description Michael Wu [:mwu] 2011-03-21 17:35:55 PDT
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 2011-03-22 00:43:29 PDT
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 2011-03-24 13:48:21 PDT
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 Ehsan Akhgari [:ehsan] 2011-03-27 14:51:02 PDT
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 Ehsan Akhgari [:ehsan] 2011-03-27 15:16:44 PDT
Created attachment 522252 [details] [review]
Patch (v1)
Comment 5 Ehsan Akhgari [:ehsan] 2011-03-27 15:28:54 PDT
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8aae8f26aae7
Comment 6 Ehsan Akhgari [:ehsan] 2011-03-27 21:25:49 PDT
Try run was successful.
Comment 7 John Daggett (:jtd) 2011-03-28 00:58:17 PDT
Created attachment 522317 [details] [review]
patch, use array length instead of sizeof

simple fix for my screwup
Comment 8 John Daggett (:jtd) 2011-03-28 01:16:14 PDT
Created attachment 522320 [details]
process monitor font substitutes profiling, patch
Comment 9 John Daggett (:jtd) 2011-03-28 01:17:24 PDT
Created attachment 522321 [details]
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) 2011-03-28 01:22:52 PDT
(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) 2011-03-28 01:26:24 PDT
> Attached are the Process Manager event logs...

s/Process Manager/Process Monitor/

http://technet.microsoft.com/en-us/sysinternals/bb896645
Comment 12 Ehsan Akhgari [:ehsan] 2011-03-28 12:42:38 PDT
Comment on attachment 522252 [details] [review]
Patch (v1)

OK, fair enough.
Comment 13 Joe Drew (:JOEDREW!) 2011-03-28 19:51:51 PDT
Comment on attachment 522317 [details] [review]
patch, use array length instead of sizeof

diff -U 8 would have been better :)
Comment 14 John Daggett (:jtd) 2011-03-28 20:12:48 PDT
Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/5e69b22218e9
Comment 15 Daniel Veditz 2011-04-08 10:53:45 PDT
Comment on attachment 522317 [details] [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 2011-04-08 13:19:24 PDT
I pushed this to mozilla-2.0:
http://hg.mozilla.org/releases/mozilla-2.0/rev/087fc58e6622
Comment 17 Christian Legnitto [:LegNeato] 2011-04-13 17:05:08 PDT
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 2011-04-13 23:59:21 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.