Closed
Bug 1186489
Opened 9 years ago
Closed 9 years ago
Clamp the resolution of performance.now() in workers too
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: csectype-disclosure, privacy, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41-])
Attachments
(1 file)
2.21 KB,
patch
|
froydnj
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8637312 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56bc3c9a172
Assignee | ||
Comment 4•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
Comment 5•9 years ago
|
||
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+
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e56bc3c9a172 https://hg.mozilla.org/releases/mozilla-aurora/rev/15b3cf5a2831
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-b2g-v2.1S:
--- → wontfix
status-b2g-v2.2:
--- → affected
status-b2g-v2.2r:
--- → affected
status-b2g-master:
--- → fixed
status-firefox-esr38:
--- → affected
tracking-firefox-esr38:
--- → ?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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.
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Group: core-security → core-security-release
Comment 8•9 years ago
|
||
bz, do you want to uplift this to ESR 38.3.0? Sounds like a good candidate for it.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 9•9 years ago
|
||
We could do that, but there's no point unless we also uplift bug 1167489. Uplifting both might make sense.
Flags: needinfo?(bzbarsky)
Comment 10•9 years ago
|
||
Jason didn't think it was a good risk to uplift. Thanks bz and Jason for the advice!
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•