openssh bugs - Mar 2015 - [Bug 2366] New: ssh-keygen doesn't correctly decode new format GCM-encrypted keys

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

            Bug ID: 2366
           Summary: ssh-keygen doesn't correctly decode new format
                    GCM-encrypted keys
           Product: Portable OpenSSH
           Version: 6.7p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh-keygen
          Assignee: unassigned-bugs at mindrot.org
          Reporter: ronf at timeheart.net

Created attachment 2567
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2567&action=edit
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 at 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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

--- Comment #1 from Ron Frederick <ronf at timeheart.net> ---
Created attachment 2578
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2578&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

--- Comment #2 from Ron Frederick <ronf at timeheart.net> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2360
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

--- Comment #4 from Ron Frederick <ronf at timeheart.net> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Patch applied - will be in OpenSSH 6.9

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2366

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.