Bug 3190 - Inconsistent handling of private keys without accompanying public keys
Summary: Inconsistent handling of private keys without accompanying public keys
Status: ASSIGNED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 8.3p1
Hardware: Other Linux
: P5 enhancement
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-03 00:48 AEST by Jakub Jelen
Modified: 2020-11-27 13:49 AEDT (History)
1 user (show)

See Also:


Attachments
Proposed patch to fall back to alternative methods of getting public key (666 bytes, patch)
2020-07-03 04:35 AEST, Jakub Jelen
no flags Details | Diff
regression test + getting public key from PEM (6.84 KB, patch)
2020-07-03 17:48 AEST, Jakub Jelen
no flags Details | Diff
attempt to load public key from passphraseless private keys (2.53 KB, patch)
2020-07-17 14:12 AEST, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelen 2020-07-03 00:48:42 AEST
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.
Comment 1 Jakub Jelen 2020-07-03 04:35:44 AEST
Created attachment 3424 [details]
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.
Comment 2 Jakub Jelen 2020-07-03 06:15:22 AEST
Comment on attachment 3424 [details]
Proposed patch to fall back to alternative methods of getting public key

ok, not so fast. It will require some more tweaking.
Comment 3 Jakub Jelen 2020-07-03 06:30:04 AEST
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.
Comment 4 Jakub Jelen 2020-07-03 17:48:48 AEST
Created attachment 3425 [details]
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.
Comment 5 Damien Miller 2020-07-17 14:12:37 AEST
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?

IMO we could load passphrase-free keys, but we should remove their private elements immediately after loading.
Comment 6 Jakub Jelen 2020-07-17 18:00:02 AEST
(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.