openssh bugs - Jul 2020 - [Bug 3190] New: Inconsistent handling of private keys without accompanying public keys

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

            Bug ID: 3190
           Summary: Inconsistent handling of private keys without
                    accompanying public keys
           Product: Portable OpenSSH
           Version: 8.3p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

It comes up from time to time that somebody uses private key without
public key in separate file. OpenSSH is trying to be helpful to read
the separate public key file initially, to prevent decrypting private
keys to early, but currently it is very inconsistent. See the following
steps:

1) generate private key (unencrypted, in openssh format)
    $ ssh-keygen -f /tmp/rsa -N ''

2) remove public part
    $ rm /tmp/rsa.pub

3) ssh-keygen handles this use case well:
    $ ssh-keygen -lf /tmp/rsa

4) We can add the key simply to ssh-agent:
    $ ssh-add /tmp/rsa0

5) Whoops, we can not remove it afterward (this error message is very
confusing since it refers to /tmp/rsa.pub and /tmp/rsa is in place):
    $ ssh-add -d /tmp/rsa
    Bad key file /tmp/rsa: No such file or directory

6) Using the key from ssh gives bogus warnings, even though the key is
used afteward without any issues:
    $ ssh -v -i /tmp/rsa localhost
    [...]
    debug1: identity file /tmp/rsa type -1
    debug1: identity file /tmp/rsa-cert type -1
    [...]
    debug1: Trying private key: /tmp/rsa
    debug1: Authentication succeeded (publickey).

I think the requirement of the separate public key made sense in the
encrypted legacy file formats, but the new OpenSSH file format stores
public key already inside of the private key container and if the key
is not encrypted at all, sidecar file should not be needed either.

I believe we should drop the requirement for separate public key file
at least in these cases and make the above more consistent.

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

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

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3424|                            |ok?
              Flags|                            |

--- Comment #1 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3424
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3424&action=edit
Proposed patch to fall back to alternative methods of getting public
key

It turned out that initial solution is as easy as fixing the logic in
the conditions (see attached patch).

In this function we need to return (goto out) in case we found the key,
not the other way round.

As this code was recently written by Damien, I added him for review.

With the attached patch, keys in openssh format seems to work
correctly. If there would not be anything against, I would like to have
a look also to normal non-encrypted PEM files to take similar approach
and probably add some regression test case to keep this working.

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

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

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
   Attachment #3424|ok?                         |ok?(djm at mindrot.org)
              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=3190

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3424|ok?(djm at mindrot.org)        |
              Flags|                            |
   Attachment #3424|0                           |1
        is obsolete|                            |

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
Comment on attachment 3424
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3424
Proposed patch to fall back to alternative methods of getting public
key

ok, not so fast. It will require some more tweaking.

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

--- Comment #3 from Jakub Jelen <jjelen at redhat.com> ---
It looks like the initial attempt to reproduce the issue was not done
with master, where the new openssh keys work as expected.

But the same does not work with legacy PEM files and this conversion is
not tested at all so this is what the bug/patch will be about.

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

--- Comment #4 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3425
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3425&action=edit
regression test + getting public key from PEM

This is another take with regression test for the
sshkey_parse_pubkey_from_private_fileblob_type() function, adding
support for parsing PEM files and retrieving pubkeys from them.

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

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Created attachment 3428
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3428&action=edit
attempt to load public key from passphraseless private keys

PEM doesn't include the public key in encrypted private keys' cleartext
though, right?

IMO we could load passphrase-free keys, but we should remove their
private elements immediately after loading.

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

--- Comment #6 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to Damien Miller from comment #5)
> Created attachment 3428 [details]
> attempt to load public key from passphraseless private keys
> 
> PEM doesn't include the public key in encrypted private keys'
> cleartext though, right?
right.
> IMO we could load passphrase-free keys, but we should remove their
> private elements immediately after loading.
Right. That was the idea and I think the only missing bit.

But I got a bit confused since had old openssh installed and the
handling of new format was already in master.

Your patch works fine after fixing two minor nits:


 {
        char *pubfile = NULL, *privcmt = NULL;
        int r, oerrno;
-       struct sshkey *privkey;
+       struct sshkey *privkey = NULL;

        if (keyp != NULL)
                *keyp = NULL;


         */
        if ((r = sshkey_load_private(filename, "", &privkey,
&privcmt))
== 0) {
                if ((r = sshkey_from_private(privkey, keyp)) == 0) {
-                       if (commentp != NULL)
+                       if (commentp != NULL) {
                                *commentp = privcmt;
                                privcmt = NULL; /* transferred */
                        }


The only ugly corner case is the removal of key from ssh-agent, which
still fails with cryptic error if the key is encrypted PEM missing
sidecar public key:

    $ ssh-add -d /tmp/rsa
    Bad key file /tmp/rsa: No such file or directory

Otherwise it looks good.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

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