Reduce precision of performance.now() to 20us

VERIFIED FIXED in Firefox -esr52

Status

()

defect
VERIFIED FIXED
2 years ago
4 months ago

People

(Reporter: dveditz, Assigned: tjr)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox57blocking verified, firefox58blocking verified, firefox59+ verified)

Details

(Whiteboard: [adv-esr52.6+])

Attachments

(3 attachments)

Bug 1424341 sets out to make our timer precision controllable via pref setting, and reduces the basic precision from 5us to 20us to defeat some timing attacks we're aware of. But that patch is quite large and due to some disclosures happening sooner than we expected we'd like to reduce the precision of performance.now() immediately. Taking Tom's patch from
https://hg.mozilla.org/try/rev/e52f5d2314991ebbad8ebf503e4fdff06d5330a0
Ben: you reviewed this change as part of bug 1424341. We'd like to pull this piece out so we can chemspill in response to some impending disclosures about timing attacks.
Attachment #8939670 - Flags: sec-approval+
Attachment #8939670 - Flags: review?(bkelly)
Attachment #8939670 - Flags: review?(bkelly) → review+
AFAICT, this affects ESR52 as well.
https://hg.mozilla.org/mozilla-central/rev/c9bec96cc78978b5282185534d7e7c8c436e7822
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Can you request uplift here if you're ready? Should this go into 57.0.4 or do we need more testing first?
Flags: needinfo?(dveditz)
Flags: needinfo?(bkelly)
Looks like it was already uplifted.
Flags: needinfo?(bkelly)
Yup, got it, but, we have not uplifted to esr though it is marked affected. We still could uplift to ESR tonight. I have not started builds yet.

