ewd340 / kr

A simple file encryption/decryption program using Monocypher.
The Unlicense
6 stars 2 forks source link

Consider storing password hash parameters in the key file #3

Closed LoupVaillant closed 1 year ago

LoupVaillant commented 1 year ago

Evidenced in gen_key() function, though it's a more general problem with the file format.

The problem here is that password hashing is a rather fast moving field, where the relatively weak entropy of passwords encourage us to routinely update the strength of our password hashes, or even the algorithm itself. And depending on context (are you using your beefy desktop or your crappy palmtop?) you may want to adjust the password strength. Version numbers are insufficient to deal with such a fast evolution, or the various use cases. Which is why I believe it is worth putting at least Argon2 parameters in the file format itself:

Speaking of lanes, I recall some papers noting that parallel versions of Argon2 are a bit weaker than just running several serial instances in parallel (with different additional data to distinguish them), then hashing the results together. So if you use a parallelism parameter, you should probably do that instead of using Argon2 lanes (whose support is not universal anyway, see Monocypher not supporting threads, and libsodium not even supporting lanes).


Of course, nothing here stops you from setting good defaults, or even hard code the parameters you encode into the key file. You can always add options or change the defaults later, as long as the file format itself (and the reader side) is prepared for the change.

ewd340 commented 1 year ago

I agree 100%. We just need to revise the gen_key function using a fifth parameter of type crypto_argon2_config as follows:

static enum error gen_key(uint8_t *passphrase, size_t passphrase_size,
                          uint8_t *salt, const crypto_argon2_config config, 
                          uint8_t key[KEY_SIZE])

and directly use config in the call to crypto_argon2 as follows:

crypto_argon2(key, KEY_SIZE, work_area, config, inputs, extras);

That would call for a new key format, and a new version of the program. Thus, my plan would be to freeze the current version of the app in a v-2.0 tag in its own "legacy" branch, and work on a new version with longer keys. Speaking of which, we can start with some hard-coded values for the Argon2i algorithm and eventually offer them as advanced parameters in the key generation operation.

In other words, one could invoke kr as follows with the default parameters:

$ kr -g ~/key

Or with advanced parameters as follows:

$kr -g -i5 -b200 -l4 ~/key

Where -i if for passes (iterations), -bfor blocks, and l for lanes.

ewd340 commented 1 year ago

A new version of kr has been pushed into the new-keyformat branch to address this issue. Some more reviewing (and eventually refactoring) might be needed before merging the changes back to the master branch, but for now it seems to work fine, and the tests passed just fine. Thanks again @LoupVaillant. Again your comments and suggestions on this issue (or the project as a whole) are more than welcome :smile: