search

nielsfaber / alarmo

Easy to use alarm system integration for Home Assistant
1.41k stars 119 forks source link

Replace bcrypt hashing with blake3 for improved performance #1030

Closed mrpriv4te closed 1 month ago

mrpriv4te commented 2 months ago

Hi nielsfaber! I made this change because the bcrypt hash function was too slow, and with several users and codes on my installation, I observed delays of 5-10s to verify the pincode/password.

nielsfaber commented 2 months ago

Hi @mrpriv4te, thanks for the PR. I am not an expert on hashing methods so if you found a better alternative it's good to me. I must say I'm surprised with the high delay you are experiencing, I believe Alarmo uses a similar implementation as HA.

One thing holding me back from merging this: Alarmo stores the hashed passwords, so users have now bcrypt-ed hashes in their configuration. I assume these are not compatible with your modified code, so users will need to reset their password before it would work. I think we should make a backwards-compatible implementation, for example by detecting the pattern of the stored hash and using bcrypt if so. What are your thoughts about this?

mrpriv4te commented 2 months ago

After further testing, I realized that the time saving was not significant, so I opted for sha256, which is faster.

I must say I'm surprised with the high delay you are experiencing, I believe Alarmo uses a similar implementation as HA.

If you want to reproduce the latency, add several users (12 in my case) to Alarmo and try arming or disarming with the code of the last user created.

I think we should make a backwards-compatible implementation, for example by detecting the pattern of the stored hash and using bcrypt if so. What are your thoughts about this?

I totally agree with you, and now it's done.

marazmarci commented 2 months ago

@mrpriv4te please don't just use plain SHA256 without salting because a password will be much easier to reverse from known hashes or by using a rainbow table.

The original bcrypt.hashpw method used a randomly generated salt and ran 2^12=4096 rounds of hashing, which is pretty standard and secure. We should come up with a better solution that doesn't compromise security.

mrpriv4te commented 2 months ago

@mrpriv4te please don't just use plain SHA256 without salting because a password will be much easier to reverse from known hashes or by using a rainbow table.

The original bcrypt.hashpw method used a randomly generated salt and ran 2^12=4096 rounds of hashing, which is pretty standard and secure. We should come up with a better solution that doesn't compromise security.

Hi @marazmarci, thanks for your feedback !

I've now added salting to the password hash, and I've replaced sha256 with blake3, which is not vulnerable to length extension attack.

There's still one problem, blake3's python module needs rust/cargo and gcc to build, but I don't know how an HA integration can install these dependencies.

nielsfaber commented 1 month ago

@mrpriv4te Following the discussion I'm not confident we should change the current password encryption. I don't have the knowledge to evaluate the impact of your proposed change so I rather keep the same method as used by HA.

I also think there is not much benefit to this change for other users. You said you created 12 different pin codes, which I don't expect many other users will have. In addition the computation time can also be related to the hardware you are using for running HA.

If you still want to see this PR merged, I would like this change to be reviewed by multiple people who have experience with encryption methods.

mrpriv4te commented 1 month ago

Hi @nielsfaber, I totally understand, I'll use my fork on my side.