Looks like Fennec is also affected here.
tracking-fennec: --- → ?
Flags: needinfo?(sledru)
From the slack discussion, we don't want to chemspill esr.
Flags: needinfo?(sledru)
We managed to verify this issue on 57.0.4-build1 (20180103231032) and 58.0b14-build2 (20180103230655) using the manual resolution test from comment 3, across the platforms (Windows 10 x64, Windows 7 x86, macOS 10.13 and Ubuntu 16.04 x64).
There is still a possible problem that we reproduced on both builds, using Ubuntu, Windows 10 and Windows 7 - the 20 us interval is not constant every time. A 19/21 us interval is sometimes triggered (e.g. Output: 360,380,399,420,440,460,479,500,520,540,560,580,600,619,640,660,680,700,720,740,759,780,800,820,840 or 360,380,400,460,480,500,520,540,560,580,599,619,720,740,760,779,800,820,840,860,880,900,920,939,960).
Any thoughts about this?
Flags: needinfo?(tom)
(In reply to Iulia Cristescu, QA [:JuliaC] from comment #10)
> We managed to verify this issue on 57.0.4-build1 (20180103231032) and
> 58.0b14-build2 (20180103230655) using the manual resolution test from
> comment 3, across the platforms (Windows 10 x64, Windows 7 x86, macOS 10.13
> and Ubuntu 16.04 x64).
> There is still a possible problem that we reproduced on both builds, using
> Ubuntu, Windows 10 and Windows 7 - the 20 us interval is not constant every
> time. A 19/21 us interval is sometimes triggered (e.g. Output:
> 360,380,399,420,440,460,479,500,520,540,560,580,600,619,640,660,680,700,720,
> 740,759,780,800,820,840 or
> 360,380,400,460,480,500,520,540,560,580,599,619,720,740,760,779,800,820,840,
> 860,880,900,920,939,960).
> Any thoughts about this?

My suspicion off the top of my head is floating point fuzziness. But I am not 100% certain. It should be easy enough to confirm this using a one-off build; if I create such a try run, are you able to repro and test it easily?
Flags: needinfo?(tom) → needinfo?(iulia.cristescu)
(In reply to Tom Ritter [:tjr] from comment #11)
> 
> My suspicion off the top of my head is floating point fuzziness. But I am
> not 100% certain. It should be easy enough to confirm this using a one-off
> build; if I create such a try run, are you able to repro and test it easily?

Yes, if you will be able to provide us the build, it should be a quick check.
Flags: needinfo?(iulia.cristescu) → needinfo?(tom)
Okay, try this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd4098d29b653ffb522cfec5117c8c388e9cfac1
Flags: needinfo?(tom) → needinfo?(iulia.cristescu)
(In reply to Tom Ritter [:tjr] from comment #13)
> Okay, try this one:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bd4098d29b653ffb522cfec5117c8c388e9cfac1

I was still able to reproduce the 19/21 us interval on Windows 7 x64, using the provided build. (see the screenshot https://goo.gl/yLt76u). This is also intermittent.
Flags: needinfo?(iulia.cristescu) → needinfo?(tom)
My guess its because the test is hitting some unexpected floating point rounding because it does its mod operation before flooring to an integer.

Ultimately if our mean resolution is 20us it still addresses the problem.  I don't think we need this test to show precisely 20us resolution on every run.
I think Ben is correct. I'm attaching a new test. (Should be more friendly to run too.)

First though: I haven't seen any 21 ms entries in the output you've shown, only 19's. Can you confirm you did get 21's?  That would be *very* weird I think.

In the new test:

- If you see anything in red: that's bad.
- If you see things with R: that's okay
- You should see entires with a blue 'Rounded:' with approximately the same frequency you previously saw the '19' entries.
Flags: needinfo?(tom) → needinfo?(iulia.cristescu)
Yes, I should have mentioned there's occasional floating point weirdness in my quick-n-dirty test (sometimes performance.now() itself returns a wonky value so there's some rounding going on inside too). Math.floor() was an attempt to hide most of it. In any case it's easy to confirm the difference between 5us builds and 20us builds.
Flags: needinfo?(dveditz)
(In reply to Tom Ritter [:tjr] from comment #16)
> Created attachment 8940249 [details]
> manual resolution test 2
> 
> I think Ben is correct. I'm attaching a new test. (Should be more friendly
> to run too.)
> 
> First though: I haven't seen any 21 ms entries in the output you've shown,
> only 19's. Can you confirm you did get 21's?  That would be *very* weird I
> think.
> 
See the (...) 540, 559, 580 (...) series, where the first interval is 19us and the next one is 21us. Same as in (...) 680, 699, 720 (...) or in (...) 720, 739, 760 (...). 
>
> In the new test:
> 
> - If you see anything in red: that's bad.
> - If you see things with R: that's okay
> - You should see entires with a blue 'Rounded:' with approximately the same
> frequency you previously saw the '19' entries.

Investigated this again with the "manual resolution test 2" on Windows 7 x64, Windows 10 x64 and Ubuntu 16.04 x64, using 58.0b14-build2 (20180103230655) and I can confirm that only the expected results were triggered.
Flags: needinfo?(iulia.cristescu)
I also confirm that the same expected results (as Tom mentioned in comment 16) are triggered using 59.0a1 (2018-01-07) and 57.0.4-build1 (20180103231032) across platforms (Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13.2.
See Also: → 1424341
Is there any reason for us not to get this landed on ESR52 now at least?
Comment on attachment 8939670 [details] [diff] [review]
reduce performance precision

This needs to land on ESR52 still before we ship 52.6.0. We should probably also set the Beta/Release approval flags for consistency's sake :)
Attachment #8939670 - Flags: approval-mozilla-release?
Attachment #8939670 - Flags: approval-mozilla-esr52?
Attachment #8939670 - Flags: approval-mozilla-beta?
For 52, our options are:

1) Uplift a truly massive set of patches. I think this is infeasible.

2) Rewrite the patches from scratch to provide an adjustable pref controlling all timers (Largeish patch)

3) Hardcode all timers to round at some value we choose (Small-Medium patch)

4) Hardcode performance.now() to round at some value we choose (Tiny patch)
Flags: needinfo?(overholt)
Flags: needinfo?(luke)
Will #2 be largeish, but mechanical, easy to write and low-risk?  If so, that seems like the best option (assuming we have the same system-addon ability on ESR as we do for release).
Flags: needinfo?(luke)
(In reply to Luke Wagner [:luke] from comment #23)
> Will #2 be largeish, but mechanical, easy to write and low-risk?  If so,
> that seems like the best option (assuming we have the same system-addon
> ability on ESR as we do for release).

I can think of at least two things we'll be doing that are non-mechanical.  We'll need to (re-create) nsRFPService:
https://searchfox.org/mozilla-central/source/toolkit/components/resistfingerprinting/nsRFPService.h
https://searchfox.org/mozilla-central/source/toolkit/components/resistfingerprinting/nsRFPService.cpp

Which is where we centralize the rounding, watch for pref changes, and such.


Over in https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.h#304 we'll be adding AddAtomicXVarCache functions.

There's probably one or two other non-mechanical things to add also, nsRFPService has been added and subtracted to over quite a few patches in the past 6 months.

The first one is the one I'm more nervous about. It's probably not that big of a change in the real scope of things, I just worry I'd miss something while doing it.
Ok, sounds like not something we should be doing in the final weeks before ESR52.6, then?  Given that, yeah, option 3 sounds good.  So what constant?  I wonder if the slow/tested deployment we assume for ESR (as opposed to the silent rapid update of release) justifies us in going straight to 1ms.
Comment on attachment 8939670 [details] [diff] [review]
reduce performance precision

this was already uplifted in comment 6
Attachment #8939670 - Flags: approval-mozilla-release?
Attachment #8939670 - Flags: approval-mozilla-release+
Attachment #8939670 - Flags: approval-mozilla-beta?
Attachment #8939670 - Flags: approval-mozilla-beta+
(In reply to Luke Wagner [:luke] from comment #25)
> Ok, sounds like not something we should be doing in the final weeks before
> ESR52.6, then?  Given that, yeah, option 3 sounds good.  So what constant? 
> I wonder if the slow/tested deployment we assume for ESR (as opposed to the
> silent rapid update of release) justifies us in going straight to 1ms.

While this is probably correct, is it acceptable for us to do this *not* on release (where, despite ESR not being 0 users, the majority of our users are)?
Flags: needinfo?(overholt) → needinfo?(luke)
(In reply to Andrew Overholt [:overholt] from comment #27)
> (In reply to Luke Wagner [:luke] from comment #25)
> > Ok, sounds like not something we should be doing in the final weeks before
> > ESR52.6, then?  Given that, yeah, option 3 sounds good.  So what constant? 
> > I wonder if the slow/tested deployment we assume for ESR (as opposed to the
> > silent rapid update of release) justifies us in going straight to 1ms.
> 
> While this is probably correct, is it acceptable for us to do this *not* on
> release (where, despite ESR not being 0 users, the majority of our users
> are)?

Also, please open a new bug, given that the 20us patch was already uplifted by RyanVM a couple days ago.
Good point.  I wasn't familiar with ESR's dot-release schedule, but looking at it now, it appears ESR52.7 will ship either at the same time or a few weeks after we would set release to 1ms and so that suggests we be symmetric between release/ESR52.6 and use 20us on ESR52.6 (with Tom's Option 3).
Flags: needinfo?(luke)
(In reply to Luke Wagner [:luke] from comment #29)
> suggests we be symmetric between release/ESR52.6

You mean 52.7?
Flags: needinfo?(luke)
And do you want to do 20us with Option 3 (all timers) or Option 4 (just performance.now() )?
Okay I misunderstood. Yea we should be symmetric between 52.6 and current release and set it to 20us. But do we want to set all timers in 52.6 or just performance.now?

I think we should just do performance.now() (Option 4) in .6.

In both 58 and 52.7 we should set all timers to.... 1ms? 200us? And in .7 this will be done via hardcoding (Option 3)
(In reply to Tom Ritter [:tjr] from comment #32)
> In both 58 and 52.7 we should set all timers to.... 1ms? 200us? And in .7
> this will be done via hardcoding (Option 3)

If I read https://wiki.mozilla.org/RapidRelease/Calendar correctly, 58 and .6 are released at the same time.  Our current Plan of Record for 58 is 20us.  59 would be released at the same time as .7 and by then both should hopefully be at 1ms (if Beta experiments go well).

> I think we should just do performance.now() (Option 4) in .6.

Unless it would cause your patch to be rejected, Option 3 sounds much better and would match 58 which, iirc, would also fix all the timers?
Flags: needinfo?(luke)
Blocks: 1430173
(In reply to Luke Wagner [:luke] from comment #33)
> (In reply to Tom Ritter [:tjr] from comment #32)
> > In both 58 and 52.7 we should set all timers to.... 1ms? 200us? And in .7
> > this will be done via hardcoding (Option 3)
> 
> If I read https://wiki.mozilla.org/RapidRelease/Calendar correctly, 58 and
> .6 are released at the same time.  Our current Plan of Record for 58 is
> 20us.  59 would be released at the same time as .7 and by then both should
> hopefully be at 1ms (if Beta experiments go well).
> 
> > I think we should just do performance.now() (Option 4) in .6.
> 
> Unless it would cause your patch to be rejected, Option 3 sounds much better
> and would match 58 which, iirc, would also fix all the timers?

So I spoke with RyanVM. The thing is, while 58 can round all timers, it's not (the pref isn't on.) We don't know when we'll turn it on or to what value. It's probably safe to turn on at 20us but we don't know for sure. Actually we do know it causes one serious issue already: Bug 1429764  So we should let it bake more just being 'on' and then conduct more testing on an actual value to set it to.

We think it's best to land 20us on ESR now, and then figure out what value (and when) we want to hardcode all timers to. I opened Bug 1430173 for that.

Ritu: can you land this on ESR at the earliest convenience?
Flags: needinfo?(rkothari)
Flags: needinfo?(rkothari)
Comment on attachment 8939670 [details] [diff] [review]
reduce performance precision

Thanks Tom for finalizing the plan for ESR52. Let's just land this fix in ESR52.
Attachment #8939670 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-esr52.6+]
Group: dom-core-security → core-security-release
tracking-fennec: ? → ---
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.