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

[Loïc]

On 17/04/2020, Loïc wrote:
> On 17/04/2020, Damien Miller wrote :
>> 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)


Hi,

In order to help catching those regressions, here is a new regression
test file.

It is successful on V_8_2 tag of the git but failing on master
(https://anongit.mindrot.org/openssh.git/commit/?id=32f2d0aad42c15e19bd3b07496076ca891573a58),
and differently on
https://anongit.mindrot.org/openssh.git/commit/?id=094dd513f4b42e6a3cebefd18d1837eb709b4d99

Hope it helps.


Subject: [PATCH] Add regression test for comment extraction from private
key file

---
 regress/keygen-comment.sh | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 regress/keygen-comment.sh

diff --git a/regress/keygen-comment.sh b/regress/keygen-comment.sh
new file mode 100644
index 000000000000..6bd3a69e2857
--- /dev/null
+++ b/regress/keygen-comment.sh
@@ -0,0 +1,55 @@
+#    Placed in the Public Domain.
+
+tid="Comment extraction from private key"
+
+S1="secret1"
+
+check_fingerprint () {
+    file="$1"
+    comment="$2"
+    CMD="${SSHKEYGEN} -l -E sha256 -f $file"
+    $CMD > $OBJ/$t-fgp
+    if [ $? -ne 0 ]; then
+        fail "ssh-keygen -l failed for $t-key"
+    fi
+    grep -qE "^([0-9]+) SHA256:(.){43} ${comment} \(.*\)$" $OBJ/$t-fgp
+    if [ $? -ne 0 ]; then
+        trace "Expected comment is: \"${comment}\""
+        trace "ssh-keygen -l output is \"$( cat $OBJ/$t-fgp )\""
+        fail "comment is not correctly recovered for $t-key"
+    fi
+    rm -f $OBJ/$t-fgp
+}
+
+for fmt in '' RFC4716 PKCS8 PEM; do
+    for t in $SSH_KEYTYPES; do
+        trace "generating $t key in '$fmt' format"
+        rm -f $OBJ/$t-key
+        comment="foo bar"
+        ${SSHKEYGEN} -q ${fmt:+-m} $fmt -N '' -C "${comment}" -t $t -f
$OBJ/$t-key
+        if [ $? -eq 0 ]; then
+            if [ "$fmt" = "PKCS8" ] || [ "$fmt" = "PEM" ] && grep -q --
"-----BEGIN OPENSSH PRIVATE KEY-----" $OBJ/$t-key ; then
+                # some key types like ed25519 and *@openssh.com are never
+                # stored in old formats, it is not useful to test them
twice
+                continue
+            fi
+            # fingerprint using public key file if available
+            trace "fingerprinting $t key"
+            check_fingerprint $OBJ/$t-key "${comment}"
+            # fingerprint using public key file forced
+            trace "fingerprinting $t key using public key file"
+            check_fingerprint $OBJ/$t-key.pub "${comment}"
+            # Output fingerprint using only private file
+            trace "fingerprinting $t key using private key file"
+            rm -f $OBJ/$t-key.pub
+            if [ "$fmt" = "PKCS8" ] || [ "$fmt" = "PEM" ]; then
+                comment="no comment"
+                trace "comment cannot be recovered in the $fmt format
private key"
+            fi
+            check_fingerprint $OBJ/$t-key "${comment}"
+        else
+            fail "ssh-keygen for $t-key failed"
+        fi
+        rm -f $OBJ/$t-key $OBJ/$t-key.pub
+    done
+done
-- 
2.17.1