Created attachment 2567 [details] Patch for sshkey.c in OpenSSH 6.7p1 I was trying out the new OpenSSH private key format and I ran into a problem when trying to work with keys encrypted in aes128-gcm and aes256-gcm format. While ssh-keygen encrypted these keys correctly, it was not able to decrypt them. I've identified the problem as an issue with the lengths it passes into cipher_crypt() when dealing with a cipher with integrated MAC support. Steps to reproduce: 1) Create a new format key with a command like: ssh-keygen -t ed25519 -N test -Z aes128-gcm@openssh.com -f new_key 2) Attempt to decrypt this key with a command like: ssh-keygen -p -P test -N '' -f new_key With OpenSSH 6.7p1, this fails with the error "Bad passphrase" for aes128-gcm and aes256-gcm, but works correctly for other ciphers which don't include a built-in MAC. The error happens for all key types when using the new private key format. The error is in the call inside sshkey_parse_private2() where it passes in the length of the encrypted buffer: if ((r = cipher_crypt(&ciphercontext, 0, dp, sshbuf_ptr(decoded), sshbuf_len(decoded), 0, cipher_authlen(cipher))) != 0) { The length here should be encrypted_len, not sshbuf_len(decoded), as that includes the cipher_authlen(cipher) additional MAC bytes. A few additional changes are needed to use encrypted_len safely here and to later properly consume the auth data. I have attached a patch which I believe fixes this problem. With the fix, step 2 above succeeds and properly decrypts the key created in step 1.
Created attachment 2578 [details] Patch for sshkey.c in OpenSSH 6.8p1 This is an update to the patch I submitted which should apply against OpenSSH 6.8p1. The changes are identical and the original patch actually applies cleanly, but with an offset on the line numbers. This fixes that.
I just noticed that this bug also applies when using chacha20-poly1305 to encrypt private keys, and the patch I previously submitted fixes the problem for both GCM and Chacha.
We didn't originally include support for the AEAD ciphers because we couldn't come to a decision on whether the non-encrypted part of the key should be included as "additional authenticated data". Since we can't undo the wrapping of the encrypted part of the key without peeking at the unencrypted data anyway, I think it makes sense not to.
The bug I found actually wasn't related to the additional authenticated data. For this particular case of using the cipher to encrypt an OpenSSH format private key, the additional data is empty in fact. The bug here had to do with the length of the data passed to the cipher_crypt() call when decrypting the key. Instead of passing in the length of the encrypted data, the code is passing in the length of the encrypted data plus the length of the MAC which is placed after it. However, the MAC is not encrypted, since the keys are following the "encrypt then MAC" approach. The fix is to pass in the proper encrypted length, after validating that there are enough bytes in the buffer to hold both this encrypted data and a trailing MAC of the expected size. There was also a bug when consuming the data later than only encrypted_len bytes were consumed, even though it attempted to decrypt more bytes than that. The correct thing here would be to consume encrypted_len bytes plus the length of the MAC, and then check to make sure that no bytes beyond that were remaining in the buffer. The patch I've attached here addresses all of these issues. Only the decrypt code needed any changes. The encrypt function was fine as-is.
Patch applied - will be in OpenSSH 6.9
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1