openssh unix dev - Apr 2020 - [PATCH] regression of comment extraction in private key file without passphrase

Hello,

In one recent change
(https://anongit.mindrot.org/openssh.git/commit/?id=2b13d3934d5803703c04803ca3a93078ecb5b715),
I noticed a regression.

If ssh-keygen is given a private file without passphrase and without the
corresponding .pub file, I doesn't extract the comment after the commit,
while it did before:

Before the commit:

$ ./ssh-keygen -q -t dsa -N '' -C foobar -f test_dsa
$ ./ssh-keygen -l -f test_dsa
1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)
$ rm test_dsa.pub
$ ./ssh-keygen -l -f test_dsa
1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)

Last command after the commit:

$ ./ssh-keygen -l -f test_dsa
1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk no comment (DSA)

It is due to the fact that the 'sshkey_load_public' function is now
finishing by sshkey_load_public_from_private, which is not failing on a
(new format) private file. Previously, if did fail and so the
fingerprint_private function was calling sshkey_load_private without
passphrase as a fallback.


I suggest to move the fallback inside the sshkey_load_public, so to call
the sshkey_load_private without passphrase in the sshkey_load_public
before extracting the public key from the private file.

Here is the suggested patch below.

I'm open to any suggestion on the correct fix to do.

And other idea would be to add the comment in the public key part of the
private key file (new format). I think it is a good idea, the comment
isn't private any way since it is present in the public key file.

I'm ok to implement this idea if you think it is worth it. Or any other
suggestion.


Note that the upper commit is very useful because it does extract the
fingerprint from a private file with passphrase while previously
ssh-keygen failed with the unsatisfying error "test_dsa is not a key
file". Thanks for it !

Regards

Lo?c

---
?authfile.c?? | 5 +++++
?ssh-keygen.c | 6 +-----
?2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/authfile.c b/authfile.c
index 50fa48e4a3b6..9e6e2a00a896 100644
--- a/authfile.c
+++ b/authfile.c
@@ -304,6 +304,11 @@ sshkey_load_public(const char *filename, struct
sshkey **keyp, char **commentp)
???? if ((r = sshkey_try_load_public(keyp, pubfile, commentp)) == 0)
???? ??? goto out;
?
+??? /* If the comment is wanted, try loading the private key with no
passphrase,

+??? ??? since it contains the comment while the public key in the
private file doesn't */
+??? if (commentp != NULL && (r = sshkey_load_private(filename, NULL,
keyp, commentp)) == 0)
+??? ??? goto out;
+
???? /* finally, try to extract public key from private key file */
???? if ((r = sshkey_load_pubkey_from_private(filename, keyp)) == 0)
???? ??? goto out;
diff --git a/ssh-keygen.c b/ssh-keygen.c
index 802fd25c286f..cb7872fb9165 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -917,11 +917,7 @@ fingerprint_private(const char *path)
???? ??? fatal("%s: %s", path, strerror(errno));
???? if ((r = sshkey_load_public(path, &public, &comment)) != 0) {
???? ??? debug("load public \"%s\": %s", path, ssh_err(r));
-??? ??? if ((r = sshkey_load_private(path, NULL,
-??? ??? ??? &public, &comment)) != 0) {
-??? ??? ??? debug("load private \"%s\": %s", path,
ssh_err(r));
-??? ??? ??? fatal("%s is not a key file.", path);
-??? ??? }
+??? ??? fatal("%s is not a key file.", path);
???? }
?
???? fingerprint_one_key(public, comment);
-- 
2.17.1

On Wed, 15 Apr 2020, Lo?c wrote:
> Hello,
> 
> In one recent change
>
(https://anongit.mindrot.org/openssh.git/commit/?id=2b13d3934d5803703c04803ca3a93078ecb5b715),
> I noticed a regression.
> 
> If ssh-keygen is given a private file without passphrase and without the
> corresponding .pub file, I doesn't extract the comment after the
commit,
> while it did before:
> 
> Before the commit:
> 
> $ ./ssh-keygen -q -t dsa -N '' -C foobar -f test_dsa
> $ ./ssh-keygen -l -f test_dsa
> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)
> $ rm test_dsa.pub
> $ ./ssh-keygen -l -f test_dsa
> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)
> 
> Last command after the commit:
> 
> $ ./ssh-keygen -l -f test_dsa
> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk no comment (DSA)
> 
> It is due to the fact that the 'sshkey_load_public' function is now
> finishing by sshkey_load_public_from_private, which is not failing on a
> (new format) private file. Previously, if did fail and so the
> fingerprint_private function was calling sshkey_load_private without
> passphrase as a fallback.
> 
> 
> I suggest to move the fallback inside the sshkey_load_public, so to call
> the sshkey_load_private without passphrase in the sshkey_load_public
> before extracting the public key from the private file.
> 
> Here is the suggested patch below.
IMO it's easier to flip the order of operations in
ssh-keygen.c:fingerprint_private():

