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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mwu, Assigned: jtd)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 Macaw+, status2.0 .1-fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

8 years ago
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.
Whiteboard: [sg:moderate]

Comment 3

8 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

8 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #522252 - Flags: review?(joe)

Comment 6

8 years ago
Try run was successful.
Assignee

Comment 7

8 years ago
simple fix for my screwup
Attachment #522317 - Flags: review?(joe)
Assignee

Updated

8 years ago
Attachment #522320 - Attachment description: process monitor font substitutes profiling, trunk → process monitor font substitutes profiling, patch
Assignee

Comment 9

8 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

8 years ago
Attachment #522252 - Flags: review-
Assignee

Comment 10

8 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

8 years ago
> Attached are the Process Manager event logs...

s/Process Manager/Process Monitor/

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

Comment 12

8 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

8 years ago
Assignee: ehsan → jdaggett
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

8 years ago
Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/5e69b22218e9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee

Updated

8 years ago
blocking2.0: --- → ?
blocking2.0: ? → Macaw+
status2.0: --- → wanted
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 17

8 years ago
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.
OS: Windows XP → All
Group: core-security
You need to log in before you can comment on or make changes to this bug.