Fix for bug 1223
authorWerner Koch <wk@gnupg.org>
Fri, 7 May 2010 13:13:56 +0000 (13:13 +0000)
committerWerner Koch <wk@gnupg.org>
Fri, 7 May 2010 13:13:56 +0000 (13:13 +0000)
g10/ChangeLog
g10/import.c
tests/openpgp/ChangeLog
tests/openpgp/Makefile.am
tests/openpgp/bug1223-bogus.asc [new file with mode: 0644]
tests/openpgp/bug1223-good.asc [new file with mode: 0644]
tests/openpgp/import.test

index 0a1a264..2014ec7 100644 (file)
@@ -1,3 +1,11 @@
+2010-05-07  Werner Koch  <wk@g10code.com>
+
+       * 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  <wk@g10code.com>
 
        * passphrase.c (gpg_format_keydesc): New.
index 6d02a49..53349ee 100644 (file)
@@ -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;
-
-    (void)fname;
-    (void)pk;
+  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;
 
-    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( 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( 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 ( 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( !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));
-                           }
+      /* 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.  */
 
-                         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;
index 6afd87c..68a9671 100644 (file)
@@ -1,3 +1,8 @@
+2010-05-07  Werner Koch  <wk@g10code.com>
+
+       * import.test: Add test case for bug#1223.
+       * bug1223-good.asc, bug1223-bogus.asc: New.
+
 2009-12-21  Werner Koch  <wk@g10code.com>
 
        * Makefile.am (required_pgms): New.
index 57d3e6d..9356cb3 100644 (file)
@@ -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 (file)
index 0000000..469e6b9
--- /dev/null
@@ -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 (file)
index 0000000..5622cb3
--- /dev/null
@@ -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-----
index 6117046..f9fe990 100755 (executable)
@@ -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
+
+