Secret keys are not protected by password

[rrrooommmaaa]

Description

rnpkeys key generation doesn't protect the secret key with password

Steps to Reproduce

A command like this is used in cli_tests.py rnpkeys --numbits 1024 --pass-fd 17 --userid some@rnp --gen-key

Secret key is not protected by the password

Expected Behavior

Messages encrypted to this recipient can not be decrypted without entering the correct password rnp -r some@rnp --encrypt message.txt --output encrypted.rnp rnp --decrypt encrypted.rnp --output message.dec

Actual Behavior

During the key generation, the parameter --pass-fd is used to create ffi_pass_callback_file, though this callback is never called. This happens because rnp_op_generate_st fields password and request_password are both zero (not initialized in cli_rnp_generate_key)

[dewyatt]

Confirmed, we're gonna need some tests added for this.

[dewyatt]

(Once this is fixed I'll do a 0.13.1 release)

[rrrooommmaaa]

@dewyatt I added rnp_op_generate_set_request_password(genkey, true) call to cli_rnp_generate_key function and it now ecnrypts (or at least tries to) the master key, but I still can decrypt without entering the password. Any ideas?

[dewyatt]

@rrrooommmaaa It's only encrypting the primary, not the sub unfortunately, so we'll have to dig into that issue.

[rrrooommmaaa]

@dewyatt and soultion is... to encrypt both keys?

[dewyatt]

Yes we probably just need an additional call to rnp_op_generate_set_request_password later in that function I see.

[rrrooommmaaa]

ok, I'll add that shortly (need to check some cli tests).

[dewyatt]

That does fix the issue. I still think there's a usability issue here in that the user has to enter the password one more time than they should ideally be required to.

[rrrooommmaaa]

they actually would need to enter it 3 times (encrypt primary, decrypt primary, encrypt subkey)

[dewyatt]

@rrrooommmaaa That's what I mean, they shouldn't necessarily need to decrypt the primary, it can be left protected but unlocked (decrypted in memory, but encrypted if written to disk) for this operation.

[dewyatt]

We may want to add an additional API to get rid of that usability issue (something to indicate "leave the primary unlocked please"), but let's not focus on that. I see why it's done the way it is, but there's some room for improvement.

Once we get a PR to fix the lack of protection I'll cut a quick release and try to coordinate updating packages etc.

[ni4]

This should be my fault. Actually this can be done in 2 passes, like it is done in rnp_generate_key_ex() - protect primary key only after subkey is generated and protected.

[dewyatt]

Related: https://github.com/rnpgp/rnp/issues/930 Generally I'm against using the password provider for encrypting (or any time there is enough context at the user call site to avoid it).

Also, for a test, using rnp_key_is_protected in rnp_tests should be sufficient (probably don't need cli_tests here).

[ni4]

@dewyatt I think cli_tests should be updated as well - to make sure that --pass-fd and/or --password CLI parameters worked correctly during the batch generation.

[dewyatt]

Actually this can be done in 2 passes, like it is done in rnp_generate_key_ex() - protect primary key only after subkey is generated and protected.

Good point we shouldn't need any additional API, so long as they aren't saved to disk until protection succeeds for both.

@dewyatt I think cli_tests should be updated as well - to make sure that --pass-fd and/or --password CLI parameters worked correctly during the batch generation.

Probably a good idea!

[dewyatt]

Maybe we should try to split this work up? @rrrooommmaaa The fix @ni4 cli_tests @dewyatt rnp_tests

Yes?

[ni4]

@dewyatt Ok, I'll take cli_tests part.

[ni4]

@rrrooommmaaa @dewyatt Added commit d6e548b2f77d9319f4853f346dd5c68d2984a6f9 to the separate branch, feel free to cherry pick it. It should work fine once generated key is encrypted, but feel free to request changes.

[dewyatt]

@ni4 rnpkeys doesn't seem to take a --password parameter

[ni4]

@dewyatt wow, was sure it is implemented. Will file an issue for this and force-push fixes.

[dewyatt]

@ni4 Do you think that test should write to stdin instead anyways?

EDIT: I mean I think it should probably write to stdin (via run_proc) since that's the currently-implemented functionality (since we don't have --password for rnpkeys).

[ni4]

@dewyatt Yes, this kind of test should be added as well. Probably let's wait for the fixes so it would be easier/faster to make test work as expected.

[dewyatt]

@rrrooommmaaa Are you still working on a fix?

[rrrooommmaaa]

Yes, I can handle this in about 3 hours. If you need this more urgently feel free to re-assign

[dewyatt]

@rrrooommmaaa Ok I think I have a fix for this so I'll get a PR going, I'd like to get this fixed ASAP.