Key in source - security issue

[doridori]

First of all, great job - looks nice.

From a quick look it seems that you key is based upon a constant in the Hawk source

final class AesEncryption implements Encryption {
    private static final String KEY_SALT = "asdf3242klj";
    private static final String PASSWORD = "asdf39230234242klj";
...

This is not really that secure. Granted better than storing prefs in plain text but not really great practise and should probably be documented in the README.

Maybe I have missed something and you have already done the below. Improvement suggestions would be to

a) base the encryption key on some user entered info, i.e. a pin or pass b) utilise the OS level key storage so you are not storing your key in the code

This seems to be the fundamental error made with in-app encryption.

Apologies if you have accounted for this somewhere and I have missed it. Again, may be worth documenting as this is the primary function of an encryption library and the details are important.

[jnagels]

:+1:

[orhanobut]

@doridori Thank you for this issue. I am actually aware of this and wanted to have a good solution really. At the moment I just made it temporary to see if the whole system works but then had no time to change it actually. I'll consider this priority and find a better solution. I'll try to use OS level key storage approach.

[chrisjenx]

:+1:

[doridori]

@orhanobut Great stuff! I recommend this book for some info on it http://www.nostarch.com/androidsecurity. If you do decide to go the system keystore route beware there are a few issues relating to the stored keys being lost if you are requiring a device pin to encrypt the stored key(s) and the user changes the pin.

See here for the current open issues list https://code.google.com/p/android/issues/list?can=2&q=KeyPairGeneratorSpec&colspec=ID+Type+Status+Owner+Summary+Stars&cells=tiles

Would be great to have a pluggable key handler component to your lib - then user input or system backed storage would be easier to swap in and out.

[orhanobut]

@doridori Thanks for the book recommendation!. I'll certainly check it out. It'll be great help. I'm also checking the current open issue. I'll consider this as well. Thank you for this great feedback!

[hamen]

:+1:

[orhanobut]

We've done some improvements regarding to the password. Salt is still stored in the storage(currently it is shared preferences). Password is not constant now and it should be provided by the user. Need to work on this issue more apparently.

[orhanobut]

I also added the details to the readme file in order to explain the situation.

[doridori]

Cool. As mentioned above you can have it so you encrypt a generated password by a software or hardware backed keypair. I am writing a blogpost highlighting some issues on this at the moment however. Will post the link here when done.

[orhanobut]

@doridori Looking forward to see the post :+1:

[doridori]

http://systemdotrun.blogspot.co.uk/2015/02/android-security-forgetful-keystore.html

[dcgavril]

If I may suggest something, you could generate the "salt" by using the device ID (that is mostly unique) + device model/product (or similar formula), so in this way it's different on most devices(almost, because sometimes the device id it's the same on all devices from that manufacturer - but that's a max 20% of the market), and if someone manages to get access at the preferences, at least it's going to be a bit more difficult to decrypt. (now, everyone who uses the compiled version of the library uses the same salt key, from what I understand). Anyway, I believe that the salt should not be kept in plain text in the shared preferences with the other data, the way you can generate the salt as I've described before, you don't need to actually store it somewhere, you regenerate it every time you need it.

Thanks!

PS: I'm interested to see some updates for this library.

[Rainer-Lang]

@dcgavril +1

[doridori]

I would avoid generating a salt in this way. @dcgavril you have not mentioned where you will obtain the deviceID from exactly but this can be an error prone approach (look at this SO post for example http://stackoverflow.com/questions/2785485/is-there-a-unique-android-device-id). Plus it does not really give you any advantage imho than just generating a salt on the fly for each device to use and storing in plain txt, but it does create disadvantages by allowing the secure store to be corrupted for the edge cases where the deviceID can change or not be available (again see post). Even if the deviceID was always available and never changed under any circumstance, I don't not see an advantage in this method. In fact, I would maybe not even bother with a salt at all and just put the time allowing for a secure key generation / storage approach as I dont see the advantage of using a salt for this libs use-case either! (if using hardware/software backed keystore for storing keys)

[dcgavril]

@doridori fair point on the deviceID mess ... but from Android 2.3 > it's a lot better and if the user does a factory reset that will change the device id... than it will also clean up the apps so he will lose the database anyway. There is another solution that was presented on the android dev blog http://android-developers.blogspot.com.au/2011/03/identifying-app-installations.html that it's actually generating an id/key on the fly, but you need to store it somewhere safe, so that you can use it for decrypting. Anyway, there are many solutions, none of them 100% perfect, so we just need to find the one with the best compromise.

[doridori]

@dcgavril regarding the article, this is why I was suggesting if you really wanted to use a random salt then you could generate on the fly for each device. This is the approach I always use for some unique app installation identifier, due to the article you referenced. Really a salt is public data and it is simpler to just generate randomly than any other strategy imho.