From d07666412d4317460c6f03b3ffd03edf4a715ef7 Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Mon, 13 May 2019 12:38:32 +0200 Subject: [PATCH] gpg: Cleanup use of make_keysig_packet. * g10/sign.c (make_keysig_packet): Remove obsolete arg diegst_algo which was always passed as 0. Change all callers. * g10/gpgcompose.c (signature): Warn when trying to set a digest algo. -- Signed-off-by: Werner Koch --- doc/gpg.texi | 12 ++++++++---- g10/gpgcompose.c | 9 ++++++++- g10/keyedit.c | 19 ++++++++++--------- g10/keygen.c | 8 ++++---- g10/packet.h | 2 +- g10/revoke.c | 4 ++-- g10/sign.c | 38 +++++++++++++++----------------------- 7 files changed, 48 insertions(+), 44 deletions(-) diff --git a/doc/gpg.texi b/doc/gpg.texi index d3b7be598..df807fafc 100644 --- a/doc/gpg.texi +++ b/doc/gpg.texi @@ -3081,10 +3081,14 @@ the same thing. @opindex cert-digest-algo Use @var{name} as the message digest algorithm used when signing a key. Running the program with the command @option{--version} yields a -list of supported algorithms. Be aware that if you choose an algorithm -that GnuPG supports but other OpenPGP implementations do not, then some -users will not be able to use the key signatures you make, or quite -possibly your entire key. +list of supported algorithms. Be aware that if you choose an +algorithm that GnuPG supports but other OpenPGP implementations do +not, then some users will not be able to use the key signatures you +make, or quite possibly your entire key. Note also that a public key +algorithm must be compatible with the specified digest algorithm; thus +selecting an arbitrary digest algorithm may result in error messages +from lower crypto layers or lead to security flaws. + @item --disable-cipher-algo @var{name} @opindex disable-cipher-algo diff --git a/g10/gpgcompose.c b/g10/gpgcompose.c index e882fa8e3..9e6d51a57 100644 --- a/g10/gpgcompose.c +++ b/g10/gpgcompose.c @@ -1799,12 +1799,19 @@ signature (const char *option, int argc, char *argv[], void *cookie) keyid_copy (si.issuer_pk->keyid, pk_keyid (pripk)); } + /* The reuse of core gpg stuff by this tool is questionable when it + * requires adding extra code to the actual gpg code. It does not + * make sense to pass an extra parameter and in particular not given + * that gpg already has opt.cert_digest_algo to override it. */ + if (si.digest_algo) + log_info ("note: digest algo can't be passed to make_keysig_packet\n"); + /* Changing the issuer's key id is fragile. Check to make sure make_keysig_packet didn't recompute the keyid. */ keyid_copy (keyid, si.issuer_pk->keyid); err = make_keysig_packet (global_ctrl, &sig, si.pk, si.uid, si.sk, si.issuer_pk, - si.class, si.digest_algo, + si.class, si.timestamp, si.expiration, mksubpkt_callback, &si, NULL); log_assert (keyid_cmp (keyid, si.issuer_pk->keyid) == 0); diff --git a/g10/keyedit.c b/g10/keyedit.c index c28a565b1..7f4c5a509 100644 --- a/g10/keyedit.c +++ b/g10/keyedit.c @@ -1012,7 +1012,8 @@ sign_uids (ctrl_t ctrl, estream_t fp, node->pkt->pkt.user_id, NULL, pk, - 0x13, 0, 0, 0, + 0x13, + 0, 0, keygen_add_std_prefs, primary_pk, NULL); else @@ -1020,7 +1021,7 @@ sign_uids (ctrl_t ctrl, estream_t fp, node->pkt->pkt.user_id, NULL, pk, - class, 0, + class, timestamp, duration, sign_mk_attrib, &attrib, NULL); @@ -3991,7 +3992,7 @@ menu_adduid (ctrl_t ctrl, kbnode_t pub_keyblock, return 0; } - err = make_keysig_packet (ctrl, &sig, pk, uid, NULL, pk, 0x13, 0, 0, 0, + err = make_keysig_packet (ctrl, &sig, pk, uid, NULL, pk, 0x13, 0, 0, keygen_add_std_prefs, pk, NULL); if (err) { @@ -4374,7 +4375,7 @@ menu_addrevoker (ctrl_t ctrl, kbnode_t pub_keyblock, int sensitive) break; } - rc = make_keysig_packet (ctrl, &sig, pk, NULL, NULL, pk, 0x1F, 0, 0, 0, + rc = make_keysig_packet (ctrl, &sig, pk, NULL, NULL, pk, 0x1F, 0, 0, keygen_add_revkey, &revkey, NULL); if (rc) { @@ -5898,7 +5899,7 @@ reloop: /* (must use this, because we are modifying the list) */ } rc = make_keysig_packet (ctrl, &sig, primary_pk, unode->pkt->pkt.user_id, - NULL, signerkey, 0x30, 0, 0, 0, + NULL, signerkey, 0x30, 0, 0, sign_mk_attrib, &attrib, NULL); free_public_key (signerkey); if (rc) @@ -5977,11 +5978,11 @@ core_revuid (ctrl_t ctrl, kbnode_t keyblock, KBNODE node, memset (&attrib, 0, sizeof attrib); /* should not need to cast away const here; but revocation_reason_build_cb needs to take a non-const - void* in order to meet the function signtuare for the + void* in order to meet the function signutare for the mksubpkt argument to make_keysig_packet */ attrib.reason = (struct revocation_reason_info *)reason; - rc = make_keysig_packet (ctrl, &sig, pk, uid, NULL, pk, 0x30, 0, + rc = make_keysig_packet (ctrl, &sig, pk, uid, NULL, pk, 0x30, timestamp, 0, sign_mk_attrib, &attrib, NULL); if (rc) @@ -6111,7 +6112,7 @@ menu_revkey (ctrl_t ctrl, kbnode_t pub_keyblock) return 0; rc = make_keysig_packet (ctrl, &sig, pk, NULL, NULL, pk, - 0x20, 0, 0, 0, + 0x20, 0, 0, revocation_reason_build_cb, reason, NULL); if (rc) { @@ -6173,7 +6174,7 @@ menu_revsubkey (ctrl_t ctrl, kbnode_t pub_keyblock) node->flag &= ~NODFLG_SELKEY; rc = make_keysig_packet (ctrl, &sig, mainpk, NULL, subpk, mainpk, - 0x28, 0, 0, 0, sign_mk_attrib, &attrib, + 0x28, 0, 0, sign_mk_attrib, &attrib, NULL); if (rc) { diff --git a/g10/keygen.c b/g10/keygen.c index 22ac6f0b5..ac6bcc890 100644 --- a/g10/keygen.c +++ b/g10/keygen.c @@ -1022,7 +1022,7 @@ make_backsig (ctrl_t ctrl, PKT_signature *sig, PKT_public_key *pk, cache_public_key (sub_pk); err = make_keysig_packet (ctrl, &backsig, pk, NULL, sub_pk, sub_psk, 0x19, - 0, timestamp, 0, NULL, NULL, cache_nonce); + timestamp, 0, NULL, NULL, cache_nonce); if (err) log_error ("make_keysig_packet failed for backsig: %s\n", gpg_strerror (err)); @@ -1130,7 +1130,7 @@ write_direct_sig (ctrl_t ctrl, kbnode_t root, PKT_public_key *psk, /* Make the signature. */ err = make_keysig_packet (ctrl, &sig, pk, NULL,NULL, psk, 0x1F, - 0, timestamp, 0, + timestamp, 0, keygen_add_revkey, revkey, cache_nonce); if (err) { @@ -1185,7 +1185,7 @@ write_selfsigs (ctrl_t ctrl, kbnode_t root, PKT_public_key *psk, /* Make the signature. */ err = make_keysig_packet (ctrl, &sig, pk, uid, NULL, psk, 0x13, - 0, timestamp, 0, + timestamp, 0, keygen_add_std_prefs, pk, cache_nonce); if (err) { @@ -1245,7 +1245,7 @@ write_keybinding (ctrl_t ctrl, kbnode_t root, oduap.usage = use; oduap.pk = sub_pk; err = make_keysig_packet (ctrl, &sig, pri_pk, NULL, sub_pk, pri_psk, 0x18, - 0, timestamp, 0, + timestamp, 0, keygen_add_key_flags_and_expire, &oduap, cache_nonce); if (err) diff --git a/g10/packet.h b/g10/packet.h index d05a8f0f2..d787bde9e 100644 --- a/g10/packet.h +++ b/g10/packet.h @@ -931,7 +931,7 @@ int ask_for_detached_datafile( gcry_md_hd_t md, gcry_md_hd_t md2, int make_keysig_packet (ctrl_t ctrl, PKT_signature **ret_sig, PKT_public_key *pk, PKT_user_id *uid, PKT_public_key *subpk, - PKT_public_key *pksk, int sigclass, int digest_algo, + PKT_public_key *pksk, int sigclass, u32 timestamp, u32 duration, int (*mksubpkt)(PKT_signature *, void *), void *opaque, diff --git a/g10/revoke.c b/g10/revoke.c index e8ce3544c..e63060cb9 100644 --- a/g10/revoke.c +++ b/g10/revoke.c @@ -343,7 +343,7 @@ gen_desig_revoke (ctrl_t ctrl, const char *uname, strlist_t locusr) push_armor_filter (afx, out); /* create it */ - rc = make_keysig_packet (ctrl, &sig, pk, NULL, NULL, pk2, 0x20, 0, + rc = make_keysig_packet (ctrl, &sig, pk, NULL, NULL, pk2, 0x20, 0, 0, revocation_reason_build_cb, reason, NULL); @@ -474,7 +474,7 @@ create_revocation (ctrl_t ctrl, afx->hdrlines = "Comment: This is a revocation certificate\n"; push_armor_filter (afx, out); - rc = make_keysig_packet (ctrl, &sig, psk, NULL, NULL, psk, 0x20, 0, + rc = make_keysig_packet (ctrl, &sig, psk, NULL, NULL, psk, 0x20, 0, 0, revocation_reason_build_cb, reason, cache_nonce); if (rc) diff --git a/g10/sign.c b/g10/sign.c index 176940bff..132b94101 100644 --- a/g10/sign.c +++ b/g10/sign.c @@ -1593,7 +1593,7 @@ make_keysig_packet (ctrl_t ctrl, PKT_signature **ret_sig, PKT_public_key *pk, PKT_user_id *uid, PKT_public_key *subpk, PKT_public_key *pksk, - int sigclass, int digest_algo, + int sigclass, u32 timestamp, u32 duration, int (*mksubpkt)(PKT_signature *, void *), void *opaque, const char *cache_nonce) @@ -1601,6 +1601,7 @@ make_keysig_packet (ctrl_t ctrl, PKT_signature *sig; int rc = 0; int sigversion; + int digest_algo; gcry_md_hd_t md; log_assert ((sigclass >= 0x10 && sigclass <= 0x13) || sigclass == 0x1F @@ -1612,31 +1613,22 @@ make_keysig_packet (ctrl_t ctrl, else sigversion = 4; - if (!digest_algo) + /* Select the digest algo to use. */ + if (opt.cert_digest_algo) /* Forceful override by the user. */ + digest_algo = opt.cert_digest_algo; + else if (pksk->pubkey_algo == PUBKEY_ALGO_DSA) /* Meet DSA requirements. */ + digest_algo = match_dsa_hash (gcry_mpi_get_nbits (pksk->pkey[1])/8); + else if (pksk->pubkey_algo == PUBKEY_ALGO_ECDSA /* Meet ECDSA requirements. */ + || pksk->pubkey_algo == PUBKEY_ALGO_EDDSA) { - /* Basically, this means use SHA1 always unless the user - * specified something (use whatever they said), or it's DSA - * (use the best match). They still can't pick an inappropriate - * hash for DSA or the signature will fail. Note that this - * still allows the caller of make_keysig_packet to override the - * user setting if it must. */ - - if (opt.cert_digest_algo) - digest_algo = opt.cert_digest_algo; - else if (pksk->pubkey_algo == PUBKEY_ALGO_DSA) - digest_algo = match_dsa_hash (gcry_mpi_get_nbits (pksk->pkey[1])/8); - else if (pksk->pubkey_algo == PUBKEY_ALGO_ECDSA - || pksk->pubkey_algo == PUBKEY_ALGO_EDDSA) - { - if (openpgp_oid_is_ed25519 (pksk->pkey[0])) - digest_algo = DIGEST_ALGO_SHA256; - else - digest_algo = match_dsa_hash - (ecdsa_qbits_from_Q (gcry_mpi_get_nbits (pksk->pkey[1]))/8); - } + if (openpgp_oid_is_ed25519 (pksk->pkey[0])) + digest_algo = DIGEST_ALGO_SHA256; else - digest_algo = DEFAULT_DIGEST_ALGO; + digest_algo = match_dsa_hash + (ecdsa_qbits_from_Q (gcry_mpi_get_nbits (pksk->pkey[1]))/8); } + else /* Use the default. */ + digest_algo = DEFAULT_DIGEST_ALGO; if (gcry_md_open (&md, digest_algo, 0)) BUG ();