diff --git a/ssh-keygen.c b/ssh-keygen.c
index dd676c0..8159821 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -890,22 +890,21 @@ fingerprint_private(const char *path)
 {
 	struct stat st;
 	char *comment = NULL;
-	struct sshkey *public = NULL;
+	struct sshkey *key = NULL;
 	int r;
 
 	if (stat(identity_file, &st) == -1)
 		fatal("%s: %s", path, strerror(errno));
-	if ((r = sshkey_load_public(path, &public, &comment)) != 0) {
-		debug("load public \"%s\": %s", path, ssh_err(r));
-		if ((r = sshkey_load_private(path, NULL,
-		    &public, &comment)) != 0) {
-			debug("load private \"%s\": %s", path, ssh_err(r));
+	if ((r = sshkey_load_private(path, NULL, &key, &comment)) != 0) {
+		debug("load private \"%s\": %s", path, ssh_err(r));
+		if ((r = sshkey_load_public(path, &key, &comment)) != 0) {
+			debug("load public \"%s\": %s", path, ssh_err(r));
 			fatal("%s is not a key file.", path);
 		}
 	}
 
-	fingerprint_one_key(public, comment);
-	sshkey_free(public);
+	fingerprint_one_key(key, comment);
+	sshkey_free(key);
 	free(comment);
 }

Hi

Le 17/04/2020 ? 05:52, Damien Miller a ?crit?:
> On Wed, 15 Apr 2020, Lo?c wrote:
>
>> Hello,
>>
>> In one recent change
>>
(https://anongit.mindrot.org/openssh.git/commit/?id=2b13d3934d5803703c04803ca3a93078ecb5b715),
>> I noticed a regression.
>>
>> If ssh-keygen is given a private file without passphrase and without
the
>> corresponding .pub file, I doesn't extract the comment after the
commit,
>> while it did before:
>>
>> Before the commit:
>>
>> $ ./ssh-keygen -q -t dsa -N '' -C foobar -f test_dsa
>> $ ./ssh-keygen -l -f test_dsa
>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)
>> $ rm test_dsa.pub
>> $ ./ssh-keygen -l -f test_dsa
>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk foobar (DSA)
>>
>> Last command after the commit:
>>
>> $ ./ssh-keygen -l -f test_dsa
>> 1024 SHA256:/E/JUVD3FO4vHYs+8RfXJW+ah4H4bHcBrCRKFcfZSJk no comment
(DSA)
>>
>> It is due to the fact that the 'sshkey_load_public' function is
now
>> finishing by sshkey_load_public_from_private, which is not failing on a
>> (new format) private file. Previously, if did fail and so the
>> fingerprint_private function was calling sshkey_load_private without
>> passphrase as a fallback.
>>
>>
>> I suggest to move the fallback inside the sshkey_load_public, so to
call
>> the sshkey_load_private without passphrase in the sshkey_load_public
>> before extracting the public key from the private file.
>>
>> Here is the suggested patch below.
> IMO it's easier to flip the order of operations in
> ssh-keygen.c:fingerprint_private():
Yes, your patch is simpler. Unfortunately, it also has a regression when
the private key file is in the old format which doesn't contain the
comment and when the .pub is not removed which is a more common case
probably:

On latest git:

$ ./ssh-keygen -q -m PEM -t dsa -N '' -C foobar -f test_dsa
$ ./ssh-keygen -l -f test_dsa
1024 SHA256:Yqp+0QYlbsfJotozWtbWVHv+WAAu2PEFwo2ZTeRPzv8 no comment (DSA)

With openssh version 8.2:

$ ssh-keygen -l -f test_dsa
1024 SHA256:Yqp+0QYlbsfJotozWtbWVHv+WAAu2PEFwo2ZTeRPzv8 foobar (DSA)

Best regards

Lo?c