Improper use of bcrypt API

[ebuchman]

In Tendermint v0.27.3, we reverted to use the mainline Golang crypto lib, which means we reverted the modifications we had made to bcrypto.GenerateFromPassword. The modification was to add a salt argument. In the mainline API, a salt is generated randomly within the function, though it is returned as part of the output. I believe there were two reasons to add the salt argument: 1) so salt could come from user supplied randomness (?) and 2) so we could use this to repeatedly derive the same symmetric key from a password. Without this, multiple calls to GenerateFromPassword with the same password would result in different outputs (due to different salts).

This means the bcrypt API is not meant to be used to derive a symmetric key like this, since it cannot be re-derived without persisting it, which would compromise the key. Of course the output of bcrypt.GenerateFromPassword is expected to be persisted, since it's supposed to be used to prove that a user has the pre-image (ie. the password). Using it to generate an encryption key requires a modification of the API to take a salt, otherwise future calls will utilize new random salts, and thus it will not be possible to re-derive the same key.

For the SDK to update to Tendermint v0.27.3, it would need to copy in the modified bcrypt module for users to be able to continue to decrypt existing keys.

We should consider fixing this misuse of the bcrypt API all together. Probably the right thing to do is use https://godoc.org/golang.org/x/crypto/pbkdf2 instead of bcrypt here.

A further consideration would be to adopt the same standard that go-ethereum is using for keys.

[ebuchman]

Here's an old issue discussing ethereum format: tendermint/go-crypto#13

LOL not sure what caused me to say this. Maybe the code base was still changing a lot then. But doesn't seem like a well justified reason ...

everything they do tends to be painful

[ValarDragon]

For password hashing, the salt is there to protect against rainbow tables.

I agree with doing pbkdf as a quick fix, and then moving to the ethereum key format once we have more time. We can actually upgrade lazily, when you decrypt with a bcrypt key, just save it with the pbkdf key.

[ebuchman]

For password hashing, the salt is there to protect against rainbow tables.

Yes, but it's not meant to be recomputed again, so you can't specify it, it just get's generated for you.

[ebuchman]

when you decrypt with a bcrypt key, just save it with the pbkdf key.

This works, but I guess we'll have to copy in the modified bcrypt module to do this since Tendermint removed it.

[ebuchman]

Oh, in the meantime we can just continue to import the forked version, we don't have to copy it in

[ebuchman]

Cross linking #2089 for reference - most of that seems to be completed already, but anything that's not can be tracked from here (mostly the dependency issues)

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[julienrbrt]

Relevant: #14646.