[Pkg-gnupg-commit] [gnupg2] 44/205: gpg: Systematically detect and fix signatures that are out of order.
Daniel Kahn Gillmor
dkg at fifthhorseman.net
Wed May 11 08:38:13 UTC 2016
This is an automated email from the git hooks/post-receive script.
dkg pushed a commit to branch experimental
in repository gnupg2.
commit 2d1d795481bc011447284f8ce0a3ae96a08daf17
Author: Neal H. Walfield <neal at g10code.com>
Date: Fri Feb 19 15:52:08 2016 +0100
gpg: Systematically detect and fix signatures that are out of order.
* g10/keyedit.c (sig_comparison): New function.
(fix_key_signature_order): Merge functionality into...
(check_all_keysigs): ... this function. Rewrite to eliminate
duplicates and use a systematic approach to detecting and moving
signatures that are out of order instead of a heuristic.
(fix_keyblock): Don't call fix_key_signature_order. Call
check_all_keysigs instead after collapsing the uids.
--
Signed-off-by: Neal H. Walfield <neal at g10code.com>
GnuPG-bug-id: 2236
---
g10/keyedit.c | 689 +++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 564 insertions(+), 125 deletions(-)
diff --git a/g10/keyedit.c b/g10/keyedit.c
index 8cec66a..d7c2a4b 100644
--- a/g10/keyedit.c
+++ b/g10/keyedit.c
@@ -322,97 +322,584 @@ print_and_check_one_sig (KBNODE keyblock, KBNODE node,
-/*
- * Check the keysigs and set the flags to indicate errors.
- * Returns true if error found.
- */
+/* Order two signatures. The actual ordering isn't important. Our
+ goal is to ensure that identical signatures occur together. */
static int
-check_all_keysigs (KBNODE keyblock, int only_selected, int only_selfsigs)
+sig_comparison (const void *av, const void *bv)
{
- KBNODE kbctx;
- KBNODE node;
- int inv_sigs = 0;
- int no_key = 0;
- int oth_err = 0;
- int has_selfsig = 0;
- int mis_selfsig = 0;
- int selected = !only_selected;
- int anyuid = 0;
- u32 keyid[2];
+ const KBNODE an = *(const KBNODE *) av;
+ const KBNODE bn = *(const KBNODE *) bv;
+ const PKT_signature *a;
+ const PKT_signature *b;
+ int ndataa;
+ int ndatab;
+ int i;
+
+ assert (an->pkt->pkttype == PKT_SIGNATURE);
+ assert (bn->pkt->pkttype == PKT_SIGNATURE);
+
+ a = an->pkt->pkt.signature;
+ b = bn->pkt->pkt.signature;
- for (kbctx = NULL; (node = walk_kbnode (keyblock, &kbctx, 0));)
+ if (a->digest_algo < b->digest_algo)
+ return -1;
+ if (a->digest_algo > b->digest_algo)
+ return 1;
+
+ ndataa = pubkey_get_nsig (a->pubkey_algo);
+ ndatab = pubkey_get_nsig (a->pubkey_algo);
+ assert (ndataa == ndatab);
+
+ for (i = 0; i < ndataa; i ++)
{
- if (node->pkt->pkttype == PKT_PUBLIC_KEY)
+ int c = gcry_mpi_cmp (a->data[i], b->data[i]);
+ if (c != 0)
+ return c;
+ }
+
+ /* Okay, they are equal. */
+ return 0;
+}
+
+/* Perform a few sanity checks on a keyblock is okay and possibly
+ repair some damage. Concretely:
+
+ - Detect duplicate signatures and remove them.
+
+ - Detect out of order signatures and relocate them (e.g., a sig
+ over user id X located under subkey Y).
+
+ Note: this function does not remove signatures that don't belong or
+ components that are not signed! (Although it would be trivial to
+ do so.)
+
+ If ONLY_SELFSIGS is true, then this function only reorders self
+ signatures (it still checks all signatures for duplicates,
+ however).
+
+ Returns 1 if the keyblock was modified, 0 otherwise. */
+static int
+check_all_keysigs (KBNODE kb, int only_selected, int only_selfsigs)
+{
+ gpg_error_t err;
+ PKT_public_key *pk;
+ KBNODE n, n_next, *n_prevp, n2;
+ char *pending_desc = NULL;
+ PKT_public_key *issuer;
+ KBNODE last_printed_component;
+ KBNODE current_component = NULL;
+ int dups = 0;
+ int missing_issuer = 0;
+ int reordered = 0;
+ int bad_signature = 0;
+ int missing_selfsig = 0;
+ int modified = 0;
+
+ assert (kb->pkt->pkttype == PKT_PUBLIC_KEY);
+ pk = kb->pkt->pkt.public_key;
+
+ /* First we look for duplicates. */
+ {
+ int nsigs = 0;
+ KBNODE *sigs;
+ int i;
+ int last_i;
+
+ /* Count the sigs. */
+ for (n = kb; n; n = n->next)
+ if (is_deleted_kbnode (n))
+ continue;
+ else if (n->pkt->pkttype == PKT_SIGNATURE)
+ nsigs ++;
+
+ /* Add them all to the SIGS array. */
+ sigs = xmalloc_clear (sizeof (*sigs) * nsigs);
+
+ i = 0;
+ for (n = kb; n; n = n->next)
+ {
+ if (is_deleted_kbnode (n))
+ continue;
+
+ if (n->pkt->pkttype != PKT_SIGNATURE)
+ continue;
+
+ sigs[i] = n;
+ i ++;
+ }
+ assert (i == nsigs);
+
+ qsort (sigs, nsigs, sizeof (sigs[0]), sig_comparison);
+
+ last_i = 0;
+ for (i = 1; i < nsigs; i ++)
+ {
+ assert (sigs[last_i]);
+ assert (sigs[last_i]->pkt->pkttype == PKT_SIGNATURE);
+ assert (sigs[i]);
+ assert (sigs[i]->pkt->pkttype == PKT_SIGNATURE);
+
+ if (sig_comparison (&sigs[last_i], &sigs[i]) == 0)
+ /* They are the same. Kill the latter. */
+ {
+ if (DBG_PACKET)
+ {
+ PKT_signature *sig = sigs[i]->pkt->pkt.signature;
+
+ log_debug ("Signature appears multiple times, deleting duplicate:\n");
+ log_debug (" sig: class 0x%x, issuer: %s, timestamp: %s (%lld), digest: %02x %02x\n",
+ sig->sig_class, keystr (sig->keyid),
+ isotimestamp (sig->timestamp),
+ (long long) sig->timestamp,
+ sig->digest_start[0], sig->digest_start[1]);
+ }
+
+ /* Remove sigs[i] from the keyblock. */
+ {
+ KBNODE z, *prevp;
+ int to_kill = last_i;
+ last_i = i;
+
+ for (prevp = &kb, z = kb; z; prevp = &z->next, z = z->next)
+ if (z == sigs[to_kill])
+ break;
+
+ *prevp = sigs[to_kill]->next;
+
+ sigs[to_kill]->next = NULL;
+ release_kbnode (sigs[to_kill]);
+ sigs[to_kill] = NULL;
+
+ dups ++;
+ modified = 1;
+ }
+ }
+ else
+ last_i = i;
+ }
+
+ xfree (sigs);
+ }
+
+ /* Make sure the sigs occur after the component (public key, subkey,
+ user id) that they sign. */
+ issuer = NULL;
+ last_printed_component = NULL;
+ for (n_prevp = &kb, n = kb;
+ n;
+ /* If we moved n, then n_prevp is need valid. */
+ n_prevp = (n->next == n_next ? &n->next : n_prevp), n = n_next)
+ {
+ PACKET *p;
+ int processed_current_component;
+ PKT_signature *sig;
+ int rc;
+ int dump_sig_params = 0;
+
+ n_next = n->next;
+
+ if (is_deleted_kbnode (n))
+ continue;
+
+ p = n->pkt;
+
+ if (issuer && issuer != pk)
{
- if (only_selfsigs)
- keyid_from_pk (node->pkt->pkt.public_key, keyid);
+ free_public_key (issuer);
+ issuer = NULL;
}
- else if (node->pkt->pkttype == PKT_USER_ID)
- {
- PKT_user_id *uid = node->pkt->pkt.user_id;
- if (only_selected)
- selected = (node->flag & NODFLG_SELUID);
- if (selected)
- {
- tty_printf ("uid ");
- tty_print_utf8_string (uid->name, uid->len);
- tty_printf ("\n");
- if (anyuid && !has_selfsig)
- mis_selfsig++;
- has_selfsig = 0;
- anyuid = 1;
- }
- }
- else if (selected && node->pkt->pkttype == PKT_SIGNATURE
- && ((node->pkt->pkt.signature->sig_class & ~3) == 0x10
- || node->pkt->pkt.signature->sig_class == 0x30))
- {
- int selfsig;
- PKT_signature *sig = node->pkt->pkt.signature;
+ xfree (pending_desc);
+ pending_desc = NULL;
- if (only_selfsigs
- && !(keyid[0] == sig->keyid[0] && keyid[1] == sig->keyid[1]))
+ switch (p->pkttype)
+ {
+ case PKT_PUBLIC_KEY:
+ assert (p->pkt.public_key == pk);
+ if (only_selected && ! (n->flag & NODFLG_SELKEY))
{
- /* Not a selfsig but we want only selfsigs - skip. */
- /* Static analyzer note: A claim that KEYID above has
- garbage is not correct because KEYID is set from the
- public key packet which is always the first packet in
- a keyblock and thus parsed before this signature. */
+ current_component = NULL;
+ break;
}
- else if (print_and_check_one_sig (keyblock, node, &inv_sigs,
- &no_key, &oth_err, &selfsig,
- 0, only_selfsigs))
- {
- if (selfsig)
- has_selfsig = 1;
- }
- /* Hmmm: should we update the trustdb here? */
- }
+
+ if (DBG_PACKET)
+ log_debug ("public key %s: timestamp: %s (%lld)\n",
+ pk_keyid_str (pk),
+ isotimestamp (pk->timestamp),
+ (long long) pk->timestamp);
+ current_component = n;
+ break;
+ case PKT_PUBLIC_SUBKEY:
+ if (only_selected && ! (n->flag & NODFLG_SELKEY))
+ {
+ current_component = NULL;
+ break;
+ }
+
+ if (DBG_PACKET)
+ log_debug ("subkey %s: timestamp: %s (%lld)\n",
+ pk_keyid_str (p->pkt.public_key),
+ isotimestamp (p->pkt.public_key->timestamp),
+ (long long) p->pkt.public_key->timestamp);
+ current_component = n;
+ break;
+ case PKT_USER_ID:
+ if (only_selected && ! (n->flag & NODFLG_SELUID))
+ {
+ current_component = NULL;
+ break;
+ }
+
+ if (DBG_PACKET)
+ log_debug ("user id: %s\n",
+ p->pkt.user_id->attrib_data
+ ? "[ photo id ]"
+ : p->pkt.user_id->name);
+ current_component = n;
+ break;
+ case PKT_SIGNATURE:
+ if (! current_component)
+ /* The current component is not selected, don't check the
+ sigs under it. */
+ break;
+
+ sig = n->pkt->pkt.signature;
+
+ pending_desc = xasprintf (" sig: class: 0x%x, issuer: %s, timestamp: %s (%lld), digest: %02x %02x",
+ sig->sig_class,
+ keystr (sig->keyid),
+ isotimestamp (sig->timestamp),
+ (long long) sig->timestamp,
+ sig->digest_start[0], sig->digest_start[1]);
+
+
+ if (keyid_cmp (pk_keyid (pk), sig->keyid) == 0)
+ issuer = pk;
+ else
+ /* Issuer is a different key. */
+ {
+ if (only_selfsigs)
+ continue;
+
+ issuer = xmalloc (sizeof (*issuer));
+ err = get_pubkey (issuer, sig->keyid);
+ if (err)
+ {
+ xfree (issuer);
+ issuer = NULL;
+ if (DBG_PACKET)
+ {
+ if (pending_desc)
+ log_debug ("%s", pending_desc);
+ log_debug (" Can't check signature allegedly issued by %s: %s\n",
+ keystr (sig->keyid), gpg_strerror (err));
+ }
+ missing_issuer ++;
+ break;
+ }
+ }
+
+ if ((err = openpgp_pk_test_algo (sig->pubkey_algo)))
+ {
+ if (DBG_PACKET && pending_desc)
+ log_debug ("%s", pending_desc);
+ tty_printf (_("can't check signature with unsupported public-key algorithm (%d): %s.\n"),
+ sig->pubkey_algo, gpg_strerror (err));
+ break;
+ }
+ if ((err = openpgp_md_test_algo (sig->digest_algo)))
+ {
+ if (DBG_PACKET && pending_desc)
+ log_debug ("%s", pending_desc);
+ tty_printf (_("can't check signature with unsupported message-digest algorithm %d: %s.\n"),
+ sig->digest_algo, gpg_strerror (err));
+ break;
+ }
+
+ /* We iterate over the keyblock. Most likely, the matching
+ component is the current component so always try that
+ first. */
+ processed_current_component = 0;
+ for (n2 = current_component;
+ n2;
+ n2 = (processed_current_component ? n2->next : kb),
+ processed_current_component = 1)
+ if (is_deleted_kbnode (n2))
+ continue;
+ else if (processed_current_component && n2 == current_component)
+ /* Don't process it twice. */
+ continue;
+ else
+ {
+ err = check_signature_over_key_or_uid (issuer, sig, kb, n2->pkt,
+ NULL, NULL);
+ if (! err)
+ break;
+ }
+
+ /* n/sig is a signature and n2 is the component (public key,
+ subkey or user id) that it signs, if any.
+ current_component is that component that it appears to
+ apply to (according to the ordering). */
+
+ if (current_component == n2)
+ {
+ if (DBG_PACKET)
+ {
+ log_debug ("%s", pending_desc);
+ log_debug (" Good signature over last key or uid!\n");
+ }
+
+ rc = 0;
+ }
+ else if (n2)
+ {
+ assert (n2->pkt->pkttype == PKT_USER_ID
+ || n2->pkt->pkttype == PKT_PUBLIC_KEY
+ || n2->pkt->pkttype == PKT_PUBLIC_SUBKEY);
+
+ if (DBG_PACKET)
+ {
+ log_debug ("%s", pending_desc);
+ log_debug (" Good signature out of order! (Over %s (%d) '%s')\n",
+ n2->pkt->pkttype == PKT_USER_ID
+ ? "user id"
+ : n2->pkt->pkttype == PKT_PUBLIC_SUBKEY
+ ? "subkey"
+ : "primary key",
+ n2->pkt->pkttype,
+ n2->pkt->pkttype == PKT_USER_ID
+ ? n2->pkt->pkt.user_id->name
+ : pk_keyid_str (n2->pkt->pkt.public_key));
+ }
+
+ /* Reorder the packets: move the signature n to be just
+ after n2. */
+
+ /* Unlink the signature. */
+ assert (n_prevp);
+ *n_prevp = n->next;
+
+ /* Insert the sig immediately after the component. */
+ n->next = n2->next;
+ n2->next = n;
+
+ reordered ++;
+ modified = 1;
+
+ rc = 0;
+ }
+ else
+ {
+ if (DBG_PACKET)
+ {
+ log_debug ("%s", pending_desc);
+ log_debug (" Bad signature.\n");
+ }
+
+ if (DBG_PACKET)
+ dump_sig_params = 1;
+
+ bad_signature ++;
+
+ rc = GPG_ERR_BAD_SIGNATURE;
+ }
+
+ /* We don't cache the result here, because we haven't
+ completely checked that the signature is legitimate. For
+ instance, if we have a revocation certificate on Alice's
+ key signed by Bob, the signature may be good, but we
+ haven't checked that Bob is a designated revoker. */
+ /* cache_sig_result (sig, rc); */
+
+ {
+ int has_selfsig = 0;
+ if (! rc && issuer == pk)
+ {
+ if (n2->pkt->pkttype == PKT_PUBLIC_KEY
+ && (/* Direct key signature. */
+ sig->sig_class == 0x1f
+ /* Key revocation signature. */
+ || sig->sig_class == 0x20))
+ has_selfsig = 1;
+ if (n2->pkt->pkttype == PKT_PUBLIC_SUBKEY
+ && (/* Subkey binding sig. */
+ sig->sig_class == 0x18
+ /* Subkey revocation sig. */
+ || sig->sig_class == 0x28))
+ has_selfsig = 1;
+ if (n2->pkt->pkttype == PKT_USER_ID
+ && (/* Certification sigs. */
+ sig->sig_class == 0x10
+ || sig->sig_class == 0x11
+ || sig->sig_class == 0x12
+ || sig->sig_class == 0x13
+ /* Certification revocation sig. */
+ || sig->sig_class == 0x30))
+ has_selfsig = 1;
+ }
+
+ if ((n2 && n2 != last_printed_component)
+ || (! n2 && last_printed_component != current_component))
+ {
+ int is_reordered = n2 && n2 != current_component;
+ if (n2)
+ last_printed_component = n2;
+ else
+ last_printed_component = current_component;
+
+ if (last_printed_component->pkt->pkttype == PKT_USER_ID)
+ {
+ tty_printf ("uid ");
+ tty_print_utf8_string (last_printed_component
+ ->pkt->pkt.user_id->name,
+ last_printed_component
+ ->pkt->pkt.user_id->len);
+ }
+ else if (last_printed_component->pkt->pkttype
+ == PKT_PUBLIC_KEY)
+ tty_printf ("pub %s",
+ pk_keyid_str (last_printed_component
+ ->pkt->pkt.public_key));
+ else
+ tty_printf ("sub %s",
+ pk_keyid_str (last_printed_component
+ ->pkt->pkt.public_key));
+
+ if (is_reordered)
+ tty_printf (_(" (reordered signatures follow)"));
+ tty_printf ("\n");
+ }
+
+ print_one_sig (rc, kb, n, NULL, NULL, NULL, has_selfsig,
+ 0, only_selfsigs);
+ }
+
+ if (dump_sig_params)
+ {
+ int i;
+
+ for (i = 0; i < pubkey_get_nsig (sig->pubkey_algo); i ++)
+ {
+ char buffer[1024];
+ size_t len;
+ char *printable;
+ gcry_mpi_print (GCRYMPI_FMT_USG,
+ buffer, sizeof (buffer), &len,
+ sig->data[i]);
+ printable = bin2hex (buffer, len, NULL);
+ log_info (" %d: %s\n", i, printable);
+ xfree (printable);
+ }
+ }
+ break;
+ default:
+ if (DBG_PACKET)
+ log_debug ("unhandled packet: %d\n", p->pkttype);
+ break;
+ }
}
- if (!has_selfsig)
- mis_selfsig++;
- if (inv_sigs)
- tty_printf (ngettext("%d bad signature\n",
- "%d bad signatures\n", inv_sigs), inv_sigs);
+ xfree (pending_desc);
+ pending_desc = NULL;
- if (no_key)
- tty_printf (ngettext("%d signature not checked due to a missing key\n",
- "%d signatures not checked due to missing keys\n",
- no_key), no_key);
+ if (issuer != pk)
+ free_public_key (issuer);
+ issuer = NULL;
- if (oth_err)
- tty_printf (ngettext("%d signature not checked due to an error\n",
- "%d signatures not checked due to errors\n",
- oth_err), oth_err);
+ /* Identify keys / uids that don't have a self-sig. */
+ {
+ int has_selfsig = 0;
+ PACKET *p;
+ PKT_signature *sig;
+
+ current_component = NULL;
+ for (n = kb; n; n = n->next)
+ {
+ if (is_deleted_kbnode (n))
+ continue;
+
+ p = n->pkt;
+
+ switch (p->pkttype)
+ {
+ case PKT_PUBLIC_KEY:
+ case PKT_PUBLIC_SUBKEY:
+ case PKT_USER_ID:
+ if (current_component && ! has_selfsig)
+ missing_selfsig ++;
+ current_component = n;
+ has_selfsig = 0;
+ break;
- if (mis_selfsig)
- tty_printf (ngettext("%d user ID without valid self-signature detected\n",
- "%d user IDs without valid self-signatures detected\n",
- mis_selfsig), mis_selfsig);
+ case PKT_SIGNATURE:
+ if (! current_component || has_selfsig)
+ break;
+
+ sig = n->pkt->pkt.signature;
+
+ if (! (sig->flags.checked && sig->flags.valid))
+ break;
+
+ if (keyid_cmp (pk_keyid (pk), sig->keyid) != 0)
+ /* Different issuer, couldn't be a self-sig. */
+ break;
+
+ if (current_component->pkt->pkttype == PKT_PUBLIC_KEY
+ && (/* Direct key signature. */
+ sig->sig_class == 0x1f
+ /* Key revocation signature. */
+ || sig->sig_class == 0x20))
+ has_selfsig = 1;
+ if (current_component->pkt->pkttype == PKT_PUBLIC_SUBKEY
+ && (/* Subkey binding sig. */
+ sig->sig_class == 0x18
+ /* Subkey revocation sig. */
+ || sig->sig_class == 0x28))
+ has_selfsig = 1;
+ if (current_component->pkt->pkttype == PKT_USER_ID
+ && (/* Certification sigs. */
+ sig->sig_class == 0x10
+ || sig->sig_class == 0x11
+ || sig->sig_class == 0x12
+ || sig->sig_class == 0x13
+ /* Certification revocation sig. */
+ || sig->sig_class == 0x30))
+ has_selfsig = 1;
+
+ break;
+
+ default:
+ if (current_component && ! has_selfsig)
+ missing_selfsig ++;
+ current_component = NULL;
+ }
+ }
+ }
- return inv_sigs || no_key || oth_err || mis_selfsig;
+ if (dups || missing_issuer || bad_signature || reordered)
+ tty_printf (_("key %s:\n"), pk_keyid_str (pk));
+
+ if (dups)
+ tty_printf (ngettext ("%d duplicate signature removed\n",
+ "%d duplicate signatures removed\n", dups), dups);
+ if (missing_issuer)
+ tty_printf (ngettext ("%d signature not checked due to a missing key\n",
+ "%d signatures not checked due to missing keys\n",
+ missing_issuer), missing_issuer);
+ if (bad_signature)
+ tty_printf (ngettext ("%d bad signature\n",
+ "%d bad signatures\n",
+ bad_signature), bad_signature);
+ if (reordered)
+ tty_printf (ngettext ("%d signature reordered\n",
+ "%d signatures reordered\n",
+ reordered), reordered);
+
+ if (only_selfsigs && (bad_signature || reordered))
+ tty_printf (_("Warning: errors found and only checked self-signatures, run 'check' to check all signatures.\n"));
+
+ return modified;
}
@@ -1247,54 +1734,6 @@ change_passphrase (ctrl_t ctrl, kbnode_t keyblock)
-/*
- * There are some keys out (due to a bug in gnupg), where the sequence
- * of the packets is wrong. This function fixes that.
- * Returns: true if the keyblock has been fixed.
- *
- * Note: This function does not work if there is more than one user ID.
- */
-static int
-fix_key_signature_order (KBNODE keyblock)
-{
- KBNODE node, last, subkey;
- int fixed = 0;
-
- /* Locate key signatures of class 0x10..0x13 behind sub key packets. */
- for (subkey = last = NULL, node = keyblock; node;
- last = node, node = node->next)
- {
- switch (node->pkt->pkttype)
- {
- case PKT_PUBLIC_SUBKEY:
- case PKT_SECRET_SUBKEY:
- if (!subkey)
- subkey = last; /* Actually it is the one before the subkey. */
- break;
- case PKT_SIGNATURE:
- if (subkey)
- {
- PKT_signature *sig = node->pkt->pkt.signature;
- if (sig->sig_class >= 0x10 && sig->sig_class <= 0x13)
- {
- log_info (_("moving a key signature to the correct place\n"));
- last->next = node->next;
- node->next = subkey->next;
- subkey->next = node;
- node = last;
- fixed = 1;
- }
- }
- break;
- default:
- break;
- }
- }
-
- return fixed;
-}
-
-
/* Fix various problems in the keyblock. Returns true if the keyblock
was changed. Note that a pointer to the keyblock must be given and
the function may change it (i.e. replacing the first node). */
@@ -1303,10 +1742,10 @@ fix_keyblock (kbnode_t *keyblockp)
{
int changed = 0;
- if (fix_key_signature_order (*keyblockp))
- changed++;
if (collapse_uids (keyblockp))
changed++;
+ if (check_all_keysigs (*keyblockp, 0, 1))
+ changed++;
reorder_keyblock (*keyblockp);
/* If we modified the keyblock, make sure the flags are right. */
if (changed)
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-gnupg/gnupg2.git
More information about the Pkg-gnupg-commit
mailing list