openssh bugs - Jan 2016 - [Bug 2522] New: Key parser should reflect errors from OpenSSL

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

            Bug ID: 2522
           Summary: Key parser should reflect errors from OpenSSL
           Product: Portable OpenSSH
           Version: 7.1p1
          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 2781
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2781&action=edit
proposed patch

TL;DR

    ./ssh-add <(echo "")

asks for passphrase for empty key. The blob is passed to OpenSSL and
every parser failure is reported as "wrong passphrase", which is
broken. Checking the actual error from OpenSSL would solve this issue.


Long story:

Based on Red hat Bugzilla [1], my post on mailing list [2] and SO
question [3] I went down the source code and investigated a bit
communication between OpenSSH and OpenSSL in terms of private key
parsing.

When openSSH can not derive the key type it passes the whole blob to
OpenSSL function and waits if it can parse it. 

    PEM_read_bio_PrivateKey()      # called from sshkey.c @ 3791

Only return value from this function is NULL on failure. The reason can
be obtained using ERR_ functions from OpenSSL

    ERR_get_error();               # actual error code
    ERR_print_errors_fp(stderr);   # prints verbose info

Possible reasons for failure of the above mentioned function are
described in openssl/pem.h [4]. Basically, some of the reasons are
obviously not related to wrong passphrase. I tried with somehow
crippled keys and I ended up with attached patch so far (currently
contains also debug output from openssl on failure). The proposed patch
does not solve only the issue with empty key and "crippled" one, but
also problems with unknown RSA1 keys when built without SSH1 protocol:

    $ ./ssh-add <(echo "")
   
140239613359768:error:0906D06C:lib(9):func(109):reason(108):pem_lib.c:701:Expecting:
ANY PRIVATE KEY
    Error loading key "/dev/fd/63": invalid format
    $ ./ssh-add /tmp/rsa
   
140408665470616:error:0906D066:lib(9):func(109):reason(102):pem_lib.c:809:
    Error loading key "/tmp/rsa": invalid format
    $ ./ssh-add /tmp/rsa1
   
140632696993432:error:0906D06C:lib(9):func(109):reason(108):pem_lib.c:701:Expecting:
ANY PRIVATE KEY
    Error loading key "/tmp/rsa1": invalid format

Let me know if there is something not clear and if it can be included
in the next release.


[1] https://bugzilla.redhat.com/show_bug.cgi?id=1282423
[2]
http://lists.mindrot.org/pipermail/openssh-unix-dev/2015-December/034617.html
[3] http://unix.stackexchange.com/q/251194/121504
[4]
https://github.com/openssl/openssl/blob/master/include/openssl/pem.h#L509

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2781|0                           |1
        is obsolete|                            |
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at zip.com.au
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
             Status|NEW                         |ASSIGNED
   Attachment #2806|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 2806
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2806&action=edit
translate libcrypto error code on PEM_read_bio_PrivateKey failure

This patch translates a few more error codes, making that function
return either SSH_ERR_KEY_WRONG_PASSPHRASE, SSH_ERR_INVALID_FORMAT or
SSH_ERR_LIBCRYPTO_ERROR.

It also uses ERR_peek_last_error() which AFAIK is what we want.
ERR_peek_error() might be confused by prior soft libcrypto errors in
that process/thread's life.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2543


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2523


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2523
[Bug 2523] An RSA private key file consistently gives "Badd
Passphrase"
errors, but worked before
-- 
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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P5                          |P3
           Severity|enhancement                 |minor

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

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
Thanks (it was probably my first encounter with openssl API so excuse
the brevity of my patch). Your looks good to me and works fine to the
extent I was able to try.

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

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2806|ok?(dtucker at zip.com.au)     |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=2522

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
path applied. Will be released in openssh-7.3 - thanks!

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
change was reverted as it caused regressions in pubkey auth. Needs more
investigation

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

--- Comment #5 from Jakub Jelen <jjelen at redhat.com> ---
I saw that reverted in the upstream CVS, but quite without any details,
about use case which it breaks. Is there some more information I can
have a look into or help?

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

--- Comment #6 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Jakub Jelen from comment #5)
> I saw that reverted in the upstream CVS, but quite without any
> details, about use case which it breaks. Is there some more
> information I can have a look into or help?
The breakage seems to occur with keys that are new-format rather than
PEM (ie generated with "ssh-keygen -o" or any ED25519 keys).

one reproduction case was trying to read the key to convert it with
ssh-keygen.

$ ssh-keygen -y -f .ssh/id_rsa -vvv
Load key ".ssh/id_bert": invalid format

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

--- Comment #7 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2844
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2844&action=edit
Do not fallback to PEM parser, if only passphrase is wrong

Thank you for the details. Now I see. When the key in new format have a
passphrase, it is not accepted.

There is assumption that sshkey_load_private returns
SSH_ERR_KEY_WRONG_PASSPHRASE when the initial attempt without
passphrase fails (which quite makes sense).

But parsing the key in  sshkey_parse_private_fileblob_type  goes
through waterfall from parsing openssh format to parsing PEM format.

Current implementation depends on the assumption that 
sshkey_parse_private_pem_fileblob  returns WRONG_PASSPHRASE for
everything that it does not know which gets wrong with the above patch
applied (sorry that I did miss that).

Proper solution would be to pass-through WRONG_PASSPHRASE return value
from sshkey_parse_private2, which indicates that the parser knows the
key type, but the passphrase it wrong and do not let it fall-through to
the PEM parser (which does not know the key format).

I ran the tests from initial bug, from the last comments and the the
regress and all test passed.

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

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next 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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
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=2522

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release

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

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next 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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2543                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2647

--- Comment #12 from Damien Miller <djm at mindrot.org> ---
OpenSSH 7.4 release is closing; punt the bugs to 7.5


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2594                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 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=2522

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2844|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2844
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2844
Do not fallback to PEM parser, if only passphrase is wrong

Thanks for tracking this down. The fix looks fine to me and, with the
error code translation re-applied, passes the test-cases that were
failing previously.

I'd like to commit this and the original error code translation.

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

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2844|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #14 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2844
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2844
Do not fallback to PEM parser, if only passphrase is wrong

third time's a charm?

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

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Applied and will be in openssh-7.5. Thanks!

commit 155d540d00ff55f063421ec182ec8ff2b7ab6cbe
Author: djm at openbsd.org <djm at openbsd.org>
Date:   Fri Feb 10 04:34:50 2017 +0000

    upstream commit

    bring back r1.34 that was backed out for problems loading
    public keys:

    translate OpenSSL error codes to something more
    meaninful; bz#2522 reported by Jakub Jelen, ok dtucker@

    with additional fix from Jakub Jelen to solve the backout.
    bz#2525 bz#2523 re-ok dtucker@

    Upstream-ID: a9d5bc0306f4473d9b4f4484f880e95f3c1cc031

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |horsley1953 at gmail.com

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
*** Bug 2523 has been marked as a duplicate of this bug. ***

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

Damien Miller <djm at mindrot.org> changed:

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

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #17 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

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