-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed a security issue #22
Conversation
… 0 after kill an restart.
Thanks a lot for finding this one @happylance. Will test it soon, but it looks good |
I also found this bug, but I mitigated it by recreating the instance (in order to reset the state of the instance). |
@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 |
PR #27 looks good. Thanks. |
As @PanosSakkos correctly pointed there is path that is not handled. You can test it like in the PR #27 demo:
What we can do is to add a method in What do you think? |
Sorry that I do not understand why |
Because then the developer who uses the library will have to implement where (Keychain, NSUserDefaults) and how to persist the |
I see. My only concern is that it will make this library too complicated to use. |
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.commailto: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. |
I think it is better to handle |
@ziogaschr thanks for the fix! 👏 |
Let's thanks @happylance and you (@PanosSakkos), rather than me :) |
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? |
By the way shouldn't the pod (patch) version be increased? |
Good point, we can do this, although we can't update the Pod repository as this is not the main repo. |
Isn't your repository the first result here? // I'm not familiar with how publishing on cocoapods works 😋 |
Woow, I didn't know this. Nice work @velikanov. Nice catch @PanosSakkos |
Bump! 💥 Any ETA on when this fix will go in? 😄 |
Is there any reason that this PL wasn't merged yet? 😟 |
Previously,
inccorectPasscodeAttempts
was reset to 0 after kill and restart. So an attacker can bypass themaximumInccorectPasscodeAttempts
setting by killing and restarting the app after trying a different password again and again.