279979 - Possible buffer overrun in nsFontMetricsPh::Init

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The sprintf into prop[256] in nsFontMetricsPh::Init seems like it could overrun
the size of the buffer. I'd suggest using a safer printf function from NSPR, or
using simple string join-operators.

Comment 1

[Daniel Veditz [:dveditz]]

Or failing that, max-width specifiers in the format string.

Comment 2

[cwiebe]

FYI Adrian is the guy you want - I have no involvement in Mozilla development. 
Reassigning.

Comment 3

[Benjamin Smedberg]

Why does this have [sg:fix]?

Comment 4

[Asa Dotzler [:asa]]

transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1

Comment 5

[Daniel Veditz [:dveditz]]

Is anyone ever going to fix this one? Maybe we should remove the group set so someone who cares about Photon can see it.

[sg:low] on the assumption that the strings involved come from installed fonts. If they're taken directly from a web page's styles without vetting against the installed list then this would be sg:critical.

Simple fix, either format width specifiers, snprintf, or the PR_ equivalent. Someone who can compile on photon would have to review/test. Or we could simply WONTFIX the bug for lack of interest.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Reopening, this bug has not been fixed. If the bug is invalid in that it's not exploitable please mark it INVALID

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

wha! sorry, my bad. 

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)
> wha! sorry, my bad. 
> 

did you mean to mark it fixed again?
 char *prop = PR_smprintf( "font.name.%s.%s", str, cstring ); 
is still there.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I didn't actually check that the new code was safe, i just put the bug back to the state it was in. Not sure if PR_smprintf is safe or not.

Adrian: please attach a patch to the bug when you fix it. This is why I didn't think the bug was fixed at first, because there was no activity in the bug. This is especially important for security patches.

Comment 10

[Daniel Veditz [:dveditz]]

PR_smprintf() mallocs sufficient space, it's fine. The original code used a plain sprintf() into a fixed-size buffer.