From e37c2e184448f64e285f925ab9636b5f21be99f7 Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Fri, 13 Nov 2020 15:43:30 +0100 Subject: [PATCH] gpg: Fix the encrypt+sign hash algo preference selection for ECDSA. * g10/keydb.h (pref_hint): Change from union to struct and add field 'exact'. Adjust callers. * g10/pkclist.c (algo_available): Take care of the exact hint. * g10/sign.c (sign_file): Rework the hash detection from recipient prefs. -- This fixes a encrypt+sign case like: One recipient key has SHA512 as highest ranked hash preference but the the signing key is a 256 bit curve. Because we don't want to use a truncated hash with ECDSA, we need to have an exact match - this is in particular important for smartcard which check that the hash matches the curves. Signed-off-by: Werner Koch Ported-from-stable: aeed0b93ff660fe271d8f98f8d5ce60aa5bf3ebe --- doc/HACKING | 1 + g10/keydb.h | 9 ++++---- g10/pkclist.c | 20 ++++++++++++---- g10/sign.c | 63 ++++++++++++++++++++++++++++++--------------------- 4 files changed, 58 insertions(+), 35 deletions(-) diff --git a/doc/HACKING b/doc/HACKING index 3d57ea634..0f7a33a25 100644 --- a/doc/HACKING +++ b/doc/HACKING @@ -229,6 +229,7 @@ Note that such a comment will be removed if the git commit option - Proofread-by :: Sometimes used by translation commits. - Signed-off-by :: Name or mail address of the developer. - Backported-from-master :: Value is the commit id of the original patch. + - Ported-from-stable :: Value is the commit id of the original patch. * Windows ** How to build an installer for Windows diff --git a/g10/keydb.h b/g10/keydb.h index 5ef837be8..ed58b9443 100644 --- a/g10/keydb.h +++ b/g10/keydb.h @@ -134,9 +134,10 @@ struct pubkey_find_info { /* Helper type for preference functions. */ -union pref_hint +struct pref_hint { - int digest_length; + int digest_length; /* We want at least this digest length. */ + int exact; /* We need to use exactly this length. */ }; @@ -276,9 +277,9 @@ gpg_error_t find_and_check_key (ctrl_t ctrl, pk_list_t *pk_list_addr); int algo_available( preftype_t preftype, int algo, - const union pref_hint *hint ); + const struct pref_hint *hint ); int select_algo_from_prefs( PK_LIST pk_list, int preftype, - int request, const union pref_hint *hint); + int request, const struct pref_hint *hint); int select_mdc_from_pklist (PK_LIST pk_list); aead_algo_t select_aead_from_pklist (pk_list_t pk_list); void warn_missing_aead_from_pklist (PK_LIST pk_list); diff --git a/g10/pkclist.c b/g10/pkclist.c index 55cafcacb..643a0fb03 100644 --- a/g10/pkclist.c +++ b/g10/pkclist.c @@ -1444,7 +1444,7 @@ build_pk_list (ctrl_t ctrl, strlist_t rcpts, PK_LIST *ret_pk_list) preference list, so I'm including it. -dms */ int -algo_available( preftype_t preftype, int algo, const union pref_hint *hint) +algo_available( preftype_t preftype, int algo, const struct pref_hint *hint) { if( preftype == PREFTYPE_SYM ) { @@ -1465,16 +1465,26 @@ algo_available( preftype_t preftype, int algo, const union pref_hint *hint) { if (hint && hint->digest_length) { - if (hint->digest_length!=20 || opt.flags.dsa2) + unsigned int n = gcry_md_get_algo_dlen (algo); + + if (hint->exact) + { + /* For example ECDSA requires an exact hash value so + * that we do not truncate. For DSA we allow truncation + * and thus exact is not set. */ + if (hint->digest_length != n) + return 0; + } + else if (hint->digest_length!=20 || opt.flags.dsa2) { /* If --enable-dsa2 is set or the hash isn't 160 bits (which implies DSA2), then we'll accept a hash that is larger than we need. Otherwise we won't accept any hash that isn't exactly the right size. */ - if (hint->digest_length > gcry_md_get_algo_dlen (algo)) + if (hint->digest_length > n) return 0; } - else if (hint->digest_length != gcry_md_get_algo_dlen (algo)) + else if (hint->digest_length != n) return 0; } @@ -1511,7 +1521,7 @@ algo_available( preftype_t preftype, int algo, const union pref_hint *hint) */ int select_algo_from_prefs(PK_LIST pk_list, int preftype, - int request, const union pref_hint *hint) + int request, const struct pref_hint *hint) { PK_LIST pkr; u32 bits[8]; diff --git a/g10/sign.c b/g10/sign.c index d92531eb2..ad605001a 100644 --- a/g10/sign.c +++ b/g10/sign.c @@ -997,13 +997,14 @@ write_signature_packets (ctrl_t ctrl, /* - * Sign the files whose names are in FILENAME. - * If DETACHED has the value true, - * make a detached signature. If FILENAMES->d is NULL read from stdin - * and ignore the detached mode. Sign the file with all secret keys - * which can be taken from LOCUSR, if this is NULL, use the default one + * Sign the files whose names are in FILENAME usingall secret keys + * which can be taken from LOCUSR, if this is NULL, use the default + * secret key. + * If DETACHED has the value true, make a detached signature. * If ENCRYPTFLAG is true, use REMUSER (or ask if it is NULL) to encrypt the - * signed data for these users. + * signed data for these users. If ENCRYPTFLAG is 2 symmetric encryption + * is also used. + * If FILENAMES->d is NULL read from stdin and ignore the detached mode. * If OUTFILE is not NULL; this file is used for output and the function * does not ask for overwrite permission; output is then always * uncompressed, non-armored and in binary mode. @@ -1151,8 +1152,8 @@ sign_file (ctrl_t ctrl, strlist_t filenames, int detached, strlist_t locusr, else { int algo; - int smartcard=0; - union pref_hint hint; + int conflict = 0; + struct pref_hint hint = { 0 }; hint.digest_length = 0; @@ -1180,29 +1181,39 @@ sign_file (ctrl_t ctrl, strlist_t filenames, int detached, strlist_t locusr, int temp_hashlen = gcry_mpi_get_nbits (sk_rover->pk->pkey[1]); if (sk_rover->pk->pubkey_algo == PUBKEY_ALGO_ECDSA) - temp_hashlen = ecdsa_qbits_from_Q (temp_hashlen); + { + temp_hashlen = ecdsa_qbits_from_Q (temp_hashlen); + if (!temp_hashlen) + conflict = 1; /* Better don't use the prefs. */ + temp_hashlen = (temp_hashlen+7)/8; + /* Fixup for that funny nistp521 (yes, 521) were + * we need to use a 512 bit hash algo. */ + if (temp_hashlen == 66) + temp_hashlen = 64; + } + else + temp_hashlen = (temp_hashlen+7)/8; - temp_hashlen = (temp_hashlen+7)/8; - - /* Pick a hash that is large enough for our largest Q */ + /* Pick a hash that is large enough for our largest + * Q or matches our Q. If there are several of them + * we run into a conflict and don't use the + * preferences. */ if (hint.digest_length < temp_hashlen) - hint.digest_length = temp_hashlen; + { + if (sk_rover->pk->pubkey_algo == PUBKEY_ALGO_ECDSA) + { + if (hint.exact) + conflict = 1; + hint.exact = 1; + } + hint.digest_length = temp_hashlen; + } } - /* FIXME: need to check gpg-agent for this. */ - /* else if (sk_rover->pk->is_protected */ - /* && sk_rover->pk->protect.s2k.mode == 1002) */ - /* smartcard = 1; */ } - /* Current smartcards only do 160-bit hashes. If we have - * to have a >160-bit hash, then we can't use the - * recipient prefs as we'd need both =160 and >160 at the - * same time and recipient prefs currently require a - * single hash for all signatures. All this may well have - * to change as the cards add algorithms. */ - if ((!smartcard || (smartcard && hint.digest_length==20)) - && ((algo = select_algo_from_prefs (pk_list, PREFTYPE_HASH, - -1, &hint)) > 0)) + if (!conflict + && (algo = select_algo_from_prefs (pk_list, PREFTYPE_HASH, + -1, &hint)) > 0) { /* Note that we later check that the algo is not weak. */ recipient_digest_algo = algo;