Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Vulnerability] Bad password storing practices #73

Closed
bakad3v opened this issue Sep 7, 2024 · 5 comments
Closed

[Vulnerability] Bad password storing practices #73

bakad3v opened this issue Sep 7, 2024 · 5 comments

Comments

@bakad3v
Copy link

bakad3v commented Sep 7, 2024

I noticed that you are storing automation key in plaintext in shared preferences. It's bad practice for storing passwords. Shared preferences possibly can be read by anyone who have root access. It's a good practice to store hash instead of password, and ideally computation of password hash must be a time consuming process. There are special key derivation functions such as Argon2 created for this purpose. I implemented password hashing in this pull request. I used default parameters for Argon2, may be they must be changed.
#72

@bakad3v
Copy link
Author

bakad3v commented Sep 7, 2024

Also more complex checking for password strength required, key with length 6 is really weak. And it will be nice to add random key generation function.

@BinTianqi
Copy link
Owner

Shared preferences possibly can be read by anyone who have root access.

Anyone who have root access can also write SharedPrefs. So, in my opinion, storing hashed automation key is futile.

key with length 6 is really weak. And it will be nice to add random key generation function.

This is a good idea, I will add random key generation function. 20 bytes of automation key should be enough

BinTianqi added a commit that referenced this issue Sep 8, 2024
Require length of automation key at least 20
Random automation key generation
@1fexd
Copy link

1fexd commented Sep 8, 2024

Anyone who have root access can also write SharedPrefs. So, in my opinion, storing hashed automation key is futile.

I think you could use the Android KeyStore

@bakad3v
Copy link
Author

bakad3v commented Sep 8, 2024

Yes, I also think using encrypted shared preferences may be a proper solution.

@BinTianqi
Copy link
Owner

As a user application, it is impossible to compete with root privilege.

So, all I have to do is prevent the key from being cracked by other user apps. I think a 20-digit key should be difficult enough to crack.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants