Closed Bug 1369560 Opened 3 years ago Closed 3 years ago

address potentially unsafe snprintf usage in FPSCounter

Categories

(Core :: Graphics, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 - wontfix
firefox54 - wontfix
firefox55 - wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])

Attachments

(1 file)

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).
Attached patch patchSplinter Review
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)
Comment on attachment 8874029 [details] [diff] [review]
patch

Review of attachment 8874029 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8874029 - Flags: review?(mchang) → review+
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
Actually it looks like sec-moderate is more appropriate.
Keywords: sec-lowsec-moderate
(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.
Per comment #3, track 53-/54- as it's unlikely to overflow the buffers.
tracking for 55.
(To be clear, I'm waiting for bug 1368652 to land before landing this.)
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.
Actually, on second thought, it would be safest to wait until bug 1368652 actually ships (in 55).
Keywords: checkin-needed
bug 1368652 has now shipped, is anything else blocking this one?
Flags: needinfo?(dkeeler)
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.
(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.
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?
I don't think it's necessary to uplift to ESR52. If nobody's encountered this yet, they're unlikely to.
https://hg.mozilla.org/mozilla-central/rev/b790f9291cdb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: gfx-core-security → core-security-release
Comment on attachment 8874029 [details] [diff] [review]
patch

Fix a security issue. Beta56+.
Attachment #8874029 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main56+]
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.