Closed Bug 1186489 Opened 4 years ago Closed 4 years ago

Clamp the resolution of performance.now() in workers too

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 + fixed
firefox42 + fixed
firefox-esr38 - wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: csectype-disclosure, privacy, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41-])

Attachments

(1 file)

Depends on: 1167489
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8637312 [details] [diff] [review]
Apply the performance.now() resolution clamping in workers as well

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

::: dom/base/nsPerformance.cpp
@@ +503,5 @@
>  {
>    double nowTimeMs = GetDOMTiming()->TimeStampToDOMHighRes(TimeStamp::Now());
> +  // Round down to the nearest 5us, because if the timer is too accurate people
> +  // can do nasty timing attacks with it.  See similar code in the worker
> +  // Performance implementation.

Thanks for modifying this!  It made it particularly easy to verify that the worker implementation in the patch was exactly the same as the non-worker one--without having to go hunting in my source tree.
Attachment #8637312 - Flags: review?(nfroyd) → review+
Comment on attachment 8637312 [details] [diff] [review]
Apply the performance.now() resolution clamping in workers as well


Approval Request Comment
[Feature/regressing bug #]: performance.now() implementation
[User impact if declined]: Various privacy leaks; see bug 1167489 and bug 1153672
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Medium risk.  Clamping to 1us would be pretty safe (since Chrome
   does that already), but no browser right now clamps to 5us.  That said, I
   don't expect anything in the wild depends on higher resolution than that right
   now... especially since worker performance.now() is not terribly interoperable
   to start with.
[String/UUID change made/needed]:
Attachment #8637312 - Flags: approval-mozilla-aurora?
Comment on attachment 8637312 [details] [diff] [review]
Apply the performance.now() resolution clamping in workers as well

Uplifting this to 41 to go with the fixes for bug 1167489 and bug 1153672.
Attachment #8637312 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Since the patch is medium risk, Kate and I reviewed this bug to determine whether we can uplift this to ESR 38.2.0 and it seems we don't know whether this patch is stable enough yet or not. We decided not to nominate for ESR uplift.

Let's review this fix again during ESR 38.3.0 time frame.
Whiteboard: [post-critsmash-triage]
Group: core-security → core-security-release
bz, do you want to uplift this to ESR 38.3.0? Sounds like a good candidate for it.
Flags: needinfo?(bzbarsky)
We could do that, but there's no point unless we also uplift bug 1167489.  Uplifting both might make sense.
Flags: needinfo?(bzbarsky)
Jason didn't think it was a good risk to uplift. Thanks bz and Jason for the advice!
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.