Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

Fix vulnerabilities and refactoring #160

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hluhovskyi
Copy link

@hluhovskyi hluhovskyi commented Oct 25, 2017

The main purpose of this PR to fix few vulnerabilities.

Due to fact that password is stored in shared preferences and algorithm of passcode is well known cause library is open source, so it is possible to create hash of own passcode and put it instead of original one on rooted devices. Then attacker can enter his passcode and get secured data.

Example:

  1. User entered “1111”,
  2. Library encrypts it to “xxxx”.Preferences looks like <string name=”PASSCODE_KEY” value=”xxxx”>
  3. Imagine that device is stolen and application contains very sensitive data. Attacker encrypts own passcode, e.g. “2222” in same way as library does. He gets “yyyy”.
  4. Attacker goes to /data/data//shared_preferences and open _preferences.xml.
  5. Attacker replaces original passcode hash with own one.
  6. Attacker run application and enter own passcode.

Fix performed – split storage from AppLock. It gives ability to implement own secured storage and use instead of default shared preferences. Also PR provides Realm implementation of storage which will be useful for developers who already uses Realm in project.

Second one – ability to enter passcode infinity times. It’s because count of attempts isn’t stored somewhere.

Condition - there is 5 attempts to enter correct password. If all attempts user enters incorrect passcode he logged out from application.

Example:

  1. Attacker launches application
  2. Attacker makes 4 attempts.
  3. Attacker relaunches application
  4. Attacker can make 4 attempts again.

Fix performed – store attempts count in storage.

Also this PR contains slight refactoring and dependency version updates.

Changes:

  • AppLockImpl now delegates storing all sensitive data to PasscodeDataStorage (salts, passcode hash, attempts count, etc.)
  • AppLockImpl now delegates storing all configuration data to ConfigurationStorage (timeout, fingerprint, etc.)
  • AppLock now has Builder which allows to create custom instances of AppLock without exposing of implementation. Previously you should directly interact with AppLockImpl, which isn’t good from architecture perspective.
  • AppLockImpl’s getInstance is deprecated. AppLock shouldn’t be singleton at all. It can be easily accessed via LockManager
  • Updates all gradle, support library dependencies. Previously it used very old dependencies that wasn’t even accessible in new versions of AndroidStudio.
  • Removed redundant genericazation of LockManager and AppLock.
  • Remove PinActionBarActivity which subclass of ActionBarActivity that was remove from support library

Ready for discussion.

@hluhovskyi hluhovskyi force-pushed the master branch 3 times, most recently from 0095350 to f928337 Compare October 26, 2017 15:14
@hluhovskyi
Copy link
Author

So, is there any chance that PR will be merged? Cause for my project it is critical. Please, left some comments and I will be glad to help with any changes or improvements related to this PR.

@daespark
Copy link
Contributor

daespark commented Oct 30, 2017 via email

@daespark
Copy link
Contributor

daespark commented Oct 30, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants