]> the.earth.li Git - onak.git/commitdiff
Improve handling of colliding 64-bit key IDs
authorJonathan McDowell <noodles@earth.li>
Thu, 1 Aug 2019 18:19:30 +0000 (19:19 +0100)
committerJonathan McDowell <noodles@earth.li>
Thu, 1 Aug 2019 18:19:30 +0000 (19:19 +0100)
Originally key retrieval was all performed on the 64 bit key ID but back
in 2013 support was added for fetching by fingerprint. However not all
the pieces to deal with colliding 64 bit IDs were added at this time.
This commit improves things by:

 * Using the fingerprint to retrieve the key to update in update_keys()
 * Returning all keys that share a 64 bit key ID from the DB4 backend,
   rather than just the first found.
 * Adding a test to ensure multiple keys are returned for a colliding
   key lookup.

It also removes the old compatibility code for the DB4 backend with key
lookups by 64 bit key ID.

Note this isn't yet common in the wild; unlike Evil32 it is not possible
to create collisions for arbitrary key IDs.

keydb.c
keydb_db4.c
keys/DDA252EBB8EBE1AF-1.key [new file with mode: 0644]
keys/DDA252EBB8EBE1AF-2.key [new file with mode: 0644]
keys/README
t/all-046-index-64bit-dupe.t [new file with mode: 0755]

