Closed
Bug 643649
Opened 14 years ago
Closed 14 years ago
Incorrect length passed to RegEnumValueW for value name buffer size in gfxGDIFontList::GetFontSubstitutes
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | Macaw+ |
status2.0 | --- | .1-fixed |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: mwu, Assigned: jtd)
Details
(Whiteboard: [sg:moderate])
Attachments
(3 files, 1 obsolete file)
1.14 KB,
patch
|
joe
:
review+
dveditz
:
approval2.0+
|
Details | Diff | Splinter Review |
25.58 KB,
text/plain
|
Details | |
9.72 KB,
text/plain
|
Details |
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•14 years ago
|
||
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•14 years ago
|
||
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.
Whiteboard: [sg:moderate]
Comment 3•14 years ago
|
||
A better solution would be switching to the nsIWindowsRegKey API. Calling the registry APIs directly is a bad choice, because of bugs like this.
Assignee: nobody → ehsan
Comment 4•14 years ago
|
||
Attachment #522252 -
Flags: review?(joe)
Comment 6•14 years ago
|
||
Try run was successful.
Assignee | ||
Comment 7•14 years ago
|
||
simple fix for my screwup
Attachment #522317 -
Flags: review?(joe)
Assignee | ||
Comment 8•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #522320 -
Attachment description: process monitor font substitutes profiling, trunk → process monitor font substitutes profiling, patch
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Attachment #522252 -
Flags: review-
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Assignee | ||
Comment 11•14 years ago
|
||
> Attached are the Process Manager event logs... s/Process Manager/Process Monitor/ http://technet.microsoft.com/en-us/sysinternals/bb896645
Comment 12•14 years ago
|
||
Comment on attachment 522252 [details] [diff] [review] Patch (v1) OK, fair enough.
Attachment #522252 -
Attachment is obsolete: true
Attachment #522252 -
Flags: review?(joe)
Updated•14 years ago
|
Assignee: ehsan → jdaggett
Comment 13•14 years ago
|
||
Comment on attachment 522317 [details] [diff] [review] patch, use array length instead of sizeof diff -U 8 would have been better :)
Attachment #522317 -
Flags: review?(joe) → review+
Assignee | ||
Comment 14•14 years ago
|
||
Landed on trunk http://hg.mozilla.org/mozilla-central/rev/5e69b22218e9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
Comment 15•14 years ago
|
||
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.
Attachment #522317 -
Flags: approval2.0+
Comment 16•14 years ago
|
||
I pushed this to mozilla-2.0: http://hg.mozilla.org/releases/mozilla-2.0/rev/087fc58e6622
Comment 17•14 years ago
|
||
Marking as unaffected for branches as the file doesn't exist and it doesn't look to appear elsewhere. Please double check.
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Comment 18•14 years ago
|
||
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.
OS: Windows XP → All
Updated•13 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•