Fixed a security issue

[happylance]

Previously, inccorectPasscodeAttempts was reset to 0 after kill and restart. So an attacker can bypass the maximumInccorectPasscodeAttempts setting by killing and restarting the app after trying a different password again and again.

[ziogaschr]

Thanks a lot for finding this one @happylance. Will test it soon, but it looks good

[le4ker]

I also found this bug, but I mitigated it by recreating the instance (in order to reset the state of the instance).

[ziogaschr]

@happylance I think that the initial intention of @yankodimitrov was to leave handling of the persistance layer to the developer. Some may prefer Keychain over NSUserDefaults or even another mechanism.

I have tried to help over with this by adding example code in the demo app. Can you please have a look at PR #27. Let me know what you think of this.

Also this PR has part of you changes in this PR. I will be more than happy to remove them from my PR if you will to change the code in this PR, so as all the Kudoz comes to you 👍

Thanks for helping

[happylance]

PR #27 looks good. Thanks.

[ziogaschr]

As @PanosSakkos correctly pointed there is path that is not handled.

You can test it like in the PR #27 demo:

1. Enable passcode lock
2. Home button
3. Open demo app
4. Try 3 non-mathcing pin combinations
5. Quit app
6. Go to 3

What we can do is to add a method in PasscodeRepositoryType like var incorrectPasscodeAttempts: Int { get set }

What do you think?

[happylance]

Sorry that I do not understand why incorrectPasscodeAttempts should be part of PasscodeRepositoryType.

[ziogaschr]

Because then the developer who uses the library will have to implement where (Keychain, NSUserDefaults) and how to persist the incorrectPasscodeAttempts

[happylance]

I see. My only concern is that it will make this library too complicated to use.

[ziogaschr]

Let's make it optional. So as only people who need this will implement it.

Let me know if you find it a good idea and also if you will make a PR or I shall add this in PR #27 so as we can do a squash of commits

Sent from my iPhone

On 12 ???? 2016, at 17:37, happylance notifications@github.com<mailto:notifications@github.com> wrote:

I see. My only concern is that it will make this library too complicated to use.

You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://github.com/velikanov/SwiftPasscodeLock/pull/22#issuecomment-225438454, or mute the threadhttps://github.com/notifications/unsubscribe/AAw_jM96GG34W0tp5y2YxVI8zYohMumqks5qLBmjgaJpZM4IfAYb.

[happylance]

I think it is better to handle incorrectPasscodeAttempts correctly within this library so that it will be easier to use. You can add it in PR #27.

[le4ker]

@ziogaschr thanks for the fix! 👏

[ziogaschr]

Let's thanks @happylance and you (@PanosSakkos), rather than me :)

[ziogaschr]

add a method in PasscodeRepositoryType like var incorrectPasscodeAttempts: Int { get set }

This is not easy without more changes in the library. I am thinking what's the best way, in order to keep the storage mechanism option to the developers.

@yankodimitrov can you suggest something on this topic?

[le4ker]

By the way shouldn't the pod (patch) version be increased?

[ziogaschr]

Good point, we can do this, although we can't update the Pod repository as this is not the main repo. Try to use this pod 'PasscodeLock', :git => 'https://github.com/ziogaschr/SwiftPasscodeLock.git', :branch => 'master' in your Podfile

[le4ker]

Isn't your repository the first result here?

// I'm not familiar with how publishing on cocoapods works :yum:

[ziogaschr]

Woow, I didn't know this. Nice work @velikanov. So, let's merge some of PRs and update the version later this week.

Nice catch @PanosSakkos

[le4ker]

Bump! :boom:

Any ETA on when this fix will go in? :smile:

[le4ker]

Is there any reason that this PL wasn't merged yet? 😟