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

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

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

On Sat, 18 Apr 2020, Lo?c wrote:
> Hi,
> 
> In order to help catching those regressions, here is a new regression
> test file.
Thanks for catching my mistake and thanks even more for writing a regression
test. Both the fix and a slightly-tweaked version of your regress test have
been committed.

-d