Bug 2522

Summary: Key parser should reflect errors from OpenSSL
Product: Portable OpenSSH Reporter: Jakub Jelen <jjelen>
Component: sshAssignee: Damien Miller <djm>
Status: CLOSED FIXED    
Severity: minor CC: djm, dtucker, horsley1953
Priority: P3 Keywords: patch
Version: 7.1p1   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 2523, 2647    
Attachments:
Description Flags
proposed patch
none
translate libcrypto error code on PEM_read_bio_PrivateKey failure
dtucker: ok+
Do not fallback to PEM parser, if only passphrase is wrong dtucker: ok+

Description Jakub Jelen 2016-01-09 03:15:29 AEDT 
Created attachment 2781 [details]
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
Comment 1 Damien Miller 2016-04-08 16:24:09 AEST 
Created attachment 2806 [details]
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.
Comment 2 Jakub Jelen 2016-04-08 19:32:48 AEST 
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.
Comment 3 Damien Miller 2016-06-17 15:06:58 AEST 
path applied. Will be released in openssh-7.3 - thanks!
Comment 4 Damien Miller 2016-06-27 15:33:02 AEST 
change was reverted as it caused regressions in pubkey auth. Needs more investigation
Comment 5 Jakub Jelen 2016-06-27 17:09:01 AEST 
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?
Comment 6 Darren Tucker 2016-06-29 11:49:45 AEST 
(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
Comment 7 Jakub Jelen 2016-06-29 20:41:33 AEST 
Created attachment 2844 [details]
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.
Comment 8 Damien Miller 2016-07-22 14:10:59 AEST 
retarget unfinished bugs to next release
Comment 9 Damien Miller 2016-07-22 14:14:48 AEST 
retarget unfinished bugs to next release
Comment 10 Damien Miller 2016-07-22 14:15:40 AEST 
retarget unfinished bugs to next release
Comment 11 Damien Miller 2016-07-22 14:17:11 AEST 
retarget unfinished bugs to next release
Comment 12 Damien Miller 2016-12-16 14:31:15 AEDT 
OpenSSH 7.4 release is closing; punt the bugs to 7.5
Comment 13 Damien Miller 2017-02-10 14:22:11 AEDT 
Comment on attachment 2844 [details]
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.
Comment 14 Darren Tucker 2017-02-10 15:08:50 AEDT 
Comment on attachment 2844 [details]
Do not fallback to PEM parser, if only passphrase is wrong

third time's a charm?
Comment 15 Damien Miller 2017-02-10 15:36:20 AEDT 
Applied and will be in openssh-7.5. Thanks!

commit 155d540d00ff55f063421ec182ec8ff2b7ab6cbe
Author: djm@openbsd.org <djm@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
Comment 16 Damien Miller 2017-02-10 15:36:49 AEDT 
*** Bug 2523 has been marked as a duplicate of this bug. ***
Comment 17 Damien Miller 2018-04-06 12:26:40 AEST 
Close all resolved bugs after release of OpenSSH 7.7.