Merge lp:~unity-team/unity8/lockout-time-travel into lp:unity8
| Status: | Merged |
|---|---|
| Approved by: | Michael Zanetti |
| Approved revision: | 2085 |
| Merged at revision: | 2116 |
| Proposed branch: | lp:~unity-team/unity8/lockout-time-travel |
| Merge into: | lp:unity8 |
| Diff against target: |
49 lines (+20/-3) 2 files modified
qml/Greeter/Greeter.qml (+6/-3) tests/qmltests/Greeter/tst_Greeter.qml (+14/-0) |
| To merge this branch: | bzr merge lp:~unity-team/unity8/lockout-time-travel |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Zanetti (community) | Approve | ||
| PS Jenkins bot (community) | continuous-integration | Needs Fixing | |
|
Review via email:
|
|||
Commit message
Guard against a couple odd timing scenarios for the too-many-
Description of the change
The issue was reported here: https:/
To reproduce this, start with a phone you don't care about (so you can wipe it afterwards to get it working again):
- Set your phone time to 'manual' instead of 'automatic' in System Settings
- Run 'gsettings set com.canonical.
- Run 'sudo hwclock --set --date @0' to simulate resetting your RTC clock.
- adb reboot
- Now you're locked out for years and years.
Here's why this happened. The old code (briefly and with pseudocode):
===
var now = new Date();
delayTarget = now;
delayTarget.
// If tooEarly is true, something went very wrong. Bug or NTP
// misconfiguration maybe?
var tooEarly = now.getTime() < greeterSettings
var tooLate = now > delayTarget;
if (!tooEarly && !tooLate)
startLockout();
===
The big mistake is that we re-use the now pointer with "delayTarget = now". And this was my mistake -- this is my code. :( In Javascript, that's just a pointer assignment rather than a value/copy assignment.
So that bug means that 'now' and 'delayTarget' are equal. So tooEarly and tooLate will always be false. Which means when we start up, we always start a lockout. It's just that usually when we tick down during the lockout, we notice that the current time is later than delayTarget and stop the lockout.
But in this case, time is years and years before delayTarget. So we keep the lockout going.
There are other bugs in this code that I'm fixing with this branch:
- We shouldn't even run this code if greeterSettings
- tooLate should probably be "now >= delayTarget"
| PS Jenkins bot (ps-jenkins) wrote | # |
| Michael Terry (mterry) wrote | # |
| PS Jenkins bot (ps-jenkins) wrote | # |
| Michael Terry (mterry) wrote | # |
| Michael Terry (mterry) wrote | # |
| PS Jenkins bot (ps-jenkins) wrote | # |
| Michael Terry (mterry) wrote | # |
+ // - And we trust this check because if a malicious actor is able
+ // to manipulate time to avoid our lockout, there's not much we
+ // can do to stop them.
Hmm... not so sure about this. AFAIU, one could fake ntp replies with relatively low efforts and with that escape the time limit. Brute forcing a 4 digit pin without the time limit is not too hard. Even if the risk of someone doing this is quite low, it might just be enough to get us bad press. I would feel better if someone from security has seen this.