diff --git a/keydb.c b/keydb.c
index f8d5a0bd8ef60248157542ee55ec356a06a0ae4d..deedc4994e5f4441938ab3ef709c4e44e46034ba 100644 (file)
--- a/keydb.c
+++ b/keydb.c
@@ -200,17 +200,16 @@ int generic_update_keys(struct onak_dbctx *dbctx,
        struct openpgp_publickey *curkey = NULL;
        struct openpgp_publickey *oldkey = NULL;
        struct openpgp_publickey *prev = NULL;
+       struct openpgp_fingerprint fp;
        int newkeys = 0;
        bool intrans;
-       uint64_t keyid;
 
        for (curkey = *keys; curkey != NULL; curkey = curkey->next) {
                intrans = dbctx->starttrans(dbctx);
-               get_keyid(curkey, &keyid);
+               get_fingerprint(curkey->publickey, &fp);
                logthing(LOGTHING_INFO,
-                       "Fetching key 0x%" PRIX64 ", result: %d",
-                       keyid,
-                       dbctx->fetch_key_id(dbctx, keyid, &oldkey,
+                       "Fetching key, result: %d",
+                       dbctx->fetch_key_fp(dbctx, &fp, &oldkey,
                                        intrans));
 
                /*
index f3d20dbcafd3ff8521981b455a05c110418f8dec..aa2f229c01c3c4fc5f3e6d7dd1bd69b6e3045c18 100644 (file)
@@ -294,7 +294,11 @@ static int db4_upgradedb(struct onak_dbctx *dbctx)
  *     @keyid: The 32bit keyid.
  *
  *     This function maps a 32bit key id to the full 64bit one. It returns the
- *     full keyid. If the key isn't found a keyid of 0 is returned.
+ *     first full keyid that has this short keyid. If the key isn't found a
+ *     keyid of 0 is returned.
+ *
+ *     FIXME: This should either return the fingerprint or ideally go away
+ *     entirely.
  */
 static uint64_t db4_getfullkeyid(struct onak_dbctx *dbctx, uint64_t keyid)
 {
@@ -329,14 +333,11 @@ static uint64_t db4_getfullkeyid(struct onak_dbctx *dbctx, uint64_t keyid)
                        DB_SET);
 
                if (ret == 0) {
-                       if (data.size == 8) {
-                               keyid = * (uint64_t *) data.data;
-                       } else {
-                               keyid = 0;
-                               for (i = 12; i < 20; i++) {
-                                       keyid <<= 8;
-                                       keyid |= ((uint8_t *) data.data)[i];
-                               }
+                       keyid = 0;
+                       /* Only works for v4, not v3 or v5 */
+                       for (i = 12; i < 20; i++) {
+                               keyid <<= 8;
+                               keyid |= ((uint8_t *) data.data)[i];
                        }
 
                        if (data.data != NULL) {
@@ -465,87 +466,68 @@ static int db4_fetch_key_id(struct onak_dbctx *dbctx, uint64_t keyid,
        struct onak_db4_dbctx *privctx = (struct onak_db4_dbctx *) dbctx->priv;
        struct openpgp_packet_list *packets = NULL;
        DBT key, data;
+       DBC *cursor = NULL;
        int ret = 0;
        int numkeys = 0;
-       struct buffer_ctx fetchbuf;
+       uint32_t  shortkeyid = 0;
        struct openpgp_fingerprint fingerprint;
+       bool first;
 
-       if (keyid < 0x100000000LL) {
-               keyid = db4_getfullkeyid(dbctx, keyid);
+       if (!intrans) {
+               db4_starttrans(dbctx);
        }
 
-       memset(&key, 0, sizeof(key));
-       memset(&data, 0, sizeof(data));
+       /* If the key ID fits in 32 bits assume it's a short key id */
+       if (keyid < 0x100000000LL) {
+               ret = privctx->id32db->cursor(privctx->id32db,
+                               privctx->txn,
+                               &cursor,
+                               0);   /* flags */
 
-       data.size = 0;
-       data.data = NULL;
+               shortkeyid = keyid & 0xFFFFFFFF;
+               memset(&key, 0, sizeof(key));
+               memset(&data, 0, sizeof(data));
+               key.data = &shortkeyid;
+               key.size = sizeof(shortkeyid);
+       } else {
+               ret = privctx->id64db->cursor(privctx->id64db,
+                               privctx->txn,
+                               &cursor,
+                               0); /* flags*/
 
-       key.size = sizeof(keyid);
-       key.data = &keyid;
+               memset(&key, 0, sizeof(key));
+               memset(&data, 0, sizeof(data));
+               key.data = &keyid;
+               key.size = sizeof(keyid);
+       }
 
-       if (!intrans) {
-               db4_starttrans(dbctx);
+       if (ret != 0) {
+               return 0;
        }
 
-       /*
-        * First we try a legacy stored key where we used the 64 bit key ID
-        * as the DB key.
-        */
-       ret = keydb_id(privctx, keyid)->get(keydb_id(privctx, keyid),
-                       privctx->txn,
-                       &key,
-                       &data,
-                       0); /* flags*/
+       memset(&data, 0, sizeof(data));
+       data.ulen = MAX_FINGERPRINT_LEN;
+       data.data = fingerprint.fp;
+       data.flags = DB_DBT_USERMEM;
+
+       first = true;
+       while (cursor->c_get(cursor, &key, &data,
+                               first ? DB_SET : DB_NEXT_DUP) == 0) {
+               /* We got a match; retrieve the actual key */
+               fingerprint.length = data.size;
+
+               if (db4_fetch_key_fp(dbctx, &fingerprint,
+                                       publickey, true))
+                       numkeys++;
 
-       if (ret == DB_NOTFOUND) {
-               /* If we didn't find the key ID try the 64 bit map DB */
-               memset(&key, 0, sizeof(key));
                memset(&data, 0, sizeof(data));
                data.ulen = MAX_FINGERPRINT_LEN;
                data.data = fingerprint.fp;
                data.flags = DB_DBT_USERMEM;
-               key.size = sizeof(keyid);
-               key.data = &keyid;
-
-               ret = privctx->id64db->get(privctx->id64db,
-                       privctx->txn,
-                       &key,
-                       &data,
-                       0); /* flags*/
-
-               if (ret == 0) {
-                       /* We got a match; retrieve the actual key */
-                       fingerprint.length = data.size;
-
-                       memset(&key, 0, sizeof(key));
-                       memset(&data, 0, sizeof(data));
-                       key.size = fingerprint.length;
-                       key.data = fingerprint.fp;
-
-                       ret = keydb_fp(privctx, &fingerprint)->get(
-                               keydb_fp(privctx, &fingerprint),
-                               privctx->txn,
-                               &key,
-                               &data,
-                               0); /* flags*/
-               }
-       }
-
-       if (ret == 0) {
-               fetchbuf.buffer = data.data;
-               fetchbuf.offset = 0;
-               fetchbuf.size = data.size;
-               read_openpgp_stream(buffer_fetchchar, &fetchbuf,
-                               &packets, 0);
-               parse_keys(packets, publickey);
-               free_packet_list(packets);
-               packets = NULL;
-               numkeys++;
-       } else if (ret != DB_NOTFOUND) {
-               logthing(LOGTHING_ERROR,
-                               "Problem retrieving key: %s",
-                               db_strerror(ret));
+               first = false;
        }
+       cursor->c_close(cursor);
+       cursor = NULL;
 
        if (!intrans) {
                db4_endtrans(dbctx);
@@ -615,18 +597,9 @@ static int db4_fetch_key_text(struct onak_dbctx *dbctx, const char *search,
                while (ret == 0 && strncmp(key.data, curword->object,
                                        key.size) == 0 &&
                                ((char *) curword->object)[key.size] == 0) {
-                       if (data.size == 12) {
-                               /* Old style creation + key id */
-                               fingerprint.length = 8;
-                               for (i = 4; i < 12; i++) {
-                                       fingerprint.fp[i - 4] =
-                                               ((unsigned char *)
-                                                       data.data)[i];
-                               }
-                       } else {
-                               fingerprint.length = data.size;
-                               memcpy(fingerprint.fp, data.data, data.size);
-                       }
+
+                       fingerprint.length = data.size;
+                       memcpy(fingerprint.fp, data.data, data.size);
 
                        /*
                         * Only add the keys containing this word if this is
@@ -669,20 +642,9 @@ static int db4_fetch_key_text(struct onak_dbctx *dbctx, const char *search,
 
        db4_starttrans(dbctx);
        for (i = 0; i < keylist.count; i++) {
-               if (keylist.keys[i].length == 8) {
-                       keyid = 0;
-                       for (int j = 0; j < 8; j++) {
-                               keyid <<= 8;
-                               keyid |= keylist.keys[i].fp[j];
-                       }
-                       numkeys += db4_fetch_key_id(dbctx, keyid,
-                               publickey,
-                               true);
-               } else {
-                       numkeys += db4_fetch_key_fp(dbctx, &keylist.keys[i],
-                               publickey,
-                               true);
-               }
+               numkeys += db4_fetch_key_fp(dbctx, &keylist.keys[i],
+                       publickey,
+                       true);
        }
        array_free(&keylist);
        free(searchtext);
@@ -718,7 +680,9 @@ static int db4_fetch_key_skshash(struct onak_dbctx *dbctx,
        memset(&data, 0, sizeof(data));
        key.data = (void *) hash->hash;
        key.size = sizeof(hash->hash);
-       data.flags = DB_DBT_MALLOC;
+       data.ulen = MAX_FINGERPRINT_LEN;
+       data.data = fingerprint.fp;
+       data.flags = DB_DBT_USERMEM;
 
        ret = cursor->c_get(cursor,
                &key,
@@ -726,22 +690,9 @@ static int db4_fetch_key_skshash(struct onak_dbctx *dbctx,
                DB_SET);
 
        if (ret == 0) {
-               if (data.size == 8) {
-                       /* Legacy key ID record */
-                       keyid = *(uint64_t *) data.data;
-                       count = db4_fetch_key_id(dbctx, keyid, publickey,
-                               false);
-               } else {
-                       fingerprint.length = data.size;
-                       memcpy(fingerprint.fp, data.data, data.size);
-                       count = db4_fetch_key_fp(dbctx, &fingerprint,
-                               publickey, false);
-               }
-
-               if (data.data != NULL) {
-                       free(data.data);
-                       data.data = NULL;
-               }
+               fingerprint.length = data.size;
+               count = db4_fetch_key_fp(dbctx, &fingerprint,
+                       publickey, false);
        }
 
        cursor->c_close(cursor);
@@ -819,32 +770,9 @@ static int db4_delete_key(struct onak_dbctx *dbctx,
                        data.size = sizeof(worddb_data);
 
                        /*
-                        * Old format word db data was the key creation time
-                        * followed by the 64 bit key id.
+                        * New style uses the fingerprint as the data
+                        * Old (unsupported) style was the 64 bit keyid
                         */
-                       worddb_data[ 0] = publickey->publickey->data[1];
-                       worddb_data[ 1] = publickey->publickey->data[2];
-                       worddb_data[ 2] = publickey->publickey->data[3];
-                       worddb_data[ 3] = publickey->publickey->data[4];
-                       worddb_data[ 4] = (keyid >> 56) & 0xFF;
-                       worddb_data[ 5] = (keyid >> 48) & 0xFF;
-                       worddb_data[ 6] = (keyid >> 40) & 0xFF;
-                       worddb_data[ 7] = (keyid >> 32) & 0xFF;
-                       worddb_data[ 8] = (keyid >> 24) & 0xFF;
-                       worddb_data[ 9] = (keyid >> 16) & 0xFF;
-                       worddb_data[10] = (keyid >>  8) & 0xFF;
-                       worddb_data[11] = keyid & 0xFF;
-
-                       ret = cursor->c_get(cursor,
-                               &key,
-                               &data,
-                               DB_GET_BOTH);
-
-                       if (ret == 0) {
-                               cursor->c_del(cursor, 0);
-                       }
-
-                       /* New style just uses the fingerprint as the data */
                        memset(&key, 0, sizeof(key));
                        memset(&data, 0, sizeof(data));
                        key.data = curword->object;
@@ -897,26 +825,9 @@ static int db4_delete_key(struct onak_dbctx *dbctx,
                        &cursor64,
                        0);   /* flags */
 
+               /* 32 bit short key mapping to fingerprint */
                shortkeyid = keyid & 0xFFFFFFFF;
 
-               /* Old style mapping to 64 bit key id */
-               memset(&key, 0, sizeof(key));
-               memset(&data, 0, sizeof(data));
-               key.data = &shortkeyid;
-               key.size = sizeof(shortkeyid);
-               data.data = &keyid;
-               data.size = sizeof(keyid);
-
-               ret = cursor->c_get(cursor,
-                       &key,
-                       &data,
-                       DB_GET_BOTH);
-
-               if (ret == 0) {
-                       cursor->c_del(cursor, 0);
-               }
-
-               /* New style mapping to fingerprint */
                memset(&key, 0, sizeof(key));
                memset(&data, 0, sizeof(data));
                key.data = &shortkeyid;
@@ -994,23 +905,6 @@ static int db4_delete_key(struct onak_dbctx *dbctx,
 
                        shortkeyid = subkeyid & 0xFFFFFFFF;
 
-                       /* Remove 32 bit keyid -> 64 bit keyid mapping */
-                       memset(&key, 0, sizeof(key));
-                       memset(&data, 0, sizeof(data));
-                       key.data = &shortkeyid;
-                       key.size = sizeof(shortkeyid);
-                       data.data = &keyid;
-                       data.size = sizeof(keyid);
-
-                       ret = cursor->c_get(cursor,
-                               &key,
-                               &data,
-                               DB_GET_BOTH);
-
-                       if (ret == 0) {
-                               cursor->c_del(cursor, 0);
-                       }
-
                        /* Remove 32 bit keyid -> fingerprint mapping */
                        memset(&key, 0, sizeof(key));
                        memset(&data, 0, sizeof(data));
@@ -1086,24 +980,7 @@ static int db4_delete_key(struct onak_dbctx *dbctx,
                if (ret == 0) {
                        get_skshash(publickey, &hash);
 
-                       /* First delete old style keyid mapping */
-                       memset(&key, 0, sizeof(key));
-                       memset(&data, 0, sizeof(data));
-                       key.data = hash.hash;
-                       key.size = sizeof(hash.hash);
-                       data.data = &keyid;
-                       data.size = sizeof(keyid);
-
-                       ret = cursor->c_get(cursor,
-                               &key,
-                               &data,
-                               DB_GET_BOTH);
-
-                       if (ret == 0) {
-                               cursor->c_del(cursor, 0);
-                       }
-
-                       /* Then delete new style fingerprint mapping */
+                       /* Remove SKS hash -> fingerprint mapping */
                        memset(&key, 0, sizeof(key));
                        memset(&data, 0, sizeof(data));
                        key.data = hash.hash;
@@ -1243,8 +1120,8 @@ static int db4_store_key(struct onak_dbctx *dbctx,
                write_openpgp_stream(buffer_putchar, &storebuf, packets);
 
                /*
-                * Now we have the key data store it in the DB; the keyid is
-                * the key.
+                * Now we have the key data store it in the DB; the fingerprint
+                * is the key.
                 */
                memset(&key, 0, sizeof(key));
                memset(&data, 0, sizeof(data));
@@ -1278,7 +1155,8 @@ static int db4_store_key(struct onak_dbctx *dbctx,
        }
 
        /*
-        * Walk through our uids storing the words into the db with the keyid.
+        * Walk through our uids storing the words into the db with the
+        * fingerprint.
         */
        if (!deadlock) {
                uids = keyuids(publickey, &primary);
diff --git a/keys/DDA252EBB8EBE1AF-1.key b/keys/DDA252EBB8EBE1AF-1.key
new file mode 100644 (file)
index 0000000..a192c3c
Binary files /dev/null and b/keys/DDA252EBB8EBE1AF-1.key differ
diff --git a/keys/DDA252EBB8EBE1AF-2.key b/keys/DDA252EBB8EBE1AF-2.key
new file mode 100644 (file)
index 0000000..96aff09
Binary files /dev/null and b/keys/DDA252EBB8EBE1AF-2.key differ
index ac64056ec5340119d56fd5223df8f70c29ed9094..2244d3eaab8d87369d9a84af4b135a722e802175 100644 (file)
@@ -24,3 +24,7 @@ elgv3.key
        A v3 key using Elgamal (generated by older version of GnuPG)
 test-v5.key
        A v5 sample key as provided by Werner Koch on the OpenPGP mailing list
+DDA252EBB8EBE1AF-1.key
+DDA252EBB8EBE1AF-2.key
+       A pair of keys sharing a 64 bit keyid, generated by David Leon Gil and
+       taken from https://github.com/coruus/cooperpair/tree/master/pgpv4
diff --git a/t/all-046-index-64bit-dupe.t b/t/all-046-index-64bit-dupe.t
new file mode 100755 (executable)
index 0000000..23b326f
--- /dev/null
@@ -0,0 +1,26 @@
+#!/bin/sh
+# Check we can index a key by some uid text
+
+set -e
+
+# Backends should really support storing keys with full fingerprints,
+# but the file + fs backends only do 64 bit keys IDs for simplicity.
+# Skip the test for them.
+if [ "$2" = "file" -o "$2" = "fs" ]; then
+       exit 0
+fi
+
+cd ${WORKDIR}
+# Import 2 keys with the same 64 bit key ID
+${BUILDDIR}/onak -b -c $1 add < ${TESTSDIR}/../keys/DDA252EBB8EBE1AF-1.key
+${BUILDDIR}/onak -b -c $1 add < ${TESTSDIR}/../keys/DDA252EBB8EBE1AF-2.key
+# Add an extra key so we know we're not just returning all of them
+${BUILDDIR}/onak -b -c $1 add < ${TESTSDIR}/../keys/noodles.key
+c=$(${BUILDDIR}/onak -c $1 index 0xDDA252EBB8EBE1AF 2> /dev/null | \
+               grep -c -- '^pub   ')
+if [ $c != 2 ]; then
+       echo "* Did not correctly retrieve keys with duplicate 64-bit keyids"
+       exit 1
+fi
+
+exit 0