[fix/bypass-lock-prevention] Bypass Passcode Prevention #1324
Conversation
There was a problem hiding this comment.
In addition to the code comments, please also take a look at:
- variable naming: f.ex.
lockSetTime,lockDurationandlockedUntilDateleave open what data is being used in each (clock time or system up time) , which makes the code harder to read. Using something likelockSetSystemUptime,systemUpTimeLockDuration(or justSysteminstead ofSystemUptime?) would clarify what is tracking what. - I found no mechanism resetting the countdown in case of a detected lock bypass
| let lockDuration = currentTime - lockSetTime | ||
| let timeRemaining = lockedUntilDate.timeIntervalSinceNow | ||
|
|
||
| if lockDuration < timeRemaining { |
There was a problem hiding this comment.
lockDurationcomputes the time since the lock has been settimeRemainingcomputes the time until the lock expires
Comparing the two will not be able to tell if there was an attempt to bypass the lock. Image a 10 second delay, where 2 seconds have passed by and 8 seconds are remaining - without any bypass attempt. These numbers would fit the condition for a lock-bypass above even though it hasn't been bypassed.
To determine a bypass, it's necessary to also keep track of the date/time the lock has been put in place - and then use that date to compute the time passed since locking, to compare it against the time passed in system uptime.
That comparison should also leave a tiny bit of wiggle-room (f.ex. ~ 0.05 seconds), because the systemUptime and the date are not taken at the exact same moment and therefore will regularly differ slightly.
…user changes the system time to bypass the passcode lock. ProcessInfo.processInfo.systemUptime does not work, because system does not restart after setting the system date/time. lockUntilDate is also stored in user defaults than lockedUntilDate before.
|
@felix-schwarz I decided to go another approach: Converted |
There was a problem hiding this comment.
I believe that, to reliably detect the change of system time to circumvent the lock, these need to be tracked:
- system uptime at the start of the lock (via
ProcessInfo.processInfo.systemUptime) - the start date/time and duration of the lock countdown
- the last known remaining time
If the system date is set to a future date to circumvent the lock, the app can catch this because the system uptime has not moved forward by the same amount of time.
If the device is rebooted, the system uptime will be lower than the uptime at the start of the lock.
In both cases, the app can use the last known remaining time to restart the lock.
Counting down the timeout time with a timer is not a viable solution IMO for the reasons outlined below.
| let seconds = Int(lockDelay) % 60 | ||
|
|
||
| if lockDelay > 0 { | ||
| self.lockDelay = (lockDelay - 1) |
There was a problem hiding this comment.
Counting down the lock delay with a timer has several issues:
- the app needs to stay in the foreground for the entire duration of the countdown
- additional instances of
AppLockManagerrunning in different processes (possibly even scenes) also reduce the lock delay every second, so that if the lock screen is shown in the app, in the Share Extension and the File Provider UI at the same time, it would run a 3x speed
|
Shouldn't this PR point to |
|
@felix-schwarz, I converted the code to use the system's significant time change notification to get informed about time changes while the app is locked. If the device is locked for a specific delay, the counter will start from the beginning if a time change occurs. Additionally, if the app was terminated and there is a lock delay, the counter will also reset. This prevents issues if a time change was not delivered. |
| @@ -153,11 +159,13 @@ public class AppLockManager: NSObject { | |||
| userDefaults = OCAppIdentity.shared.userDefaults! | |||
|
|
|||
| super.init() | |||
| significantTimeChangeOccurred() | |||
There was a problem hiding this comment.
By handling every initialization of the AppLockManager as a significant time change and resetting the countdown, users may be hit without trying to bypass the lock timer, f.ex. during regular usage like:
- invoking the share extension from a share sheet
- iOS quits the app in the background to free memory for apps launched after it
- the user closes the app
Adding UIApplication.significantTimeChangeNotification as a signal to the mix of detecting a bypass attempt is a good idea, but if the notification is not delivered to the app and/or app extensions when they aren't running at the time the significant time change occurs, it can't be relied on alone.
Resetting the timer every time the AppLockManager is initialized in a new process closes the gaps that a UIApplication.significantTimeChangeNotification can leave, but also has negative effects on legitimate usage, unfortunately, which I think we should avoid.
Why not use ProcessInfo.processInfo.systemUptime as outlined above?
There was a problem hiding this comment.
@felix-schwarz please go ahead if by implementing the prevention using ProcessInfo.processInfo.systemUptime if you think it is a better solution.
|
@hosy wrote in a comment above:
Ok, I implemented a solution based on |
Description
Prevents bypass the passcode by changing the device system time.
Related Issue
https://github.com/owncloud/security-tracker/issues/413
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: