Bugzilla@Mozilla – Bug 643649
Incorrect length passed to RegEnumValueW for value name buffer size in gfxGDIFontList::GetFontSubstitutes
Last modified: 2011-05-24 23:34:37 PDT
Summon comment box
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.
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!
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.
A better solution would be switching to the nsIWindowsRegKey API. Calling the registry APIs directly is a bad choice, because of bugs like this.
Created attachment 522252 [details] [review] Patch (v1)
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8aae8f26aae7
Try run was successful.
Created attachment 522317 [details] [review] patch, use array length instead of sizeof simple fix for my screwup
Created attachment 522320 [details] process monitor font substitutes profiling, patch
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.
(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.
> Attached are the Process Manager event logs... s/Process Manager/Process Monitor/ http://technet.microsoft.com/en-us/sysinternals/bb896645
Comment on attachment 522252 [details] [review] Patch (v1) OK, fair enough.
Comment on attachment 522317 [details] [review] patch, use array length instead of sizeof diff -U 8 would have been better :)
Landed on trunk http://hg.mozilla.org/mozilla-central/rev/5e69b22218e9
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.
I pushed this to mozilla-2.0: http://hg.mozilla.org/releases/mozilla-2.0/rev/087fc58e6622
Marking as unaffected for branches as the file doesn't exist and it doesn't look to appear elsewhere. Please double check.
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.