From b9f2c0daaf8f512021fdad4ae1fb1cadf9dd2e2c Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Fri, 7 May 2010 13:13:56 +0000 Subject: [PATCH] Fix for bug 1223 --- g10/ChangeLog | 8 + g10/import.c | 387 +++++++++++++++++++------------- tests/openpgp/ChangeLog | 5 + tests/openpgp/Makefile.am | 3 +- tests/openpgp/bug1223-bogus.asc | 21 ++ tests/openpgp/bug1223-good.asc | 20 ++ tests/openpgp/import.test | 15 ++ 7 files changed, 298 insertions(+), 161 deletions(-) create mode 100644 tests/openpgp/bug1223-bogus.asc create mode 100644 tests/openpgp/bug1223-good.asc diff --git a/g10/ChangeLog b/g10/ChangeLog index 0a1a264d6..2014ec748 100644 --- a/g10/ChangeLog +++ b/g10/ChangeLog @@ -1,3 +1,11 @@ +2010-05-07 Werner Koch + + * import.c (fix_bad_direct_key_sigs): New. + (import_one): Call it. + (chk_self_sigs): Re-indent, slighly re-arrange code, use test + macros for the sig class. Check direct key signatures. Fixes + bug#1223. + 2010-04-27 Werner Koch * passphrase.c (gpg_format_keydesc): New. diff --git a/g10/import.c b/g10/import.c index 6d02a4988..53349ee8d 100644 --- a/g10/import.c +++ b/g10/import.c @@ -521,6 +521,46 @@ fix_pks_corruption(KBNODE keyblock) } +/* Versions of GnuPG before 1.4.11 and 2.0.16 allowed to import bogus + direct key signatures. A side effect of this was that a later + import of the same good direct key signatures was not possible + because the cmp_signature check in merge_blocks considered them + equal. Although direct key signatures are now checked during + import, there might still be bogus signatures sitting in a keyring. + We need to detect and delete them before doing a merge. This + fucntion returns the number of removed sigs. */ +static int +fix_bad_direct_key_sigs (kbnode_t keyblock, u32 *keyid) +{ + gpg_error_t err; + kbnode_t node; + int count = 0; + + for (node = keyblock->next; node; node=node->next) + { + if (node->pkt->pkttype == PKT_USER_ID) + break; + if (node->pkt->pkttype == PKT_SIGNATURE + && IS_KEY_SIG (node->pkt->pkt.signature)) + { + err = check_key_signature (keyblock, node, NULL); + if (err && gpg_err_code (err) != GPG_ERR_PUBKEY_ALGO ) + { + /* If we don't know the error, we can't decide; this is + not a problem because cmp_signature can't compare the + signature either. */ + log_info ("key %s: invalid direct key signature removed\n", + keystr (keyid)); + delete_kbnode (node); + count++; + } + } + } + + return count; +} + + static void print_import_ok (PKT_public_key *pk, PKT_secret_key *sk, unsigned int reason) { @@ -887,10 +927,15 @@ import_one( const char *fname, KBNODE keyblock, struct stats_s *stats, goto leave; } + /* Make sure the original direct key sigs are all sane. */ + n_sigs_cleaned = fix_bad_direct_key_sigs (keyblock_orig, keyid); + if (n_sigs_cleaned) + commit_kbnode (&keyblock_orig); + /* and try to merge the block */ clear_kbnode_flags( keyblock_orig ); clear_kbnode_flags( keyblock ); - n_uids = n_sigs = n_subk = n_sigs_cleaned = n_uids_cleaned = 0; + n_uids = n_sigs = n_subk = n_uids_cleaned = 0; rc = merge_blocks( fname, keyblock_orig, keyblock, keyid, &n_uids, &n_sigs, &n_subk ); if( rc ) @@ -1354,8 +1399,8 @@ import_revoke_cert( const char *fname, KBNODE node, struct stats_s *stats ) } -/**************** - * loop over the keyblock and check all self signatures. +/* + * Loop over the keyblock and check all self signatures. * Mark all user-ids with a self-signature by setting flag bit 0. * Mark all user-ids with an invalid self-signature by setting bit 1. * This works also for subkeys, here the subkey is marked. Invalid or @@ -1364,176 +1409,198 @@ import_revoke_cert( const char *fname, KBNODE node, struct stats_s *stats ) * in this keyblock. */ static int -chk_self_sigs( const char *fname, KBNODE keyblock, +chk_self_sigs (const char *fname, kbnode_t keyblock, PKT_public_key *pk, u32 *keyid, int *non_self ) { - KBNODE n,knode=NULL; - PKT_signature *sig; - int rc; - u32 bsdate=0,rsdate=0; - KBNODE bsnode=NULL,rsnode=NULL; + kbnode_t n, knode = NULL; + PKT_signature *sig; + int rc; + u32 bsdate=0, rsdate=0; + kbnode_t bsnode = NULL, rsnode = NULL; + + (void)fname; + (void)pk; - (void)fname; - (void)pk; - - for( n=keyblock; (n = find_next_kbnode(n, 0)); ) { - if(n->pkt->pkttype==PKT_PUBLIC_SUBKEY) + for (n=keyblock; (n = find_next_kbnode (n, 0)); ) + { + if (n->pkt->pkttype == PKT_PUBLIC_SUBKEY) { - knode=n; - bsdate=0; - rsdate=0; - bsnode=NULL; - rsnode=NULL; + knode = n; + bsdate = 0; + rsdate = 0; + bsnode = NULL; + rsnode = NULL; continue; } - else if( n->pkt->pkttype != PKT_SIGNATURE ) - continue; - sig = n->pkt->pkt.signature; - if( keyid[0] == sig->keyid[0] && keyid[1] == sig->keyid[1] ) { - /* This just caches the sigs for later use. That way we - import a fully-cached key which speeds things up. */ - if(!opt.no_sig_cache) - check_key_signature(keyblock,n,NULL); + if ( n->pkt->pkttype != PKT_SIGNATURE ) + continue; + + sig = n->pkt->pkt.signature; + if ( keyid[0] != sig->keyid[0] || keyid[1] != sig->keyid[1] ) + { + *non_self = 1; + continue; + } - if( IS_UID_SIG(sig) || IS_UID_REV(sig) ) - { - KBNODE unode = find_prev_kbnode( keyblock, n, PKT_USER_ID ); - if( !unode ) - { - log_error( _("key %s: no user ID for signature\n"), - keystr(keyid)); - return -1; /* the complete keyblock is invalid */ - } + /* This just caches the sigs for later use. That way we + import a fully-cached key which speeds things up. */ + if (!opt.no_sig_cache) + check_key_signature (keyblock, n, NULL); + + if ( IS_UID_SIG(sig) || IS_UID_REV(sig) ) + { + KBNODE unode = find_prev_kbnode( keyblock, n, PKT_USER_ID ); + if ( !unode ) + { + log_error( _("key %s: no user ID for signature\n"), + keystr(keyid)); + return -1; /* The complete keyblock is invalid. */ + } + + /* If it hasn't been marked valid yet, keep trying. */ + if (!(unode->flag&1)) + { + rc = check_key_signature (keyblock, n, NULL); + if ( rc ) + { + if ( opt.verbose ) + { + char *p = utf8_to_native + (unode->pkt->pkt.user_id->name, + strlen (unode->pkt->pkt.user_id->name),0); + log_info (gpg_err_code(rc) == G10ERR_PUBKEY_ALGO ? + _("key %s: unsupported public key " + "algorithm on user ID \"%s\"\n"): + _("key %s: invalid self-signature " + "on user ID \"%s\"\n"), + keystr (keyid),p); + xfree (p); + } + } + else + unode->flag |= 1; /* Mark that signature checked. */ + } + } + else if (IS_KEY_SIG (sig)) + { + rc = check_key_signature (keyblock, n, NULL); + if ( rc ) + { + if (opt.verbose) + log_info (gpg_err_code (rc) == G10ERR_PUBKEY_ALGO ? + _("key %s: unsupported public key algorithm\n"): + _("key %s: invalid direct key signature\n"), + keystr (keyid)); + n->flag |= 4; + } + } + else if ( IS_SUBKEY_SIG (sig) ) + { + /* Note that this works based solely on the timestamps like + the rest of gpg. If the standard gets revocation + targets, this may need to be revised. */ - /* If it hasn't been marked valid yet, keep trying */ - if(!(unode->flag&1)) { - rc = check_key_signature( keyblock, n, NULL); - if( rc ) - { - if( opt.verbose ) - { - char *p=utf8_to_native(unode->pkt->pkt.user_id->name, - strlen(unode->pkt->pkt.user_id->name),0); - log_info( rc == G10ERR_PUBKEY_ALGO ? - _("key %s: unsupported public key " - "algorithm on user ID \"%s\"\n"): - _("key %s: invalid self-signature " - "on user ID \"%s\"\n"), - keystr(keyid),p); - xfree(p); - } - } - else - unode->flag |= 1; /* mark that signature checked */ - } - } - else if( sig->sig_class == 0x18 ) { - /* Note that this works based solely on the timestamps - like the rest of gpg. If the standard gets - revocation targets, this may need to be revised. */ - - if( !knode ) - { - if(opt.verbose) - log_info( _("key %s: no subkey for key binding\n"), - keystr(keyid)); - n->flag |= 4; /* delete this */ - } - else - { - rc = check_key_signature( keyblock, n, NULL); - if( rc ) - { - if(opt.verbose) - log_info(rc == G10ERR_PUBKEY_ALGO ? - _("key %s: unsupported public key" - " algorithm\n"): - _("key %s: invalid subkey binding\n"), - keystr(keyid)); - n->flag|=4; - } - else - { - /* It's valid, so is it newer? */ - if(sig->timestamp>=bsdate) { - knode->flag |= 1; /* the subkey is valid */ - if(bsnode) - { - bsnode->flag|=4; /* Delete the last binding - sig since this one is - newer */ - if(opt.verbose) - log_info(_("key %s: removed multiple subkey" - " binding\n"),keystr(keyid)); - } - - bsnode=n; - bsdate=sig->timestamp; - } - else - n->flag|=4; /* older */ - } - } - } - else if( sig->sig_class == 0x28 ) { - /* We don't actually mark the subkey as revoked right - now, so just check that the revocation sig is the - most recent valid one. Note that we don't care if - the binding sig is newer than the revocation sig. - See the comment in getkey.c:merge_selfsigs_subkey for - more */ - if( !knode ) - { - if(opt.verbose) - log_info( _("key %s: no subkey for key revocation\n"), - keystr(keyid)); - n->flag |= 4; /* delete this */ - } - else - { - rc = check_key_signature( keyblock, n, NULL); - if( rc ) - { - if(opt.verbose) - log_info(rc == G10ERR_PUBKEY_ALGO ? - _("key %s: unsupported public" - " key algorithm\n"): - _("key %s: invalid subkey revocation\n"), - keystr(keyid)); - n->flag|=4; - } - else - { - /* It's valid, so is it newer? */ - if(sig->timestamp>=rsdate) - { - if(rsnode) - { - rsnode->flag|=4; /* Delete the last revocation - sig since this one is - newer */ - if(opt.verbose) - log_info(_("key %s: removed multiple subkey" - " revocation\n"),keystr(keyid)); - } - - rsnode=n; - rsdate=sig->timestamp; - } - else - n->flag|=4; /* older */ - } - } - } - } - else - *non_self=1; + if ( !knode ) + { + if (opt.verbose) + log_info (_("key %s: no subkey for key binding\n"), + keystr (keyid)); + n->flag |= 4; /* delete this */ + } + else + { + rc = check_key_signature (keyblock, n, NULL); + if ( rc ) + { + if (opt.verbose) + log_info (gpg_err_code (rc) == G10ERR_PUBKEY_ALGO ? + _("key %s: unsupported public key" + " algorithm\n"): + _("key %s: invalid subkey binding\n"), + keystr (keyid)); + n->flag |= 4; + } + else + { + /* It's valid, so is it newer? */ + if (sig->timestamp >= bsdate) + { + knode->flag |= 1; /* The subkey is valid. */ + if (bsnode) + { + /* Delete the last binding sig since this + one is newer */ + bsnode->flag |= 4; + if (opt.verbose) + log_info (_("key %s: removed multiple subkey" + " binding\n"),keystr(keyid)); + } + + bsnode = n; + bsdate = sig->timestamp; + } + else + n->flag |= 4; /* older */ + } + } + } + else if ( IS_SUBKEY_REV (sig) ) + { + /* We don't actually mark the subkey as revoked right now, + so just check that the revocation sig is the most recent + valid one. Note that we don't care if the binding sig is + newer than the revocation sig. See the comment in + getkey.c:merge_selfsigs_subkey for more. */ + if ( !knode ) + { + if (opt.verbose) + log_info (_("key %s: no subkey for key revocation\n"), + keystr(keyid)); + n->flag |= 4; /* delete this */ + } + else + { + rc = check_key_signature (keyblock, n, NULL); + if ( rc ) + { + if(opt.verbose) + log_info (gpg_err_code (rc) == G10ERR_PUBKEY_ALGO ? + _("key %s: unsupported public" + " key algorithm\n"): + _("key %s: invalid subkey revocation\n"), + keystr(keyid)); + n->flag |= 4; + } + else + { + /* It's valid, so is it newer? */ + if (sig->timestamp >= rsdate) + { + if (rsnode) + { + /* Delete the last revocation sig since + this one is newer. */ + rsnode->flag |= 4; + if (opt.verbose) + log_info (_("key %s: removed multiple subkey" + " revocation\n"),keystr(keyid)); + } + + rsnode = n; + rsdate = sig->timestamp; + } + else + n->flag |= 4; /* older */ + } + } + } } - return 0; + return 0; } + /**************** * delete all parts which are invalid and those signatures whose * public key algorithm is not available in this implemenation; diff --git a/tests/openpgp/ChangeLog b/tests/openpgp/ChangeLog index 6afd87c63..68a9671c9 100644 --- a/tests/openpgp/ChangeLog +++ b/tests/openpgp/ChangeLog @@ -1,3 +1,8 @@ +2010-05-07 Werner Koch + + * import.test: Add test case for bug#1223. + * bug1223-good.asc, bug1223-bogus.asc: New. + 2009-12-21 Werner Koch * Makefile.am (required_pgms): New. diff --git a/tests/openpgp/Makefile.am b/tests/openpgp/Makefile.am index 57d3e6d88..9356cb376 100644 --- a/tests/openpgp/Makefile.am +++ b/tests/openpgp/Makefile.am @@ -40,7 +40,8 @@ TESTS = version.test mds.test \ TEST_FILES = pubring.asc secring.asc plain-1o.asc plain-2o.asc plain-3o.asc \ plain-1.asc plain-2.asc plain-3.asc plain-1-pgp.asc \ pubring.pkr.asc secring.skr.asc secdemo.asc pubdemo.asc \ - gpg.conf.tmpl bug537-test.data.asc bug894-test.asc + gpg.conf.tmpl bug537-test.data.asc bug894-test.asc \ + bug1223-good.asc bug1223-bogus.asc DATA_FILES = data-500 data-9000 data-32000 data-80000 plain-large diff --git a/tests/openpgp/bug1223-bogus.asc b/tests/openpgp/bug1223-bogus.asc new file mode 100644 index 000000000..469e6b95f --- /dev/null +++ b/tests/openpgp/bug1223-bogus.asc @@ -0,0 +1,21 @@ +Bogus test key for bug 1223 (Designated revoker sigs are not properly merged) +Thanks to Daniel Kahn Gillmor for providing the test keys. + +-----BEGIN PGP PUBLIC KEY BLOCK----- +Version: GnuPG v1.4.10 (GNU/Linux) + +mI0ES+OoSQEEAJUZ/+fC6DXN2X7Wxl4Huud/+i2qP1hcq+Qnbr7hVCKEnn0edYl+ +6xfsKmAMBjl+qTZxPSDSx4r3ciMiIbnvXFtlBAQmji86kqoR6fm9s8BN7LTq7+2/ +c2FHVF67D7zES7WgHc4i7CfiZnwXgkLvi5b1jBt+MTAOrFhdobxoy6/XABEBAAGI +twQfAQIAIQUCS+OsRRcMgAEAAAAAAAAAAAAAAAAAAAAAAAAAAQIHAAAKCRA0t9EL +wQjoOrRXBACBqhigTcj8pJY14AkjV+ZzUbm55kJRDPdU7NQ1PSvczm7HZaL3b8Lr +Psa5c5+caVLjsGWkQycQl7lUIGU84KoUfwACQKVVLkqJz8LkL54lLcwkG70+1NH5 +xoSNcHHVbYtqDLNeCOq5jEIoXuz44wiWVEfF+/B115PvgwZ63pjH1rRGVGVzdCBL +ZXkgRGVtb25zdHJhdGluZyBSZXZva2VyIFRyb3VibGUgKERPIE5PVCBVU0UpIDx0 +ZXN0QGV4YW1wbGUubmV0Poi+BBMBAgAoBQJL46hJAhsDBQkACTqABgsJCAcDAgYV +CAIJCgsEFgIDAQIeAQIXgAAKCRA0t9ELwQjoOgLpA/9/si2QYmietY9a6VlAmMri +mhZeqo6zyn8zrO9RGU7+8jmeb5nVnXw1YmZcw2fiJgI9+tTMkTfomyR6k0EDvcEu +2Mg3USkVnJfrrkPjSL9EajW6VpOUNxlox3ZT1oyEo3OOnVF1gC1reWYfy7Ns9zIB +1leLXbMr86zYdCoXp0Xu4g== +=YV5g +-----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/openpgp/bug1223-good.asc b/tests/openpgp/bug1223-good.asc new file mode 100644 index 000000000..5622cb3f8 --- /dev/null +++ b/tests/openpgp/bug1223-good.asc @@ -0,0 +1,20 @@ +Good test key for bug 1223 (Designated revoker sigs are not properly merged) + +-----BEGIN PGP PUBLIC KEY BLOCK----- +Version: GnuPG v1.4.10 (GNU/Linux) + +mI0ES+OoSQEEAJUZ/+fC6DXN2X7Wxl4Huud/+i2qP1hcq+Qnbr7hVCKEnn0edYl+ +6xfsKmAMBjl+qTZxPSDSx4r3ciMiIbnvXFtlBAQmji86kqoR6fm9s8BN7LTq7+2/ +c2FHVF67D7zES7WgHc4i7CfiZnwXgkLvi5b1jBt+MTAOrFhdobxoy6/XABEBAAGI +twQfAQIAIQUCS+OsRRcMgAEO5b6XkoLYC591QPHM0u2U0hc56QIHAAAKCRA0t9EL +wQjoOrRXBACBqhigTcj8pJY14AkjV+ZzUbm55kJRDPdU7NQ1PSvczm7HZaL3b8Lr +Psa5c5+caVLjsGWkQycQl7lUIGU84KoUfwACQKVVLkqJz8LkL54lLcwkG70+1NH5 +xoSNcHHVbYtqDLNeCOq5jEIoXuz44wiWVEfF+/B115PvgwZ63pjH1rRGVGVzdCBL +ZXkgRGVtb25zdHJhdGluZyBSZXZva2VyIFRyb3VibGUgKERPIE5PVCBVU0UpIDx0 +ZXN0QGV4YW1wbGUubmV0Poi+BBMBAgAoBQJL46hJAhsDBQkACTqABgsJCAcDAgYV +CAIJCgsEFgIDAQIeAQIXgAAKCRA0t9ELwQjoOgLpA/9/si2QYmietY9a6VlAmMri +mhZeqo6zyn8zrO9RGU7+8jmeb5nVnXw1YmZcw2fiJgI9+tTMkTfomyR6k0EDvcEu +2Mg3USkVnJfrrkPjSL9EajW6VpOUNxlox3ZT1oyEo3OOnVF1gC1reWYfy7Ns9zIB +1leLXbMr86zYdCoXp0Xu4g== +=xsEd +-----END PGP PUBLIC KEY BLOCK----- diff --git a/tests/openpgp/import.test b/tests/openpgp/import.test index 611704699..f9fe9907a 100755 --- a/tests/openpgp/import.test +++ b/tests/openpgp/import.test @@ -18,6 +18,21 @@ else fi +boguskey=$srcdir/bug1223-bogus.asc +goodkey=$srcdir/bug1223-good.asc +keyid=0xC108E83A +info "Checking bug 1223: designated revoker sigs are not properly merged." +$GPG --delete-key --batch --yes $keyid 2>/dev/null || true +$GPG --import $boguskey || true +$GPG --import $goodkey || true +if $GPG --list-keys --with-colons $keyid \ + | grep '^rvk:.*:0EE5BE979282D80B9F7540F1CCD2ED94D21739E9:' >/dev/null; then + : +else + error "$goodkey: import failed (bug 1223)" +fi + +