GNOME Bugzilla – Bug 729381
[review] jk/crypto-fixes: make a few enhancements for crypto functions
Last modified: 2014-05-12 09:38:25 UTC
The patch * makes better errors for 802-1x setting * avoid calling crypto_md5()_hash with password_len == 0 * enables AES cipher for decrypting private keys
(In reply to comment #0) > * enables AES cipher for decrypting private keys So, this patch doesn't require any careful security analysis on our part, right? (In particular, the fact that you extended make_des_key() to do AES keys rather than writing a new make_aes_key() is because that's just the way that x509 private keys are stored, and so we have to use that exact algorithm, right?) If so, then it all looks good, except that you should probably add an AES-based test to test-crypto.
> libnm-util: do not call crypto_md5_hash() for empty passwords instead of returning on empty password, shouldn't we adjust crypto_md5_hash() to accept empty passwords? I think that the user enters the password to "decrypt", so we should also support empty passwords. If the user enters a password to encrypt something, we might do some check for password-strength, but that also should be done earlier after reading the input.
(In reply to comment #1) > (In reply to comment #0) > > * enables AES cipher for decrypting private keys > > So, this patch doesn't require any careful security analysis on our part, > right? (In particular, the fact that you extended make_des_key() to do AES keys > rather than writing a new make_aes_key() is because that's just the way that > x509 private keys are stored, and so we have to use that exact algorithm, > right?) > > If so, then it all looks good, except that you should probably add an AES-based > test to test-crypto. I second that question :) Do you have any test keys, that are encrypted with CIPHER_AES_CBC? Some tests would be nice.
> libnm-util: return better error messages on failures for _set_ functions "has to be" -> "must be" The rest looks good!
(In reply to comment #1) > (In reply to comment #0) > > * enables AES cipher for decrypting private keys > > So, this patch doesn't require any careful security analysis on our part, > right? (In particular, the fact that you extended make_des_key() to do AES keys > rather than writing a new make_aes_key() is because that's just the way that > x509 private keys are stored, and so we have to use that exact algorithm, > right?) I don't think it would require any extensive analysis since we're just adding another mechanism to verify private key passwords. We obviously must add the testcases to ensure that we are processing things correctly and not allowing invalid passwords to verify against the private key. > If so, then it all looks good, except that you should probably add an AES-based > test to test-crypto. Agreed, we need testcases for the AES stuff. At least a good-password test and a bad-password test.
(In reply to comment #4) > > libnm-util: return better error messages on failures for _set_ functions > > "has to be" -> "must be" Fixed. (In reply to comment #5) > (In reply to comment #1) > > (In reply to comment #0) > > > * enables AES cipher for decrypting private keys > > > > So, this patch doesn't require any careful security analysis on our part, > > right? (In particular, the fact that you extended make_des_key() to do AES keys > > rather than writing a new make_aes_key() is because that's just the way that > > x509 private keys are stored, and so we have to use that exact algorithm, > > right?) > > I don't think it would require any extensive analysis since we're just adding > another mechanism to verify private key passwords. We obviously must add the > testcases to ensure that we are processing things correctly and not allowing > invalid passwords to verify against the private key. > > > If so, then it all looks good, except that you should probably add an AES-based > > test to test-crypto. > > Agreed, we need testcases for the AES stuff. At least a good-password test and > a bad-password test. The AES is quite common and I tested it with several keys I have. The key pair can be easily generated using: ssh-keygen -t rsa -f test-key I have updated the branch with these changes: * added a testcase for AES * added nm_utils_rsa_key_encrypt_aes() and allowing encryption with AES
Looks good to me.
Commits in master: 810e934 libnm-util: add nm_utils_rsa_key_encrypt_aes() encrypting RSA key with AES b3e39d4 libnm-util: allow AES cipher for private keys a9f5494 libnm-util: do not call crypto_md5_hash() for empty passwords 31cd3fe libnm-util: return better error messages on failures for _set_ functions