[PATCH] regression of comment extraction in private key file without passphrase

[Damien Miller]

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);
 }