Closed
Bug 1369560
Opened 7 years ago
Closed 7 years ago
address potentially unsafe snprintf usage in FPSCounter
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])
Attachments
(1 file)
3.20 KB,
patch
|
mchang
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
snprintf returns the number of bytes it would have written when it runs out of space (rather than the number of bytes it did write). FPSCounter::PrintHistogram appears to attempt to handle this, but a) the assertion is the opposite of what should be asserted and b) assertions are debug-only (as an aside, if the given histogram is empty, PrintHistogram will just print uninitialized memory). FPSCounter::WriteFrameTimeStamps doesn't properly check the return value either (SprintfLiteral uses vsnprintf, which behaves the same as snprintf in that regard).
Assignee | ||
Comment 1•7 years ago
|
||
I'm not sure if this is exactly the desired behavior, but it's a place to start.
Assignee: nobody → dkeeler
Attachment #8874029 -
Flags: review?(mchang)
Updated•7 years ago
|
status-firefox53:
--- → wontfix
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr45:
--- → wontfix
status-firefox-esr52:
--- → affected
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox55:
--- → ?
tracking-firefox-esr52:
--- → ?
Version: unspecified → 44 Branch
Comment 2•7 years ago
|
||
Comment on attachment 8874029 [details] [diff] [review] patch Review of attachment 8874029 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8874029 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Thanks! I'm marking this as sec-low since it requires changing/adding about:config preferences and since the original implementation is unlikely to overflow the buffers anyway.
Keywords: sec-low
Assignee | ||
Comment 4•7 years ago
|
||
Actually it looks like sec-moderate is more appropriate.
Keywords: sec-low → sec-moderate
Comment 5•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4) > Actually it looks like sec-moderate is more appropriate. I disagree since this is a developer debug tool and users shouldn't actually ever expect to use this.
Assignee | ||
Comment 8•7 years ago
|
||
(To be clear, I'm waiting for bug 1368652 to land before landing this.)
Updated•7 years ago
|
Group: core-security → gfx-core-security
For 55 we have several sec-high and sec-crits that are being tracked. This is a sec-mod that we can uplift to Beta55 if the fix is low-risk. Dropping 55 tracking.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•7 years ago
|
||
Actually, on second thought, it would be safest to wait until bug 1368652 actually ships (in 55).
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bug 1368652 has now shipped, is anything else blocking this one?
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 12•7 years ago
|
||
I think we're good to go here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764a29737584ac6902ba5d803f646f175dbcb43
Flags: needinfo?(dkeeler)
Keywords: checkin-needed
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b790f9291cdbad093bb033390f6724cfe8719ac4 Please request Beta approval on this when you get a chance - I've already verified that grafts cleanly. It actually does graft cleanly to ESR52 as well, but doesn't really seem worth the backport there? I'm setting the status to wontfix, but feel free to set it back to affected and request ESR52 approval as well if you feel otherwise.
status-firefox56:
--- → affected
status-firefox57:
--- → affected
tracking-firefox-esr52:
? → ---
Flags: needinfo?(dkeeler)
Keywords: checkin-needed
Comment 14•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > b790f9291cdbad093bb033390f6724cfe8719ac4 > > Please request Beta approval on this when you get a chance - I've already > verified that grafts cleanly. It actually does graft cleanly to ESR52 as > well, but doesn't really seem worth the backport there? I'm setting the > status to wontfix, but feel free to set it back to affected and request > ESR52 approval as well if you feel otherwise. I'm still not sure this needs 52 ESR or even is sec-moderate as this is a gecko developer debug flag. Still good to find a security bug, but I don't think this has a high chance of exploit.
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8874029 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: bug 1197315 [User impact if declined]: it's unlikely non-Firefox-developers would ever be affected by this [Is this code covered by automated tests?]: well, probably not (given that the failing assertion never fired) [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: this is a debug-only feature [String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8874029 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 16•7 years ago
|
||
I don't think it's necessary to uplift to ESR52. If nobody's encountered this yet, they're unlikely to.
Comment 17•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b790f9291cdb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Group: gfx-core-security → core-security-release
Comment 18•7 years ago
|
||
Comment on attachment 8874029 [details] [diff] [review] patch Fix a security issue. Beta56+.
Attachment #8874029 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d2273fb5bca3
Updated•7 years ago
|
Whiteboard: [adv-main56+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•