From f799e9728bcadb3d4148a47848c78c5647860ea4 Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Fri, 15 Mar 2019 19:50:37 +0100 Subject: [PATCH] gpg: Avoid importing secret keys if the keyblock is not valid. * g10/keydb.h (struct kbnode_struct): Replace unused field RECNO by new field TAG. * g10/kbnode.c (alloc_node): Change accordingly. * g10/import.c (import_one): Add arg r_valid. (sec_to_pub_keyblock): Set tags. (resync_sec_with_pub_keyblock): New. (import_secret_one): Change return code to gpg_error_t. Return an error code if sec_to_pub_keyblock failed. Resync secret keyblock. -- When importing an invalid secret key ring for example without key binding signatures or no UIDs, gpg used to let gpg-agent store the secret keys anyway. This is clearly a bug because the diagnostics before claimed that for example the subkeys have been skipped. Importing the secret key parameters then anyway is surprising in particular because a gpg -k does not show the key. After importing the public key the secret keys suddenly showed up. This changes the behaviour of GnuPG-bug-id: 4392 to me more consistent but is not a solution to the actual bug. Caution: The ecc.scm test now fails because two of the sample keys don't have binding signatures. Signed-off-by: Werner Koch --- g10/import.c | 122 ++++++++++++++++++++++++++------ g10/kbnode.c | 2 +- g10/keydb.h | 13 ++-- g10/pubkey-enc.c | 2 +- tests/openpgp/ecc.scm | 2 +- tests/openpgp/samplekeys/README | 2 + 6 files changed, 112 insertions(+), 31 deletions(-) diff --git a/g10/import.c b/g10/import.c index 359a14e20..155792d5a 100644 --- a/g10/import.c +++ b/g10/import.c @@ -109,8 +109,8 @@ static gpg_error_t import_one (ctrl_t ctrl, unsigned char **fpr, size_t *fpr_len, unsigned int options, int from_sk, int silent, import_screener_t screener, void *screener_arg, - int origin, const char *url); -static int import_secret_one (ctrl_t ctrl, kbnode_t keyblock, + int origin, const char *url, int *r_valid); +static gpg_error_t import_secret_one (ctrl_t ctrl, kbnode_t keyblock, struct import_stats_s *stats, int batch, unsigned int options, int for_migration, import_screener_t screener, void *screener_arg); @@ -588,7 +588,7 @@ import (ctrl_t ctrl, IOBUF inp, const char* fname,struct import_stats_s *stats, if (keyblock->pkt->pkttype == PKT_PUBLIC_KEY) rc = import_one (ctrl, keyblock, stats, fpr, fpr_len, options, 0, 0, - screener, screener_arg, origin, url); + screener, screener_arg, origin, url, NULL); else if (keyblock->pkt->pkttype == PKT_SECRET_KEY) rc = import_secret_one (ctrl, keyblock, stats, opt.batch, options, 0, @@ -1667,7 +1667,9 @@ update_key_origin (kbnode_t keyblock, u32 curtime, int origin, const char *url) * programs which called gpg. If SILENT is no messages are printed - * even most error messages are suppressed. ORIGIN is the origin of * the key (0 for unknown) and URL the corresponding URL. FROM_SK - * indicates that the key has been made from a secret key. + * indicates that the key has been made from a secret key. If R_SAVED + * is not NULL a boolean will be stored indicating whether the keyblock + * has valid parts. */ static gpg_error_t import_one (ctrl_t ctrl, @@ -1675,7 +1677,7 @@ import_one (ctrl_t ctrl, unsigned char **fpr, size_t *fpr_len, unsigned int options, int from_sk, int silent, import_screener_t screener, void *screener_arg, - int origin, const char *url) + int origin, const char *url, int *r_valid) { gpg_error_t err = 0; PKT_public_key *pk; @@ -1694,6 +1696,9 @@ import_one (ctrl_t ctrl, int any_filter = 0; KEYDB_HANDLE hd = NULL; + if (r_valid) + *r_valid = 0; + /* If show-only is active we don't won't any extra output. */ if ((options & (IMPORT_SHOW | IMPORT_DRY_RUN))) silent = 1; @@ -1714,7 +1719,7 @@ import_one (ctrl_t ctrl, if (opt.verbose && !opt.interactive && !silent && !from_sk) { /* Note that we do not print this info in FROM_SK mode - * because import_one already printed that. */ + * because import_secret_one already printed that. */ log_info ("pub %s/%s %s ", pubkey_string (pk, pkstrbuf, sizeof pkstrbuf), keystr_from_pk(pk), datestr_from_pk(pk) ); @@ -1849,6 +1854,10 @@ import_one (ctrl_t ctrl, return 0; } + /* The keyblock is valid and ready for real import. */ + if (r_valid) + *r_valid = 1; + /* Show the key in the form it is merged or inserted. We skip this * if "import-export" is also active without --armor or the output * file has explicily been given. */ @@ -2463,14 +2472,21 @@ transfer_secret_keys (ctrl_t ctrl, struct import_stats_s *stats, /* Walk a secret keyblock and produce a public keyblock out of it. - Returns a new node or NULL on error. */ + * Returns a new node or NULL on error. Modifies the tag field of the + * nodes. */ static kbnode_t sec_to_pub_keyblock (kbnode_t sec_keyblock) { kbnode_t pub_keyblock = NULL; kbnode_t ctx = NULL; kbnode_t secnode, pubnode; + unsigned int tag = 0; + /* Set a tag to all nodes. */ + for (secnode = sec_keyblock; secnode; secnode = secnode->next) + secnode->tag = ++tag; + + /* Copy. */ while ((secnode = walk_kbnode (sec_keyblock, &ctx, 0))) { if (secnode->pkt->pkttype == PKT_SECRET_KEY @@ -2500,6 +2516,7 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock) { pubnode = clone_kbnode (secnode); } + pubnode->tag = secnode->tag; if (!pub_keyblock) pub_keyblock = pubnode; @@ -2510,23 +2527,67 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock) return pub_keyblock; } + +/* Delete all notes in the keyblock at R_KEYBLOCK which are not in + * PUB_KEYBLOCK. Modifies the tags of both keyblock's nodes. */ +static gpg_error_t +resync_sec_with_pub_keyblock (kbnode_t *r_keyblock, kbnode_t pub_keyblock) +{ + kbnode_t sec_keyblock = *r_keyblock; + kbnode_t node; + unsigned int *taglist; + unsigned int ntaglist, n; + + /* Collect all tags in an array for faster searching. */ + for (ntaglist = 0, node = pub_keyblock; node; node = node->next) + ntaglist++; + taglist = xtrycalloc (ntaglist, sizeof *taglist); + if (!taglist) + return gpg_error_from_syserror (); + for (ntaglist = 0, node = pub_keyblock; node; node = node->next) + taglist[ntaglist++] = node->tag; + + /* Walks over the secret keyblock and delete all nodes which are not + * in the tag list. Those nodes have been delete in the + * pub_keyblock. Sequential search is a bit lazt and could be + * optimized by sorting and bsearch; however secret key rings are + * short and there are easier weaus to DoS gpg. */ + for (node = sec_keyblock; node; node = node->next) + { + for (n=0; n < ntaglist; n++) + if (taglist[n] == node->tag) + break; + if (n == ntaglist) + delete_kbnode (node); + } + + xfree (taglist); + + /* Commit the as deleted marked nodes and return the possibly + * modified keyblock. */ + commit_kbnode (&sec_keyblock); + *r_keyblock = sec_keyblock; + return 0; +} + + /**************** * Ditto for secret keys. Handling is simpler than for public keys. * We allow secret key importing only when allow is true, this is so * that a secret key can not be imported accidentally and thereby tampering * with the trust calculation. */ -static int +static gpg_error_t import_secret_one (ctrl_t ctrl, kbnode_t keyblock, - struct import_stats_s *stats, int batch, unsigned int options, - int for_migration, + struct import_stats_s *stats, int batch, + unsigned int options, int for_migration, import_screener_t screener, void *screener_arg) { PKT_public_key *pk; struct seckey_info *ski; kbnode_t node, uidnode; u32 keyid[2]; - int rc = 0; + gpg_error_t err = 0; int nr_prev; kbnode_t pub_keyblock; char pkstrbuf[PUBKEY_STRING_SIZE]; @@ -2550,7 +2611,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, if (opt.verbose && !for_migration) { - log_info ("sec %s/%s %s ", + log_info ("sec %s/%s %s ", pubkey_string (pk, pkstrbuf, sizeof pkstrbuf), keystr_from_pk (pk), datestr_from_pk (pk)); if (uidnode) @@ -2610,20 +2671,35 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, /* Make a public key out of the key. */ pub_keyblock = sec_to_pub_keyblock (keyblock); if (!pub_keyblock) - log_error ("key %s: failed to create public key from secret key\n", - keystr_from_pk (pk)); + { + err = gpg_error_from_syserror (); + log_error ("key %s: failed to create public key from secret key\n", + keystr_from_pk (pk)); + } else { + int valid; + /* Note that this outputs an IMPORT_OK status message for the public key block, and below we will output another one for the secret keys. FIXME? */ import_one (ctrl, pub_keyblock, stats, NULL, NULL, options, 1, for_migration, - screener, screener_arg, 0, NULL); + screener, screener_arg, 0, NULL, &valid); - /* Fixme: We should check for an invalid keyblock and - cancel the secret key import in this case. */ - release_kbnode (pub_keyblock); + /* The secret keyblock may not have nodes which are deleted in + * the public keyblock. Otherwise we would import just the + * secret key without having the public key. That would be + * surprising and clutters out private-keys-v1.d. */ + err = resync_sec_with_pub_keyblock (&keyblock, pub_keyblock); + if (err) + goto leave; + + if (!valid) + { + err = gpg_error (GPG_ERR_NO_SECKEY); + goto leave; + } /* At least we cancel the secret key import when the public key import was skipped due to MERGE_ONLY option and a new @@ -2631,7 +2707,8 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, if (!(opt.dry_run || (options & IMPORT_DRY_RUN)) && stats->skipped_new_keys <= nr_prev) { - /* Read the keyblock again to get the effects of a merge. */ + /* Read the keyblock again to get the effects of a merge for + * the public key. */ /* Fixme: we should do this based on the fingerprint or even better let import_one return the merged keyblock. */ @@ -2641,8 +2718,6 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, keystr_from_pk (pk)); else { - gpg_error_t err; - /* transfer_secret_keys collects subkey stats. */ struct import_stats_s subkey_stats = {0}; @@ -2680,6 +2755,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, if (is_status_enabled ()) print_import_ok (pk, status); + check_prefs (ctrl, node); } release_kbnode (node); @@ -2687,7 +2763,9 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock, } } - return rc; + leave: + release_kbnode (pub_keyblock); + return err; } diff --git a/g10/kbnode.c b/g10/kbnode.c index 64def0509..93035e8f6 100644 --- a/g10/kbnode.c +++ b/g10/kbnode.c @@ -68,8 +68,8 @@ alloc_node (void) n->next = NULL; n->pkt = NULL; n->flag = 0; + n->tag = 0; n->private_flag=0; - n->recno = 0; return n; } diff --git a/g10/keydb.h b/g10/keydb.h index c52856d7f..7cdfe9bbf 100644 --- a/g10/keydb.h +++ b/g10/keydb.h @@ -52,12 +52,13 @@ typedef struct getkey_ctx_s *getkey_ctx_t; * This structure is also used to bind arbitrary packets together. */ -struct kbnode_struct { - KBNODE next; - PACKET *pkt; - int flag; - int private_flag; - ulong recno; /* used while updating the trustdb */ +struct kbnode_struct +{ + kbnode_t next; + PACKET *pkt; + int flag; /* Local use during keyblock processing (not cloned).*/ + unsigned int tag; /* Ditto. */ + int private_flag; }; #define is_deleted_kbnode(a) ((a)->private_flag & 1) diff --git a/g10/pubkey-enc.c b/g10/pubkey-enc.c index e0a6e8ae1..055c39b8f 100644 --- a/g10/pubkey-enc.c +++ b/g10/pubkey-enc.c @@ -117,7 +117,7 @@ get_session_key (ctrl_t ctrl, struct pubkey_enc_list *list, DEK *dek) * - On-card keys of an active card * - On-disk keys with protection * - On-card keys from cards which are not plugged it. Here a - * cancel-all button should stop aksing for other cards. + * cancel-all button should stop asking for other cards. * Without any anonymous keys the sorting can be skipped. */ for (k = list; k; k = k->next) diff --git a/tests/openpgp/ecc.scm b/tests/openpgp/ecc.scm index d7c02a5e2..a63ec45bd 100755 --- a/tests/openpgp/ecc.scm +++ b/tests/openpgp/ecc.scm @@ -175,7 +175,7 @@ Rg== (display "This is one line\n" (fdopen fd "wb"))) (for-each-p - "Checking ECDSA decryption" + "Checking ECDH decryption" (lambda (test) (lettmp (x y) (call-with-output-file diff --git a/tests/openpgp/samplekeys/README b/tests/openpgp/samplekeys/README index 9f1648bdf..f8a7e9ed7 100644 --- a/tests/openpgp/samplekeys/README +++ b/tests/openpgp/samplekeys/README @@ -29,3 +29,5 @@ Notes: such a file is created which is then directly followed by a separate armored public key block. To create such a sample concatenate pgp-desktop-skr.asc and E657FB607BB4F21C90BB6651BC067AF28BC90111.asc +- ecc-sample-2-sec.asc and ecc-sample-3-sec.asc do not have and + binding signatures either. ecc-sample-1-sec.asc has them, though.