openssh bugs - Oct 2018 - [Bug 2913] New: Reading PEM keys might file if they decrypt to garbage with zero-length passprahse with new OpenSSL 1.1.0i

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

            Bug ID: 2913
           Summary: Reading PEM keys might file if they decrypt to garbage
                    with zero-length passprahse with new OpenSSL 1.1.0i
           Product: Portable OpenSSH
           Version: 7.7p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Keywords: patch
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 3183
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3183&action=edit
proposed patch

Some encrypted PEM keys stopped working from OpenSSH with the new
OpenSSL 1.1.0i, while they still can be used from OpenSSL. The example
key is attached to the Red Hat bugzilla [1].

After some analysis done by Tomas, there is a change in OpenSSL that
supports zero-length passprahse, that are passed by default to the PEM
decryption methods. In a rare case, the padding is decrypted
successfully with this passprahse, the garbage is passed further which
results in fail in OpenSSH.

The correct solution is to create a password callback that returns -1
to overwrite the default passphrase callback as attached in the patch.

With this patch, the ssh tools correctly ask for the passphrase, rather
than failing with invalid key error.

For more information, see the attached bugzilla [1].

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1632902

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

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

Tomas Mraz <t8m at centrum.cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |t8m at centrum.cz
            Summary|Reading PEM keys might file |Reading PEM keys might fail
                   |if they decrypt to garbage  |if they decrypt to garbage
                   |with zero-length passprahse |with zero-length passprahse
                   |with new OpenSSL 1.1.0i     |with new OpenSSL 1.1.0i

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

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

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3183|0                           |1
        is obsolete|                            |

--- Comment #1 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3184
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3184&action=edit
patch v2

Ok, the solution is a bit more complicated. We need to shield also the
zero-lenght passphrases and otherwise pass the passprase from userdata.
With the attached patch I am able to decrypt existing keys as well as
these broken ones without any issues.

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

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Why is this not being considered a bug in OpenSSL?

-- 
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=2913

--- Comment #3 from Tomas Mraz <t8m at centrum.cz> ---
Because zero length encryption keys are technically possible and so it
is not a bug to try to decrypt encrypted data with zero length keys.

Basically openssh here depended on undocumented behavior of openssl and
this patch makes it not to depend on it anymore.

-- 
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=2913

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
It might be undocumented behaviour of OpenSSL (given the state of the
OpenSSL documentation, especially historically, most behaviours are),
but it's been a stable one for >20 years and is demonstrably being
depended upon by consumers of the API.

-- 
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=2913

--- Comment #5 from Tomas Mraz <t8m at centrum.cz> ---
I do not disagree that such change on a stable release cannot be seen
as functionality change breaking API/ABI. On the other hand if such
change was done on release which does not promise to keep the API/ABI
stable it would definitely be acceptable anyway.

So I do not see why this patch should not be applied regardless of
whether the openssl change in patchlevel release is OK or not.

-- 
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=2913

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
FWIW I don't see the new OpenSSL behaviour documented either. Are you
sure it's an intentioned change?

I'm sure OpenSSH isn't the only application that depended on the old
behaviour. Wouldn't it be more sensible for OpenSSL to deal with
zero-length passwords internally; by checking for a plaintext key first
and then attempting to decrypt with a zero-length password?

-- 
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=2913

--- Comment #7 from Tomas Mraz <t8m at centrum.cz> ---
What OpenSSH does is very specific - it depends on returning some
particular error codes when zero length password is passed to OpenSSL
key loading function to distinguish between invalid keys vs. encrypted
keys. I would be very surprised this would be done elsewhere unless
they actually copied the key loading logic from OpenSSH.

Anyway I can report to OpenSSL that this change introduced OpenSSH
regression and see what they have to say about it.

-- 
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=2913

--- Comment #8 from Tomas Mraz <t8m at centrum.cz> ---
Btw. this is the issue that triggered this change in OpenSSL:

https://github.com/openssl/openssl/issues/4716

-- 
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=2913

--- Comment #9 from Tomas Mraz <t8m at centrum.cz> ---
OpenSSL issue:

https://github.com/openssl/openssl/issues/7355

-- 
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=2913

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
Does the patch attached to bug 2901 avoid this?

-- 
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=2913

--- Comment #11 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to Damien Miller from comment #10)
> Does the patch attached to bug 2901 avoid this?
You can try that yourself with the key attached to the bug referenced
in the bug description.

It does not solve the problem. Running the same test with your patch
gives to following:

$ SSH_AUTH_SOCK= ./ssh -i invalid_key -F /dev/null localhost 
error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
error:0907B00D:PEM routines:PEM_read_bio_PrivateKey:ASN1 lib
Load key "invalid_key": invalid format

The reason why is that had_passphrase is 0 (passphrase is zero-length)
and therefore we return SSH_ERR_INVALID_FORMAT from the
translate_libcrypto_error().

-- 
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=2913

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2893


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2893
[Bug 2893] Tracking bug for 7.9 release
-- 
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=2913

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3184|0                           |1
        is obsolete|                            |
                 CC|                            |dtucker at dtucker.net
   Attachment #3190|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #12 from Damien Miller <djm at mindrot.org> ---
Created attachment 3190
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3190&action=edit
tweaked patch; avoid -Wsign-compare problem

-- 
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=2913

--- Comment #13 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to Damien Miller from comment #12)
> Created attachment 3190 [details]
> tweaked patch; avoid -Wsign-compare problem
This looks like wrong patch (this one is from #2912).

-- 
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=2913

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3190|0                           |1
        is obsolete|                            |
   Attachment #3190|ok?(dtucker at dtucker.net)    |
              Flags|                            |
   Attachment #3192|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #14 from Damien Miller <djm at mindrot.org> ---
Created attachment 3192
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3192&action=edit
correct diff

-- 
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=2913

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3192|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

-- 
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=2913

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
This has been committed and will be in OpenSSH 7.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=2913

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
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.