Unity 8

Merge lp:~unity-team/unity8/lockout-time-travel into lp:unity8

lockout-time-travel
Merge into trunk

Proposed by Michael Terry on 2015-12-10

Status:	Merged
Approved by:	Michael Zanetti on 2015-12-16
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:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Zanetti (community)		2015-12-10	Approve on 2015-12-16
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-12-15
Review via email: mp+280206@code.launchpad.net

Commit Message

Guard against a couple odd timing scenarios for the too-many-failed-login-attempts lockout screen.

Description of the Change

The issue was reported here: https://lists.launchpad.net/ubuntu-phone/msg17217.html

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.Unity8.Greeter locked-out-time 1450125324000' to simulate having gotten locked out in the past.
- 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.setTime(greeterSettings.lockedOutTime + failedLoginsDelayMinutes * 60000);

// If tooEarly is true, something went very wrong. Bug or NTP
// misconfiguration maybe?
var tooEarly = now.getTime() < greeterSettings.lockedOutTime;
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.lockedOutTime is 0 (never set)
- tooLate should probably be "now >= delayTarget"

Michael Zanetti (mzanetti) wrote on 2015-12-10:

+ // - 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.

review: Needs Information

PS Jenkins bot (ps-jenkins) wrote on 2015-12-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/6915/rebuild

review: Needs Fixing (continuous-integration)

lp:~unity-team/unity8/lockout-time-travel updated on 2015-12-14

2083. By Michael Terry on 2015-12-14: Condense code
2084. By Michael Terry on 2015-12-14: Add some more tests

Michael Terry (mterry) wrote on 2015-12-14:

@mzanetti, so there is one cheap thing we could do to mitigate that: don't change the lockout timer if the system time changes (and this MP introduces that logic, so I could just strip that out). That way, the attacker would have to reboot between attacks.

But, even with that logic in there, the attacker would have to turn networking on and off to trigger ntpdate checks (which happens in an ifupdown.d script). Which is a sight faster than rebooting or even 5 minute intervals, granted.

But I have no idea how to not trust system time. We could store "seconds left for lock out" but then we would still have locked you out if you turned your phone off for an hour. To cater to that case (turning off your phone while locked out) -- which doesn't seem SUPER unreasonable if can't use your phone for 5 minutes -- I think we have to trust system time somewhat.

Given that, maybe just the cheap fix above is a decent compromise. But will ask jdstrand his opinion.

PS Jenkins bot (ps-jenkins) wrote on 2015-12-14:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/6942/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2015-12-15:

Talked to jdstrand and tyhicks. End result is that we'll avoid relying on ntp by not caring about the reboot or power-off cases. Which admittedly are relatively corner cases anyway.

So I'll update this branch to not add the "watch clock changes during lockout" feature. But I'll propose the larger fix of not relying on ntp as a separate branch building on this. It's a separate change and I don't want to slow down this bug fix, since the bug is quite bad.

lp:~unity-team/unity8/lockout-time-travel updated on 2015-12-15

2085. By Michael Terry on 2015-12-15: Drop check-clock-every-tick logic, we don't need it

Michael Terry (mterry) wrote on 2015-12-15:

OK updated to simplified version. Will work on separate MP to drop wall clock watching.

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/6960/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2015-12-16:

(I'm also trying to get Design to comment on that proposed security change... didn't want to slow this fix down on that either.)

Michael Zanetti (mzanetti) wrote on 2015-12-16:

* Did you perform an exploratory manual test run of the code change and any related functionality?

yes

* Did CI run pass? If not, please explain why.

unrelated failures that are being addressed in another branch.

* Did you make sure that the branch does not contain spurious tags?

yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Greeter/Greeter.qml'
 --- qml/Greeter/Greeter.qml	2015-11-04 14:57:33 +0000
 +++ qml/Greeter/Greeter.qml	2015-12-15 19:09:13 +0000
@@ -226,14 +226,17 @@
+         }
          function checkForForcedDelay() {
++            if (greeterSettings.lockedOutTime === 0) {
++                return;
++            }
++
              var now = new Date();
--            delayTarget = now;
--            delayTarget.setTime(greeterSettings.lockedOutTime + failedLoginsDelayMinutes * 60000);
++            delayTarget = new Date(greeterSettings.lockedOutTime + failedLoginsDelayMinutes * 60000);
              // If tooEarly is true, something went very wrong.  Bug or NTP
              // misconfiguration maybe?
              var tooEarly = now.getTime() < greeterSettings.lockedOutTime;
--            var tooLate = now > delayTarget;
++            var tooLate = now >= delayTarget;
              // Compare stored time to system time. If a malicious actor is
              // able to manipulate time to avoid our lockout, they already have
 === modified file 'tests/qmltests/Greeter/tst_Greeter.qml'
 --- tests/qmltests/Greeter/tst_Greeter.qml	2015-09-14 07:47:24 +0000
 +++ tests/qmltests/Greeter/tst_Greeter.qml	2015-12-15 19:09:13 +0000
@@ -458,6 +458,20 @@
              compare(view.delayMinutes, greeter.failedLoginsDelayMinutes);
+         }
++        function test_forcedDelayOnConstructionIgnoredIfInFuture() {
++            greeterSettings.lockedOutTime = new Date().getTime() + greeter.failedLoginsDelayMinutes * 60000 + 1;
++            resetLoader();
++            view = findChild(greeter, "testView");
++            compare(view.delayMinutes, 0);
++        }
++
++        function test_forcedDelayOnConstructionIgnoredIfInPast() {
++            greeterSettings.lockedOutTime = new Date().getTime() - greeter.failedLoginsDelayMinutes * 60000 - 1;
++            resetLoader();
++            view = findChild(greeter, "testView");
++            compare(view.delayMinutes, 0);
++        }
++
          function test_forcedDelayRoundTrip() {
              greeter.failedLoginsDelayAttempts = 1;
              greeter.failedLoginsDelayMinutes = 0.001; // make delay very short