patrickfav / armadillo

A shared preference implementation for confidential data in Android. Per default uses AES-GCM, BCrypt and HKDF as cryptographic primitives. Uses the concept of device fingerprinting combined with optional user provided passwords and strong password hashes.
https://favr.dev/opensource/armadillo
Apache License 2.0
280 stars 52 forks source link

Allow getting and setting values from different threads in parallel #41

Closed MarkVillacampa closed 5 years ago

MarkVillacampa commented 5 years ago

This solves #40

patrickfav commented 5 years ago

LGTM

MarkVillacampa commented 5 years ago

Taking another look at ByteArrayRuntimeObfuscator, I don't see a reason to add synchronized to createAndEncrypt, since it is only called in two places:

I don't think it'd make much difference in performance, but I'll create If you agree I'll create another PR removing it, just for clarity and cleanliness :)

Also, according to Android studio, the wipe method in EncryptionFingerprint is never called. Is this right? shouldn't it be called after calling getBytes to wipe the unobfuscated bytes from memory?

If this is the case, EncryptionFingerprint would not be thread-safe if wipe and getBytes would be called simultaneously from two threads.

Lastly, any chance you could release a new version including this fix? It'd be really helpful for me, otherwise I'll have to find a way to compile and use a version from master in my app.

patrickfav commented 5 years ago

I don't see a reason to add synchronized to createAndEncrypt

True. Only thing it would guard is the constructor, but constructing is atomic afaik, ie. if it is not constructed nobody can call a method on it.

Also, according to Android studio, the wipe method in EncryptionFingerprint is never called. Is this right? shouldn't it be called after calling getBytes to wipe the unobfuscated bytes from memory?

Yes this is correct. This is more for the user of this library so it easily possible to wipe the fingerprint for them. In ByteArrayRuntimeObfuscatorthe createAndEncrypt after getting the bytes overwrites the internal byte array with a re-obfuscated version, so wipe is not necessary (if we are talking about the same thing)

If you want to apply another patch, pls open another PR. I forgot to tell you before last time, but please add the change in the CHANELOG. I will could release 0.8.0 today.