From ff2ffa539c4ce6051fc9010abacb05510bded46f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 2 Aug 2022 18:14:06 +0100 Subject: [PATCH] Windows Pageant: display RSA/DSA cert bit counts. The test in the Pageant list box code for whether we should display the bit count of a key was done by checking specifically for ssh_rsa or ssh_dsa, which of course meant that it didn't catch the certified versions of those keys. Now there's yet another footling ssh_keyalg method that asks the question 'is it worth displaying the bit count?', to which RSA and DSA answer yes, and the opensshcert family delegates to its base key type, so that RSA and DSA certified keys also answer yes. (This isn't the same as ssh_key_public_bits(alg, blob) >= 0. All supported public key algorithms _can_ display a bit count if called on. But only in RSA and DSA is it configurable, and therefore worth bothering to print in the list box.) Also in this commit, I've fixed a bug in the certificate implementation of public_bits, which was passing a wrongly formatted public blob to the underlying key. (Done by factoring out the code from opensshcert_new_shared which constructed the _correct_ public blob, and reusing it in public_bits to do the same job.) --- crypto/dsa.c | 1 + crypto/ecc-ssh.c | 5 +++++ crypto/openssh-certs.c | 50 ++++++++++++++++++++++++++++-------------- crypto/rsa.c | 1 + ssh.h | 5 +++++ utils/nullkey.c | 3 +++ windows/pageant.c | 15 ++++--------- 7 files changed, 52 insertions(+), 28 deletions(-) diff --git a/crypto/dsa.c b/crypto/dsa.c index c1514ac0..bd005957 100644 --- a/crypto/dsa.c +++ b/crypto/dsa.c @@ -511,6 +511,7 @@ const ssh_keyalg ssh_dsa = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = dsa_alg_desc, + .variable_size = nullkey_variable_size_yes, .ssh_id = "ssh-dss", .cache_id = "dss", }; diff --git a/crypto/ecc-ssh.c b/crypto/ecc-ssh.c index feab9d01..6f232c79 100644 --- a/crypto/ecc-ssh.c +++ b/crypto/ecc-ssh.c @@ -1284,6 +1284,7 @@ const ssh_keyalg ssh_ecdsa_ed25519 = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = ec_alg_desc, + .variable_size = nullkey_variable_size_no, .ssh_id = "ssh-ed25519", .cache_id = "ssh-ed25519", .extra = &sign_extra_ed25519, @@ -1312,6 +1313,7 @@ const ssh_keyalg ssh_ecdsa_ed448 = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = ec_alg_desc, + .variable_size = nullkey_variable_size_no, .ssh_id = "ssh-ed448", .cache_id = "ssh-ed448", .extra = &sign_extra_ed448, @@ -1344,6 +1346,7 @@ const ssh_keyalg ssh_ecdsa_nistp256 = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = ec_alg_desc, + .variable_size = nullkey_variable_size_no, .ssh_id = "ecdsa-sha2-nistp256", .cache_id = "ecdsa-sha2-nistp256", .extra = &sign_extra_nistp256, @@ -1376,6 +1379,7 @@ const ssh_keyalg ssh_ecdsa_nistp384 = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = ec_alg_desc, + .variable_size = nullkey_variable_size_no, .ssh_id = "ecdsa-sha2-nistp384", .cache_id = "ecdsa-sha2-nistp384", .extra = &sign_extra_nistp384, @@ -1408,6 +1412,7 @@ const ssh_keyalg ssh_ecdsa_nistp521 = { .supported_flags = nullkey_supported_flags, .alternate_ssh_id = nullkey_alternate_ssh_id, .alg_desc = ec_alg_desc, + .variable_size = nullkey_variable_size_no, .ssh_id = "ecdsa-sha2-nistp521", .cache_id = "ecdsa-sha2-nistp521", .extra = &sign_extra_nistp521, diff --git a/crypto/openssh-certs.c b/crypto/openssh-certs.c index 351b17d6..e37e42b2 100644 --- a/crypto/openssh-certs.c +++ b/crypto/openssh-certs.c @@ -217,6 +217,7 @@ static unsigned opensshcert_supported_flags(const ssh_keyalg *self); static const char *opensshcert_alternate_ssh_id(const ssh_keyalg *self, unsigned flags); static char *opensshcert_alg_desc(const ssh_keyalg *self); +static bool opensshcert_variable_size(const ssh_keyalg *self); static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, const ssh_keyalg *base); @@ -271,6 +272,7 @@ static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, .supported_flags = opensshcert_supported_flags, \ .alternate_ssh_id = opensshcert_alternate_ssh_id, \ .alg_desc = opensshcert_alg_desc, \ + .variable_size = opensshcert_variable_size, \ .related_alg = opensshcert_related_alg, \ .ssh_id = ssh_alg_id_prefix "-cert-v01@openssh.com", \ .cache_id = "opensshcert-" ssh_key_id_prefix, \ @@ -287,6 +289,26 @@ static const ssh_keyalg *const opensshcert_all_keyalgs[] = { }; #undef KEYALG_LIST_ENTRY +static strbuf *get_base_public_blob(BinarySource *src, + const opensshcert_extra *extra) +{ + strbuf *basepub = strbuf_new(); + put_stringz(basepub, extra->base_key_ssh_id); + + /* Make the base public key blob out of the public key + * material in the certificate. This invocation of the + * blobtrans system doesn't do any format translation, but it + * does ensure that the right amount of data is copied so that + * src ends up in the right position to read the remaining + * certificate fields. */ + BLOBTRANS_DECLARE(bt); + blobtrans_read(bt, src, extra->pub_fmt); + blobtrans_write(bt, BinarySink_UPCAST(basepub), extra->pub_fmt); + blobtrans_clear(bt); + + return basepub; +} + static opensshcert_key *opensshcert_new_shared( const ssh_keyalg *self, ptrlen blob, strbuf **basepub_out) { @@ -304,21 +326,7 @@ static opensshcert_key *opensshcert_new_shared( ck->sshk.vt = self; ck->nonce = strbuf_dup(get_string(src)); - strbuf *basepub = strbuf_new(); - { - put_stringz(basepub, extra->base_key_ssh_id); - - /* Make the base public key blob out of the public key - * material in the certificate. This invocation of the - * blobtrans system doesn't do any format translation, but it - * does ensure that the right amount of data is copied so that - * src ends up in the right position to read the remaining - * certificate fields. */ - BLOBTRANS_DECLARE(bt); - blobtrans_read(bt, src, extra->pub_fmt); - blobtrans_write(bt, BinarySink_UPCAST(basepub), extra->pub_fmt); - blobtrans_clear(bt); - } + strbuf *basepub = get_base_public_blob(src, extra); ck->serial = get_uint64(src); ck->type = get_uint32(src); ck->key_id = strbuf_dup(get_string(src)); @@ -877,8 +885,11 @@ static int opensshcert_pubkey_bits(const ssh_keyalg *self, ptrlen blob) get_string(src); /* key type */ get_string(src); /* nonce */ - return ssh_key_public_bits( - self->base_alg, make_ptrlen(get_ptr(src), get_avail(src))); + strbuf *basepub = get_base_public_blob(src, self->extra); + int bits = ssh_key_public_bits( + self->base_alg, ptrlen_from_strbuf(basepub)); + strbuf_free(basepub); + return bits; } static unsigned opensshcert_supported_flags(const ssh_keyalg *self) @@ -908,6 +919,11 @@ static char *opensshcert_alg_desc(const ssh_keyalg *self) return our_desc; } +static bool opensshcert_variable_size(const ssh_keyalg *self) +{ + return ssh_keyalg_variable_size(self->base_alg); +} + static const ssh_keyalg *opensshcert_related_alg(const ssh_keyalg *self, const ssh_keyalg *base) { diff --git a/crypto/rsa.c b/crypto/rsa.c index 88748288..2196d1ec 100644 --- a/crypto/rsa.c +++ b/crypto/rsa.c @@ -883,6 +883,7 @@ static const struct ssh2_rsa_extra .base_key = nullkey_base_key, \ .pubkey_bits = rsa2_pubkey_bits, \ .alg_desc = rsa2_alg_desc, \ + .variable_size = nullkey_variable_size_yes, \ .cache_id = "rsa2" const ssh_keyalg ssh_rsa = { diff --git a/ssh.h b/ssh.h index fb9d7453..bd639566 100644 --- a/ssh.h +++ b/ssh.h @@ -858,6 +858,7 @@ struct ssh_keyalg { unsigned (*supported_flags) (const ssh_keyalg *self); const char *(*alternate_ssh_id) (const ssh_keyalg *self, unsigned flags); char *(*alg_desc)(const ssh_keyalg *self); + bool (*variable_size)(const ssh_keyalg *self); /* The following methods can be NULL if !is_certificate */ const ssh_keyalg *(*related_alg)(const ssh_keyalg *self, const ssh_keyalg *base); @@ -928,6 +929,8 @@ static inline const char *ssh_keyalg_alternate_ssh_id( { return self->alternate_ssh_id(self, flags); } static inline char *ssh_keyalg_desc(const ssh_keyalg *self) { return self->alg_desc(self); } +static inline bool ssh_keyalg_variable_size(const ssh_keyalg *self) +{ return self->variable_size(self); } static inline const ssh_keyalg *ssh_keyalg_related_alg( const ssh_keyalg *self, const ssh_keyalg *base) { return self->related_alg(self, base); } @@ -936,6 +939,8 @@ static inline const ssh_keyalg *ssh_keyalg_related_alg( unsigned nullkey_supported_flags(const ssh_keyalg *self); const char *nullkey_alternate_ssh_id(const ssh_keyalg *self, unsigned flags); ssh_key *nullkey_base_key(ssh_key *key); +bool nullkey_variable_size_no(const ssh_keyalg *self); +bool nullkey_variable_size_yes(const ssh_keyalg *self); /* Utility functions implemented centrally */ ssh_key *ssh_key_clone(ssh_key *key); diff --git a/utils/nullkey.c b/utils/nullkey.c index 1b01c891..dae5c1bb 100644 --- a/utils/nullkey.c +++ b/utils/nullkey.c @@ -17,3 +17,6 @@ ssh_key *nullkey_base_key(ssh_key *key) /* When a key is not certified, it is its own base */ return key; } + +bool nullkey_variable_size_no(const ssh_keyalg *self) { return false; } +bool nullkey_variable_size_yes(const ssh_keyalg *self) { return true; } diff --git a/windows/pageant.c b/windows/pageant.c index aea98389..05447f5f 100644 --- a/windows/pageant.c +++ b/windows/pageant.c @@ -372,18 +372,11 @@ static void keylist_update_callback( put_datapl(disp->alg, keytype_word); } - put_datapl(disp->bits, ptrlen_get_word(&fingerprint, " ")); - put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " ")); + ptrlen bits_word = ptrlen_get_word(&fingerprint, " "); + if (ssh_keyalg_variable_size(alg)) + put_datapl(disp->bits, bits_word); - /* - * But we don't display the bit count if the algorithm isn't - * one of the ones where it can vary. That way, those - * algorithm names (which are generally longer) can safely - * overlap into the bits column without colliding with - * pointless text. - */ - if (!(alg == &ssh_dsa || alg == &ssh_rsa)) - strbuf_clear(disp->bits); + put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " ")); } }