From 080ccd957a87499f94ed235f127c55aff162e9f9 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Mon, 2 Sep 2013 17:31:13 +0930 Subject: [PATCH 01/11] Replace str_fromprint() with strn_fromprint() --- commandline.c | 2 +- str.c | 23 ++++++++++++++--------- str.h | 2 +- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/commandline.c b/commandline.c index 50524c3d..3338a5c9 100644 --- a/commandline.c +++ b/commandline.c @@ -482,7 +482,7 @@ int app_echo(const struct cli_parsed *parsed, struct cli_context *context) DEBUGF("echo:argv[%d]=\"%s\"", i, arg); if (escapes) { unsigned char buf[strlen(arg)]; - size_t len = str_fromprint(buf, arg); + size_t len = strn_fromprint(buf, sizeof buf, arg, '\0', NULL); cli_write(context, buf, len); } else cli_puts(context, arg); diff --git a/str.c b/str.c index e8d5f0a9..bc95951f 100644 --- a/str.c +++ b/str.c @@ -343,37 +343,42 @@ size_t toprint_str_len(const char *srcStr, const char quotes[2]) return srcStr ? strbuf_count(strbuf_toprint_quoted(strbuf_local(NULL, 0), quotes, srcStr)) : 4; } -size_t str_fromprint(unsigned char *dst, const char *src) +size_t strn_fromprint(unsigned char *dst, size_t dstlen, const char *src, char endquote, const char **afterp) { unsigned char *const odst = dst; - while (*src) { + unsigned char *const edst = dst + dstlen; + while (*src && *src != endquote && dst < edst) { switch (*src) { case '\\': ++src; + unsigned char d; switch (*src) { - case '\0': *dst++ = '\\'; break; - case '0': *dst++ = '\0'; ++src; break; - case 'n': *dst++ = '\n'; ++src; break; - case 'r': *dst++ = '\r'; ++src; break; - case 't': *dst++ = '\t'; ++src; break; + case '\0': d = '\\'; break; + case '0': d = '\0'; ++src; break; + case 'n': d = '\n'; ++src; break; + case 'r': d = '\r'; ++src; break; + case 't': d = '\t'; ++src; break; case 'x': if (isxdigit(src[1]) && isxdigit(src[2])) { ++src; - fromhex(dst++, src, 1); + fromhex(&d, src, 1); src += 2; break; } // fall through default: - *dst++ = *src++; + d = *src++; break; } + *dst++ = d; break; default: *dst++ = *src++; break; } } + if (afterp) + *afterp = src; return dst - odst; } diff --git a/str.h b/str.h index 8acc7731..3a5d0bc3 100644 --- a/str.h +++ b/str.h @@ -81,7 +81,7 @@ char *toprint(char *dstStr, ssize_t dstBufSiz, const char *srcBuf, size_t srcByt char *toprint_str(char *dstStr, ssize_t dstBufSiz, const char *srcStr, const char quotes[2]); size_t toprint_len(const char *srcBuf, size_t srcBytes, const char quotes[2]); size_t toprint_str_len(const char *srcStr, const char quotes[2]); -size_t str_fromprint(unsigned char *dst, const char *src); +size_t strn_fromprint(unsigned char *dst, size_t dstlen, const char *src, char endquote, const char **afterp); #define alloca_toprint(dstlen,buf,len) toprint((char *)alloca((dstlen) == -1 ? toprint_len((const char *)(buf),(len), "``") + 1 : (dstlen)), (dstlen), (const char *)(buf), (len), "``") #define alloca_str_toprint_quoted(str, quotes) toprint_str((char *)alloca(toprint_str_len((str), (quotes)) + 1), -1, (str), (quotes)) From 425aa1005c563b0d69a09fa3fa17a8a67e9bd035 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Mon, 2 Sep 2013 17:33:52 +0930 Subject: [PATCH 02/11] Start work on "keyring load" command --- commandline.c | 37 +++- keyring.c | 463 ++++++++++++++++++++++++++++++-------------------- serval.h | 2 +- 3 files changed, 312 insertions(+), 190 deletions(-) diff --git a/commandline.c b/commandline.c index 3338a5c9..58db5f14 100644 --- a/commandline.c +++ b/commandline.c @@ -1768,6 +1768,35 @@ int app_keyring_dump(const struct cli_parsed *parsed, struct cli_context *contex return ret; } +int app_keyring_load(const struct cli_parsed *parsed, struct cli_context *context) +{ + if (config.debug.verbose) + DEBUG_cli_parsed(parsed); + const char *path; + if (cli_arg(parsed, "file", &path, cli_path_regular, NULL) == -1) + return -1; + unsigned pinc = 0; + unsigned i; + for (i = 0; i < parsed->labelc; ++i) + if (strn_str_cmp(parsed->labelv[i].label, parsed->labelv[i].len, "pin") == 0) + ++pinc; + const char *pinv[pinc]; + unsigned pc = 0; + for (i = 0; i < parsed->labelc; ++i) + if (strn_str_cmp(parsed->labelv[i].label, parsed->labelv[i].len, "pin") == 0) { + assert(pc < pinc); + pinv[pc++] = parsed->labelv[i].text; + } + keyring_file *k = keyring_open_instance_cli(parsed); + if (!k) + return -1; + FILE *fp = path && strcmp(path, "-") != 0 ? fopen(path, "r") : stdin; + if (fp == NULL) + return WHYF_perror("fopen(%s, \"r\")", alloca_str_toprint(path)); + int ret = keyring_load(k, pinc, pinv, fp); + return ret; +} + int app_keyring_list(const struct cli_parsed *parsed, struct cli_context *context) { if (config.debug.verbose) @@ -2299,7 +2328,9 @@ int app_network_scan(const struct cli_parsed *parsed, struct cli_context *contex /* See cli_parse() for details of command specification syntax. */ -#define KEYRING_PIN_OPTIONS ,"[--keyring-pin=]","[--entry-pin=]..." +#define KEYRING_PIN_OPTION ,"[--keyring-pin=]" +#define KEYRING_ENTRY_PIN_OPTION ,"[--entry-pin=]" +#define KEYRING_PIN_OPTIONS KEYRING_PIN_OPTION KEYRING_ENTRY_PIN_OPTION "..." struct cli_schema command_line_options[]={ {commandline_usage,{"help|-h|--help","...",NULL},CLIFLAG_PERMISSIVE_CONFIG, "Display command usage."}, @@ -2307,7 +2338,7 @@ struct cli_schema command_line_options[]={ "Output the supplied string."}, {app_log,{"log","error|warn|hint|info|debug","",NULL},CLIFLAG_PERMISSIVE_CONFIG, "Log the supplied message at given level."}, - {app_server_start,{"start","[foreground|exec ]",NULL}, 0, + {app_server_start,{"start" KEYRING_PIN_OPTIONS, "[foreground|exec ]",NULL}, 0, "Start daemon with instance path from SERVALINSTANCE_PATH environment variable."}, {app_server_stop,{"stop",NULL},CLIFLAG_PERMISSIVE_CONFIG, "Stop a running daemon with instance path from SERVALINSTANCE_PATH environment variable."}, @@ -2380,6 +2411,8 @@ struct cli_schema command_line_options[]={ "Create a new keyring file."}, {app_keyring_dump,{"keyring","dump" KEYRING_PIN_OPTIONS,"[--secret]","[]",NULL}, 0, "Dump all keyring identities that can be accessed using the specified PINs"}, + {app_keyring_load,{"keyring","load" KEYRING_PIN_OPTIONS,"","[]...",NULL}, 0, + "Load identities from the given dump text and insert them into the keyring using the specified entry PINs"}, {app_keyring_list,{"keyring","list" KEYRING_PIN_OPTIONS,NULL}, 0, "List identities that can be accessed using the supplied PINs"}, {app_keyring_add,{"keyring","add" KEYRING_PIN_OPTIONS,"[]",NULL}, 0, diff --git a/keyring.c b/keyring.c index 9cc2db99..6b868b54 100644 --- a/keyring.c +++ b/keyring.c @@ -28,6 +28,8 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. #include "nacl.h" #include "overlay_address.h" +static void keyring_free_keypair(keypair *kp); + static int _keyring_open(keyring_file *k, const char *path, const char *mode) { if (config.debug.keyring) @@ -205,20 +207,30 @@ void keyring_free(keyring_file *k) return; } +static void wipestr(char *str) +{ + while (*str) { + if (config.debug.keyring) + DEBUGF("wiping PIN char '%c'", *str); + *str++ = ' '; + } +} + void keyring_free_context(keyring_context *c) { int i; if (!c) return; if (c->KeyRingPin) { - /* Wipe pin before freeing (slightly tricky since this is a variable length string */ - for(i=0;c->KeyRingPin[i];i++) c->KeyRingPin[i]=' '; i=0; - free(c->KeyRingPin); c->KeyRingPin=NULL; + /* Wipe pin from local memory before freeing. */ + wipestr(c->KeyRingPin); + free(c->KeyRingPin); + c->KeyRingPin = NULL; } if (c->KeyRingSalt) { bzero(c->KeyRingSalt,c->KeyRingSaltLen); - c->KeyRingSalt=NULL; - c->KeyRingSaltLen=0; + c->KeyRingSalt = NULL; + c->KeyRingSaltLen = 0; } /* Wipe out any loaded identities */ @@ -233,50 +245,25 @@ void keyring_free_context(keyring_context *c) void keyring_free_identity(keyring_identity *id) { - int i; if (id->PKRPin) { - /* Wipe pin before freeing (slightly tricky since this is a variable length string */ - for(i=0;id->PKRPin[i];i++) { - if (config.debug.keyring) - DEBUGF("clearing PIN char '%c'", id->PKRPin[i]); - id->PKRPin[i]=' '; - } - i=0; - - free(id->PKRPin); id->PKRPin=NULL; + /* Wipe pin from local memory before freeing. */ + wipestr(id->PKRPin); + free(id->PKRPin); + id->PKRPin = NULL; } - + int i; for(i=0;ikeypairs[i]) keyring_free_keypair(id->keypairs[i]); - - if (id->subscriber){ + if (id->subscriber) { id->subscriber->identity=NULL; if (id->subscriber->reachable == REACHABLE_SELF) id->subscriber->reachable = REACHABLE_NONE; } - bzero(id,sizeof(keyring_identity)); return; } -void keyring_free_keypair(keypair *kp) -{ - if (kp->private_key) { - bzero(kp->private_key,kp->private_key_len); - free(kp->private_key); - kp->private_key=NULL; - } - if (kp->public_key) { - bzero(kp->public_key,kp->public_key_len); - free(kp->public_key); - kp->public_key=NULL; - } - - bzero(kp,sizeof(keypair)); - return; -} - /* Create a new keyring context for the loaded keyring file. We don't need to load any identities etc, as that happens when we enter an identity pin. @@ -421,21 +408,22 @@ struct keytype { size_t public_key_size; size_t private_key_size; size_t packed_size; - int (*packer)(const struct keytype *, const keypair *, struct rotbuf *); + int (*packer)(const keypair *, struct rotbuf *); int (*unpacker)(const struct keytype *, keypair *, struct rotbuf *); void (*dumper)(const keypair *, XPRINTF, int); + int (*loader)(const keypair *, const char *); }; -static int pack_private_only(const struct keytype *kt, const keypair *kp, struct rotbuf *rb) +static int pack_private_only(const keypair *kp, struct rotbuf *rb) { - rotbuf_putbuf(rb, kp->private_key, kt->private_key_size); + rotbuf_putbuf(rb, kp->private_key, kp->private_key_len); return 0; } -static int pack_private_public(const struct keytype *kt, const keypair *kp, struct rotbuf *rb) +static int pack_private_public(const keypair *kp, struct rotbuf *rb) { - rotbuf_putbuf(rb, kp->private_key, kt->private_key_size); - rotbuf_putbuf(rb, kp->public_key, kt->public_key_size); + rotbuf_putbuf(rb, kp->private_key, kp->private_key_len); + rotbuf_putbuf(rb, kp->public_key, kp->public_key_len); return 0; } @@ -447,6 +435,11 @@ static void dump_raw_hex(const keypair *kp, XPRINTF xpf, int include_secret) xprintf(xpf, " sec=%s", alloca_tohex(kp->private_key, kp->private_key_len)); } +static int load_raw_hex(const keypair *kp, const char *text) +{ + return 0; +} + static int unpack_private_public(const struct keytype *kt, keypair *kp, struct rotbuf *rb) { rotbuf_getbuf(rb, kp->private_key, kt->private_key_size); @@ -480,12 +473,12 @@ static int unpack_private_derive_scalarmult_public(const struct keytype *kt, key return 0; } -static int pack_did_name(const struct keytype *kt, const keypair *kp, struct rotbuf *rb) +static int pack_did_name(const keypair *kp, struct rotbuf *rb) { // Ensure name is nul terminated. - if (strnchr((const char *)kp->public_key, kt->public_key_size, '\0') == NULL) + if (strnchr((const char *)kp->public_key, kp->public_key_len, '\0') == NULL) return WHY("missing nul terminator"); - return pack_private_public(kt, kp, rb); + return pack_private_public(kp, rb); } static int unpack_did_name(const struct keytype *kt, keypair *kp, struct rotbuf *rb) @@ -502,6 +495,25 @@ static void dump_did_name(const keypair *kp, XPRINTF xpf, int include_secret) xprintf(xpf, " Name=%s", alloca_str_toprint_quoted((const char *)kp->public_key, "\"\"")); } +static int load_did_name(const keypair *kp, const char *text) +{ + const char *s; + const char *t; + if ((s = strstr(text, " DID=\"")) == NULL) + return WHY("missing DID"); + bzero(kp->private_key, kp->private_key_len); + strn_fromprint(kp->private_key, kp->private_key_len, s + 6, '"', &t); + if (*t != '"') + return WHY("malformed DID quoted string"); + if ((s = strstr(text, " Name=\"")) == NULL) + return WHY("missing Name"); + bzero(kp->public_key, kp->public_key_len); + strn_fromprint(kp->public_key, kp->public_key_len, s + 6, '"', &t); + if (*t != '"') + return WHY("malformed Name quoted string"); + return 0; +} + /* This is where all the supported key types are declared. In order to preserve backward * compatibility (reading keyring files from older versions of Serval DNA), DO NOT ERASE OR RE-USE * ANY KEY TYPE ENTRIES FROM THIS ARRAY. If a key type is no longer used, it must be permanently @@ -518,7 +530,8 @@ const struct keytype keytypes[] = { .packed_size = crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES, .packer = pack_private_only, .unpacker = unpack_private_derive_scalarmult_public, - .dumper = dump_raw_hex + .dumper = dump_raw_hex, + .loader = load_raw_hex }, [KEYTYPE_CRYPTOSIGN] = { /* The NaCl API does not expose any method to derive a cryptosign public key from its private @@ -531,7 +544,8 @@ const struct keytype keytypes[] = { .packed_size = crypto_sign_edwards25519sha512batch_SECRETKEYBYTES + crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES, .packer = pack_private_public, .unpacker = unpack_private_public, - .dumper = dump_raw_hex + .dumper = dump_raw_hex, + .loader = load_raw_hex }, [KEYTYPE_RHIZOME] = { /* The Rhizome Secret (a large, unguessable number) is stored in the private key field, and @@ -542,7 +556,8 @@ const struct keytype keytypes[] = { .packed_size = 32, .packer = pack_private_only, .unpacker = unpack_private_only, - .dumper = dump_raw_hex + .dumper = dump_raw_hex, + .loader = load_raw_hex }, [KEYTYPE_DID] = { /* The DID is stored in unpacked form in the private key field, and the name in nul-terminated @@ -553,11 +568,58 @@ const struct keytype keytypes[] = { .packed_size = 32 + 64, .packer = pack_did_name, .unpacker = unpack_did_name, - .dumper = dump_did_name + .dumper = dump_did_name, + .loader = load_did_name } // ADD MORE KEY TYPES HERE }; +static void keyring_free_keypair(keypair *kp) +{ + if (kp->private_key) { + bzero(kp->private_key, kp->private_key_len); + free(kp->private_key); + } + if (kp->public_key) { + bzero(kp->public_key, kp->public_key_len); + free(kp->public_key); + } + bzero(kp, sizeof(keypair)); +} + +static keypair *keyring_alloc_keypair(unsigned ktype, size_t len) +{ + assert(ktype != 0); + keypair *kp = emalloc_zero(sizeof(keypair)); + if (!kp) + return NULL; + kp->type = ktype; + if (ktype < NELS(keytypes)) { + kp->private_key_len = keytypes[ktype].private_key_size; + kp->public_key_len = keytypes[ktype].public_key_size; + if (kp->private_key_len + kp->public_key_len != len) { + WHYF("keytype=%u private_key_len(%u) + public_key_len(%u) != len(%u)", + ktype, + kp->private_key_len, + kp->public_key_len, + len + ); + keyring_free_keypair(kp); + return NULL; + } + } else { + kp->private_key_len = len; + kp->public_key_len = 0; + } + if ( (kp->private_key_len && (kp->private_key = emalloc_zero(kp->private_key_len)) == NULL) + || (kp->public_key_len && (kp->public_key = emalloc_zero(kp->public_key_len)) == NULL) + ) { + keyring_free_keypair(kp); + return NULL; + } + return kp; +} + static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsigned char packed[KEYRING_PAGE_SIZE]) { /* Convert an identity to a KEYRING_PAGE_SIZE bytes long block that consists of 32 bytes of random @@ -589,44 +651,68 @@ static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsig unsigned kp; for (kp = 0; kp < id->keypair_count && !rbuf.wrap; ++kp) { unsigned ktype = id->keypairs[kp]->type; - if (ktype == 0x00 || ktype >= NELS(keytypes)) { - WHYF("illegal key type 0x%02x at kp=%u", ktype, kp); - goto scram; - } + const char *kts = keytype_str(ktype); + int (*packer)(const keypair *, struct rotbuf *) = NULL; + size_t keypair_len; const struct keytype *kt = &keytypes[ktype]; - if (kt->packer == NULL) { - WARNF("unsupported key type 0x%02x, omitted from keyring file", ktype); - continue; + if (ktype == 0x00) + FATALF("ktype=0 in keypair kp=%u", kp); + if (ktype < NELS(keytypes)) { + packer = kt->packer; + keypair_len = kt->packed_size; + } else { + kts = "unknown"; + packer = pack_private_only; + keypair_len = id->keypairs[kp]->private_key_len; } - if (config.debug.keyring) - DEBUGF("pack key type = 0x%02x", ktype); - // First byte is the key type code. - rotbuf_putc(&rbuf, ktype); - // The next two bytes are the key pair length, for forward compatibility: so older software can - // skip over key pairs with an unrecognised type. The original four first key types do not - // store the length, for the sake of backward compatibility with legacy keyring files. Their - // entry lengths are hard-coded. - size_t keypair_len = kt->packed_size; - switch (ktype) { - case KEYTYPE_CRYPTOBOX: - case KEYTYPE_CRYPTOSIGN: - case KEYTYPE_RHIZOME: - case KEYTYPE_DID: - break; - default: - rotbuf_putc(&rbuf, (keypair_len >> 8) & 0xff); - rotbuf_putc(&rbuf, keypair_len & 0xff); - break; - } - // The remaining bytes is the key pair in whatever format it uses. - struct rotbuf rbstart = rbuf; - if (kt->packer(kt, id->keypairs[kp], &rbuf) != 0) - break; - // Ensure the correct number of bytes were written. - unsigned packed = rotbuf_delta(&rbstart, &rbuf); - if (packed != keypair_len) { - WHYF("key type 0x%02x packed wrong length (packed %u, expecting %u)", ktype, packed, (int)keypair_len); - goto scram; + if (packer == NULL) { + WARNF("no packer function for key type 0x%02x%s%s%s, omitted from keyring file", + ktype, + kts && *kts ? " (" : "", + kts ? kts : "", + kts && *kts ? ")" : "" + ); + } else { + if (config.debug.keyring) + DEBUGF("pack key type = 0x%02x%s%s%s", + ktype, + kts && *kts ? " (" : "", + kts ? kts : "", + kts && *kts ? ")" : "" + ); + // First byte is the key type code. + rotbuf_putc(&rbuf, ktype); + // The next two bytes are the key pair length, for forward compatibility: so older software can + // skip over key pairs with an unrecognised type. The original four first key types do not + // store the length, for the sake of backward compatibility with legacy keyring files. Their + // entry lengths are hard-coded. + switch (ktype) { + case KEYTYPE_CRYPTOBOX: + case KEYTYPE_CRYPTOSIGN: + case KEYTYPE_RHIZOME: + case KEYTYPE_DID: + break; + default: + rotbuf_putc(&rbuf, (keypair_len >> 8) & 0xff); + rotbuf_putc(&rbuf, keypair_len & 0xff); + break; + } + // The remaining bytes is the key pair in whatever format it uses. + struct rotbuf rbstart = rbuf; + if (packer(id->keypairs[kp], &rbuf) != 0) + break; + // Ensure the correct number of bytes were written. + unsigned packed = rotbuf_delta(&rbstart, &rbuf); + if (packed != keypair_len) { + WHYF("key type 0x%02x%s%s%s packed wrong length (packed %u, expecting %u)", + ktype, + kts && *kts ? " (" : "", + kts ? kts : "", + kts && *kts ? ")" : "", + packed, (int)keypair_len + ); + goto scram; + } } } // Final byte is a zero key type code. @@ -681,8 +767,8 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char { /* Skip salt and MAC */ keyring_identity *id = emalloc_zero(sizeof(keyring_identity)); - if (!id) { WHY("malloc of identity failed"); return NULL; } - if (!slot) { WHY("slot is null"); return NULL; } + if (!id) + return NULL; id->PKRPin = str_edup(pin); // The two bytes immediately following the MAC describe the rotation offset. uint16_t rotation = (slot[PKR_SALT_BYTES + PKR_MAC_BYTES] << 8) | slot[PKR_SALT_BYTES + PKR_MAC_BYTES + 1]; @@ -720,14 +806,17 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char } if (rbuf.wrap) break; + // Create keyring entry to hold the key pair. Even entries of unknown type are stored, + // so they can be dumped. + keypair *kp = keyring_alloc_keypair(ktype, keypair_len); + if (kp == NULL) + return NULL; + struct rotbuf rbstart = rbuf; if (ktype < NELS(keytypes) && kt->unpacker) { if (config.debug.keyring) DEBUGF("unpack key type = 0x%02x at offset %u", ktype, (int)rotbuf_position(&rbo)); - struct rotbuf rbstart = rbuf; // Create keyring entries to hold the key pair. - keypair *kp = NULL; - if ( (kp = id->keypairs[id->keypair_count] = emalloc_zero(sizeof(keypair))) == NULL - || (kt->private_key_size && (kp->private_key = emalloc(kt->private_key_size)) == NULL) + if ( (kt->private_key_size && (kp->private_key = emalloc(kt->private_key_size)) == NULL) || (kt->public_key_size && (kp->public_key = emalloc(kt->public_key_size)) == NULL) ) { keyring_free_identity(id); @@ -743,37 +832,37 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char keyring_free_identity(id); return NULL; } - // Ensure that the correct number of bytes was consumed. - size_t unpacked = rotbuf_delta(&rbstart, &rbuf); - if (unpacked != keypair_len) { - // If the number of bytes unpacked does not match the keypair length, it is probably an - // empty slot. - if (config.debug.keyring) - DEBUGF("key type 0x%02x unpacked wrong length (unpacked %u, expecting %u)", ktype, (int)unpacked, (int)keypair_len); - keyring_free_identity(id); - return NULL; - } - // Got a valid key pair! Sort the key pairs by (key type, public key, private key) and weed - // out duplicates. - { - int c = 1; - unsigned i = 0; - for (i = 0; i < id->keypair_count && (c = cmp_keypair(id->keypairs[i], id->keypairs[id->keypair_count])) < 0; ++i) - ; - if (c > 0) { - keypair *tmp = id->keypairs[id->keypair_count]; - unsigned j; - for (j = id->keypair_count; j > i; --j) - id->keypairs[j] = id->keypairs[j - 1]; - id->keypairs[i] = tmp; - } - if (c) - ++id->keypair_count; - } } else { if (config.debug.keyring) - DEBUGF("unsupported key type 0x%02x at offset %u, skipping %u bytes", ktype, (int)rotbuf_position(&rbo), (int)keypair_len); - rotbuf_advance(&rbuf, keypair_len); // skip + DEBUGF("unsupported key type 0x%02x at offset %u, reading %u bytes as private key", ktype, (int)rotbuf_position(&rbo), (int)keypair_len); + rotbuf_getbuf(&rbuf, kp->private_key, kp->private_key_len); + } + // Ensure that the correct number of bytes was consumed. + size_t unpacked = rotbuf_delta(&rbstart, &rbuf); + if (unpacked != keypair_len) { + // If the number of bytes unpacked does not match the keypair length, it is probably an + // empty slot. + if (config.debug.keyring) + DEBUGF("key type 0x%02x unpacked wrong length (unpacked %u, expecting %u)", ktype, (int)unpacked, (int)keypair_len); + keyring_free_identity(id); + return NULL; + } + // Got a valid key pair! Sort the key pairs by (key type, public key, private key) and weed + // out duplicates. + { + int c = 1; + unsigned i = 0; + for (i = 0; i < id->keypair_count && (c = cmp_keypair(id->keypairs[i], id->keypairs[id->keypair_count])) < 0; ++i) + ; + if (c > 0) { + keypair *tmp = id->keypairs[id->keypair_count]; + unsigned j; + for (j = id->keypair_count; j > i; --j) + id->keypairs[j] = id->keypairs[j - 1]; + id->keypairs[i] = tmp; + } + if (c) + ++id->keypair_count; } } // If the buffer offset overshot, we got an invalid keypair code and length combination. @@ -814,14 +903,12 @@ int keyring_identity_mac(keyring_context *c, keyring_identity *id, } -/* Read the slot, and try to decrypt it. - Decryption is symmetric with encryption, so the same function is used - for munging the slot before making use of it, whichever way we are going. - Once munged, we then need to verify that the slot is valid, and if so - unpack the details of the identity. -*/ -int keyring_decrypt_pkr(keyring_file *k,keyring_context *c, - const char *pin,int slot_number) +/* Read the slot, and try to decrypt it. Decryption is symmetric with encryption, so the same + * function is used for munging the slot before making use of it, whichever way we are going. Once + * munged, we then need to verify that the slot is valid, and if so unpack the details of the + * identity. + */ +int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, int slot_number) { int exit_code=1; unsigned char slot[KEYRING_PAGE_SIZE]; @@ -909,39 +996,33 @@ int keyring_enter_pin(keyring_file *k, const char *pin) int slot; int identitiesFound=0; - for(slot=0;slotfile_size/KEYRING_PAGE_SIZE;slot++) - { - /* slot zero is the BAM and salt, so skip it */ - if (slot&(KEYRING_BAM_BITS-1)) { - /* Not a BAM slot, so examine */ - off_t file_offset=slot*KEYRING_PAGE_SIZE; + for(slot=0;slotfile_size/KEYRING_PAGE_SIZE;slot++) { + /* slot zero is the BAM and salt, so skip it */ + if (slot&(KEYRING_BAM_BITS-1)) { + /* Not a BAM slot, so examine */ + off_t file_offset=slot*KEYRING_PAGE_SIZE; - /* See if this part of the keyring file is organised */ - keyring_bam *b=k->bam; - while (b&&(file_offset>=b->file_offset+KEYRING_SLAB_SIZE)) - b=b->next; - if (!b) continue; + /* See if this part of the keyring file is organised */ + keyring_bam *b=k->bam; + while (b&&(file_offset>=b->file_offset+KEYRING_SLAB_SIZE)) + b=b->next; + if (!b) continue; - /* Now see if slot is marked in-use. No point checking unallocated slots, - especially since the cost can be upto a second of CPU time on a phone. */ - int position=slot&(KEYRING_BAM_BITS-1); - int byte=position>>3; - int bit=position&7; - if (b->bitmap[byte]&(1<context_count;c++) - { - int result=keyring_decrypt_pkr(k,k->contexts[c],pin?pin:"",slot); - if (!result) - identitiesFound++; - - } - } + /* Now see if slot is marked in-use. No point checking unallocated slots, + especially since the cost can be upto a second of CPU time on a phone. */ + int position=slot&(KEYRING_BAM_BITS-1); + int byte=position>>3; + int bit=position&7; + if (b->bitmap[byte]&(1<context_count;c++) + if (keyring_decrypt_pkr(k,k->contexts[c],pin?pin:"",slot) == 0) + ++identitiesFound; } } - + } /* Tell the caller how many identities we found */ RETURN(identitiesFound); OUT(); @@ -969,13 +1050,10 @@ keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, co keyring_identity *id = emalloc_zero(sizeof(keyring_identity)); if (!id) return NULL; - - /* Store pin */ - id->PKRPin = str_edup(pin); - if (!id->PKRPin) { - WHY("Could not store pin"); + + /* Remember pin */ + if (!(id->PKRPin = str_edup(pin))) goto kci_safeexit; - } /* Find free slot in keyring. Slot 0 in any slab is the BAM and possible keyring salt, so only search for @@ -1174,29 +1252,16 @@ int keyring_set_did(keyring_identity *id, const char *did, const char *name) for(i=0;ikeypair_count;i++) if (id->keypairs[i]->type==KEYTYPE_DID) { if (config.debug.keyring) - DEBUG("Identity contains DID"); + DEBUG("Identity already contains DID"); break; } - - if (i>=PKR_MAX_KEYPAIRS) return WHY("Too many key pairs"); - + if (i >= PKR_MAX_KEYPAIRS) + return WHY("Too many key pairs"); /* allocate if needed */ - if (i>=id->keypair_count) { - id->keypairs[i] = emalloc_zero(sizeof(keypair)); - if (!id->keypairs[i]) + if (i >= id->keypair_count) { + if ((id->keypairs[i] = keyring_alloc_keypair(KEYTYPE_DID, keytypes[KEYTYPE_DID].packed_size)) == NULL) return -1; - id->keypairs[i]->type=KEYTYPE_DID; - unsigned char *packedDid = emalloc_zero(32); - if (!packedDid) - return -1; - unsigned char *packedName = emalloc_zero(64); - if (!packedName) - return -1; - id->keypairs[i]->private_key=packedDid; - id->keypairs[i]->private_key_len=32; - id->keypairs[i]->public_key=packedName; - id->keypairs[i]->public_key_len=64; - id->keypair_count++; + ++id->keypair_count; if (config.debug.keyring) DEBUG("Created DID record for identity"); } @@ -1671,13 +1736,13 @@ int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) const keyring_identity *id = idx[i]; for (kp = 0; kp < id->keypair_count; ++kp) { keypair *keyp = id->keypairs[kp]; - xprintf(xpf, "%u: type=%u", i, keyp->type); - const char *kts = keytype_str(keyp->type); - if (kts && kts[0]) - xprintf(xpf, "(%s)", kts); assert(keyp->type != 0); assert(keyp->type < NELS(keytypes)); - xprintf(xpf, " "); + xprintf(xpf, "%u: type=%u", i, keyp->type); + const char *kts = keytype_str(keyp->type); + if (!kts || !kts[0]) + kts = "unknown"; + xprintf(xpf, "(%s) ", kts); if (keytypes[keyp->type].dumper) keytypes[keyp->type].dumper(keyp, xpf, include_secret); else @@ -1687,3 +1752,27 @@ int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) } return 0; } + +int keyring_load(keyring_file *k, unsigned pinc, const char **pinv, FILE *input) +{ + clearerr(input); + while (1) { + unsigned id; + unsigned ktype; + int i = 0; + char content[1024]; + content[0] = '\0'; + int n = fscanf(input, "%u: type=%u(%*[^)]) %n", &id, &ktype, &i); + fgets(content, sizeof content - 1, input); + DEBUGF("n=%d i=%d content=%s", n, i, alloca_str_toprint(content)); + if (n == EOF && (ferror(input) || feof(input))) + break; + if (n != 2) + return WHY("malformed input"); + if (ktype == 0) + return WHY("invalid input: key type = 0"); + } + if (ferror(input)) + return WHYF_perror("fscanf"); + return 0; +} diff --git a/serval.h b/serval.h index a970775b..d9afa4db 100644 --- a/serval.h +++ b/serval.h @@ -253,7 +253,6 @@ typedef struct keyring_file { void keyring_free(keyring_file *k); void keyring_free_context(keyring_context *c); void keyring_free_identity(keyring_identity *id); -void keyring_free_keypair(keypair *kp); int keyring_identity_mac(keyring_context *c,keyring_identity *id, unsigned char *pkrsalt,unsigned char *mac); #define KEYTYPE_CRYPTOBOX 0x01 @@ -286,6 +285,7 @@ int keyring_commit(keyring_file *k); keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, const char *pin); int keyring_seed(keyring_file *k); void keyring_identity_extract(const keyring_identity *id, const unsigned char **sidp, const char **didp, const char **namep); +int keyring_load(keyring_file *k, unsigned pinc, const char **pinv, FILE *input); int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret); /* Make sure we have space to put bytes of the packet as we go along */ From 9d1c3e0cbacf1f07027e1ed3bfac3d269db702e4 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Tue, 3 Sep 2013 17:05:50 +0930 Subject: [PATCH 03/11] Fix test case doc comment --- tests/keyring | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/keyring b/tests/keyring index 6bee4bff..09032faf 100755 --- a/tests/keyring +++ b/tests/keyring @@ -196,7 +196,7 @@ test_KeyringPathEnv() { assert [ "$(replayStdout)" == "$orig" ] } -doc_ReadOnlyEnv="Use keyring specified by environment variable" +doc_ReadOnlyEnv="Read-only keyring environment variable" setup_ReadOnlyEnv() { setup executeOk_servald keyring add '' From 34bbfb9b3004ab501f355b940e2865dc3c607e16 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Tue, 3 Sep 2013 17:29:41 +0930 Subject: [PATCH 04/11] Add strn_fromhex(), improve hexvalue() Implement fromhex() and fromhexstr() using strn_fromhex() --- str.c | 71 ++++++++++++++++++++++++++++++++++++++++++++--------------- str.h | 27 ++++++++++++++++++----- 2 files changed, 75 insertions(+), 23 deletions(-) diff --git a/str.c b/str.c index bc95951f..a6c79dc7 100644 --- a/str.c +++ b/str.c @@ -43,32 +43,67 @@ char *tohex(char *dstHex, const unsigned char *srcBinary, size_t bytes) } /* Convert nbinary*2 ASCII hex characters [0-9A-Fa-f] to nbinary bytes of data. Can be used to - perform the conversion in-place, eg, fromhex(buf, (char*)buf, n); Returns -1 if a non-hex-digit - character is encountered, otherwise returns the number of binary bytes produced (= nbinary). - @author Andrew Bettison + * perform the conversion in-place, eg, fromhex(buf, (char*)buf, n); Returns -1 if a non-hex-digit + * character is encountered, otherwise returns the number of binary bytes produced (= nbinary). + * + * @author Andrew Bettison */ size_t fromhex(unsigned char *dstBinary, const char *srcHex, size_t nbinary) { - size_t count = 0; - while (count != nbinary) { - unsigned char high = hexvalue(*srcHex++); - if (high & 0xf0) return -1; - unsigned char low = hexvalue(*srcHex++); - if (low & 0xf0) return -1; - dstBinary[count++] = (high << 4) + low; - } - return count; + if (strn_fromhex(dstBinary, nbinary, srcHex, NULL) == nbinary) + return nbinary; + return -1; } -/* Convert nbinary*2 ASCII hex characters [0-9A-Fa-f] followed by a nul '\0' character to nbinary bytes of data. Can be used to - perform the conversion in-place, eg, fromhex(buf, (char*)buf, n); Returns -1 if a non-hex-digit - character is encountered or the character immediately following the last hex digit is not a nul, - otherwise returns zero. - @author Andrew Bettison +/* Convert nbinary*2 ASCII hex characters [0-9A-Fa-f] followed by a nul '\0' character to nbinary + * bytes of data. Can be used to perform the conversion in-place, eg, fromhex(buf, (char*)buf, n); + * Returns -1 if a non-hex-digit character is encountered or the character immediately following the + * last hex digit is not a nul, otherwise returns zero. + * + * @author Andrew Bettison */ int fromhexstr(unsigned char *dstBinary, const char *srcHex, size_t nbinary) { - return (fromhex(dstBinary, srcHex, nbinary) == nbinary && srcHex[nbinary * 2] == '\0') ? 0 : -1; + const char *p; + if (strn_fromhex(dstBinary, nbinary, srcHex, &p) == nbinary && *p == '\0') + return 0; + return -1; +} + +/* Decode pairs of ASCII hex characters [0-9A-Fa-f] into binary data with an optional upper limit on + * the number of binary bytes produced (destination buffer size). Returns the number of binary + * bytes decoded. If 'afterHex' is not NULL, then sets *afterHex to point to the source character + * immediately following the last hex digit consumed. + * + * Can be used to perform a conversion in-place, eg: + * + * strn_fromhex((unsigned char *)buf, n, (const char *)buf, NULL); + * + * Can also be used to count hex digits without converting, eg: + * + * strn_fromhex(NULL, -1, buf, NULL); + * + * @author Andrew Bettison + */ +size_t strn_fromhex(unsigned char *dstBinary, ssize_t dstlen, const char *srcHex, const char **afterHex) +{ + unsigned char *dstorig = dstBinary; + unsigned char *dstend = dstBinary + dstlen; + while (dstlen == -1 || dstBinary < dstend) { + int high = hexvalue(srcHex[0]); + if (high == -1) + break; + int low = hexvalue(srcHex[1]); + if (low == -1) + break; + if (dstorig != NULL) + *dstBinary = (high << 4) + low; + ++dstBinary; + srcHex += 2; + } + if (afterHex) + *afterHex = srcHex; + return dstBinary - dstorig; } /* Does this whole buffer contain the same value? */ diff --git a/str.h b/str.h index 3a5d0bc3..ae50a729 100644 --- a/str.h +++ b/str.h @@ -64,19 +64,36 @@ extern const char hexdigit[16]; char *tohex(char *dstHex, const unsigned char *srcBinary, size_t bytes); size_t fromhex(unsigned char *dstBinary, const char *srcHex, size_t nbinary); int fromhexstr(unsigned char *dstBinary, const char *srcHex, size_t nbinary); -int is_all_matching(const unsigned char *ptr, size_t len, unsigned char value); -char *str_toupper_inplace(char *s); +size_t strn_fromhex(unsigned char *dstBinary, ssize_t dstlen, const char *src, const char **afterp); #define alloca_tohex(buf,len) tohex((char *)alloca((len)*2+1), (buf), (len)) __STR_INLINE int hexvalue(char c) { - if (c >= '0' && c <= '9') return c - '0'; - if (c >= 'A' && c <= 'F') return c - 'A' + 10; - if (c >= 'a' && c <= 'f') return c - 'a' + 10; + switch (c) { + case '0': return 0; + case '1': return 1; + case '2': return 2; + case '3': return 3; + case '4': return 4; + case '5': return 5; + case '6': return 6; + case '7': return 7; + case '8': return 8; + case '9': return 9; + case 'a': case 'A': return 10; + case 'b': case 'B': return 11; + case 'c': case 'C': return 12; + case 'd': case 'D': return 13; + case 'e': case 'E': return 14; + case 'f': case 'F': return 15; + } return -1; } +int is_all_matching(const unsigned char *ptr, size_t len, unsigned char value); +char *str_toupper_inplace(char *s); + char *toprint(char *dstStr, ssize_t dstBufSiz, const char *srcBuf, size_t srcBytes, const char quotes[2]); char *toprint_str(char *dstStr, ssize_t dstBufSiz, const char *srcStr, const char quotes[2]); size_t toprint_len(const char *srcBuf, size_t srcBytes, const char quotes[2]); From 08c02e0e20e47f8431cbc49c9e0b1a5bb89e009f Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Tue, 3 Sep 2013 17:31:10 +0930 Subject: [PATCH 05/11] More progress towards keyring load command --- commandline.c | 2 +- keyring.c | 391 +++++++++++++++++++++++++++++--------------------- serval.h | 2 +- 3 files changed, 229 insertions(+), 166 deletions(-) diff --git a/commandline.c b/commandline.c index 58db5f14..0498ebf0 100644 --- a/commandline.c +++ b/commandline.c @@ -1793,7 +1793,7 @@ int app_keyring_load(const struct cli_parsed *parsed, struct cli_context *contex FILE *fp = path && strcmp(path, "-") != 0 ? fopen(path, "r") : stdin; if (fp == NULL) return WHYF_perror("fopen(%s, \"r\")", alloca_str_toprint(path)); - int ret = keyring_load(k, pinc, pinv, fp); + int ret = keyring_load(k, 0, pinc, pinv, fp); return ret; } diff --git a/keyring.c b/keyring.c index 6b868b54..4264ca49 100644 --- a/keyring.c +++ b/keyring.c @@ -184,8 +184,8 @@ void keyring_free(keyring_file *k) /* Free BAMs (no substructure, so easy) */ keyring_bam *b=k->bam; - while(b) { - keyring_bam *last_bam=b; + while(b) { + keyring_bam *last_bam=b; b=b->next; /* Clear out any private data */ bzero(last_bam,sizeof(keyring_bam)); @@ -304,20 +304,6 @@ int keyring_enter_keyringpin(keyring_file *k, const char *pin) return 0; } -/* Enter an identity pin and search for matching records. - This involves going through the bitmap for each slab, and - then trying each keyring pin and identity pin with each - record marked as allocated. - We might find more than one matching identity, and that's okay; - we just load them all. -*/ -int keyring_enter_identitypin(keyring_file *k,char *pin) -{ - if (!k) return WHY("k is null"); - - return WHY("Not implemented"); -} - /* En/Decrypting a block requires use of the first 32 bytes of the block to provide salt. The next 64 bytes constitute a message authentication code (MAC) that is @@ -377,7 +363,7 @@ int keyring_munge_block(unsigned char *block,int len /* includes the first 96 by APPEND(PKRPin,strlen(PKRPin)); crypto_hash_sha512(hashNonce,work,ofs); - /* Now en/de-crypt the remainder of the block. + /* Now en/de-crypt the remainder of the block. We do this in-place for convenience, so you should not pass in a mmap()'d lump. */ crypto_stream_xsalsa20_xor(&block[96],&block[96],len-96, hashNonce,hashKey); @@ -393,14 +379,14 @@ int keyring_munge_block(unsigned char *block,int len /* includes the first 96 by #undef APPEND } -static const char *keytype_str(unsigned ktype) +static const char *keytype_str(unsigned ktype, const char *unknown) { switch (ktype) { case KEYTYPE_CRYPTOBOX: return "CRYPTOBOX"; case KEYTYPE_CRYPTOSIGN: return "CRYPTOSIGN"; case KEYTYPE_RHIZOME: return "RHIZOME"; case KEYTYPE_DID: return "DID"; - default: return ""; + default: return unknown; } } @@ -409,9 +395,9 @@ struct keytype { size_t private_key_size; size_t packed_size; int (*packer)(const keypair *, struct rotbuf *); - int (*unpacker)(const struct keytype *, keypair *, struct rotbuf *); + int (*unpacker)(keypair *, struct rotbuf *); void (*dumper)(const keypair *, XPRINTF, int); - int (*loader)(const keypair *, const char *); + int (*loader)(keypair *, const char *); }; static int pack_private_only(const keypair *kp, struct rotbuf *rb) @@ -435,27 +421,62 @@ static void dump_raw_hex(const keypair *kp, XPRINTF xpf, int include_secret) xprintf(xpf, " sec=%s", alloca_tohex(kp->private_key, kp->private_key_len)); } -static int load_raw_hex(const keypair *kp, const char *text) +static int load_raw_hex(keypair *kp, const char *text) { + const char *t = text; + while (*t) { + while (isspace(*t)) + ++t; + if (str_startswith(t, "pub=", &t)) { + const char *e = NULL; + size_t len = strn_fromhex(NULL, -1, t, &e); + if (len == 0 || e == NULL || (*e != ' ' && *e != '\0')) + return WHY("malformed pub= hex value"); + if (kp->public_key_len == 0) { + assert(kp->public_key == NULL); + kp->public_key_len = len; + if ((kp->public_key = emalloc_zero(len)) == NULL) + return -1; + } else if (len != kp->public_key_len) + return WHY("invalid pub= hex value, incorrect length"); + strn_fromhex(kp->public_key, kp->public_key_len, t, &t); + } + else if (str_startswith(t, "sec=", &t)) { + const char *e = NULL; + size_t len = strn_fromhex(NULL, -1, t, &e); + if (len == 0 || e == NULL || (*e != ' ' && *e != '\0')) + return WHY("malformed sec= hex value"); + if (kp->private_key_len == 0) { + assert(kp->private_key == NULL); + kp->private_key_len = len; + if ((kp->private_key = emalloc_zero(len)) == NULL) + return -1; + } else if (len != kp->private_key_len) + return WHY("invalid sec= hex value, incorrect length"); + strn_fromhex(kp->private_key, kp->private_key_len, t, &t); + } + else if (*t) + return WHYF("unsupported dump content: %s", t); + } return 0; } -static int unpack_private_public(const struct keytype *kt, keypair *kp, struct rotbuf *rb) +static int unpack_private_public(keypair *kp, struct rotbuf *rb) { - rotbuf_getbuf(rb, kp->private_key, kt->private_key_size); - rotbuf_getbuf(rb, kp->public_key, kt->public_key_size); + rotbuf_getbuf(rb, kp->private_key, kp->private_key_len); + rotbuf_getbuf(rb, kp->public_key, kp->public_key_len); return 0; } -static int unpack_private_only(const struct keytype *kt, keypair *kp, struct rotbuf *rb) +static int unpack_private_only(keypair *kp, struct rotbuf *rb) { - rotbuf_getbuf(rb, kp->private_key, kt->private_key_size); + rotbuf_getbuf(rb, kp->private_key, kp->private_key_len); return 0; } -static int unpack_private_derive_scalarmult_public(const struct keytype *kt, keypair *kp, struct rotbuf *rb) +static int unpack_private_derive_scalarmult_public(keypair *kp, struct rotbuf *rb) { - rotbuf_getbuf(rb, kp->private_key, kt->private_key_size); + rotbuf_getbuf(rb, kp->private_key, kp->private_key_len); /* Compute public key from private key. * * Public key calculation as below is taken from section 3 of: @@ -481,12 +502,12 @@ static int pack_did_name(const keypair *kp, struct rotbuf *rb) return pack_private_public(kp, rb); } -static int unpack_did_name(const struct keytype *kt, keypair *kp, struct rotbuf *rb) +static int unpack_did_name(keypair *kp, struct rotbuf *rb) { - if (unpack_private_public(kt, kp, rb) == -1) + if (unpack_private_public(kp, rb) == -1) return -1; // Fail if name is not nul terminated. - return strnchr((const char *)kp->public_key, kt->public_key_size, '\0') == NULL ? -1 : 0; + return strnchr((const char *)kp->public_key, kp->public_key_len, '\0') == NULL ? -1 : 0; } static void dump_did_name(const keypair *kp, XPRINTF xpf, int include_secret) @@ -495,22 +516,44 @@ static void dump_did_name(const keypair *kp, XPRINTF xpf, int include_secret) xprintf(xpf, " Name=%s", alloca_str_toprint_quoted((const char *)kp->public_key, "\"\"")); } -static int load_did_name(const keypair *kp, const char *text) +static int load_did_name(keypair *kp, const char *text) { - const char *s; - const char *t; - if ((s = strstr(text, " DID=\"")) == NULL) + assert(kp->public_key != NULL); + assert(kp->private_key != NULL); + const char *t = text; + int got_did = 0; + int got_name = 0; + while (*t) { + while (isspace(*t)) + ++t; + if (str_startswith(t, "DID=\"", &t)) { + if (got_did) + return WHY("duplicate DID"); + const char *e = NULL; + bzero(kp->private_key, kp->private_key_len); + strn_fromprint(kp->private_key, kp->private_key_len, t, '"', &e); + if (*e != '"') + return WHY("malformed DID quoted string"); + t = e + 1; + got_did = 1; + } else if (str_startswith(t, "Name=\"", &t)) { + if (got_name) + return WHY("duplicate Name"); + const char *e = NULL; + bzero(kp->public_key, kp->public_key_len); + strn_fromprint(kp->public_key, kp->public_key_len, t, '"', &e); + if (*e != '"') + return WHY("malformed Name quoted string"); + t = e + 1; + got_name = 1; + } + else if (*t) + return WHYF("unsupported dump content: %s", t); + } + if (!got_did) return WHY("missing DID"); - bzero(kp->private_key, kp->private_key_len); - strn_fromprint(kp->private_key, kp->private_key_len, s + 6, '"', &t); - if (*t != '"') - return WHY("malformed DID quoted string"); - if ((s = strstr(text, " Name=\"")) == NULL) + if (!got_name) return WHY("missing Name"); - bzero(kp->public_key, kp->public_key_len); - strn_fromprint(kp->public_key, kp->public_key_len, s + 6, '"', &t); - if (*t != '"') - return WHY("malformed Name quoted string"); return 0; } @@ -597,16 +640,6 @@ static keypair *keyring_alloc_keypair(unsigned ktype, size_t len) if (ktype < NELS(keytypes)) { kp->private_key_len = keytypes[ktype].private_key_size; kp->public_key_len = keytypes[ktype].public_key_size; - if (kp->private_key_len + kp->public_key_len != len) { - WHYF("keytype=%u private_key_len(%u) + public_key_len(%u) != len(%u)", - ktype, - kp->private_key_len, - kp->public_key_len, - len - ); - keyring_free_keypair(kp); - return NULL; - } } else { kp->private_key_len = len; kp->public_key_len = 0; @@ -651,7 +684,7 @@ static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsig unsigned kp; for (kp = 0; kp < id->keypair_count && !rbuf.wrap; ++kp) { unsigned ktype = id->keypairs[kp]->type; - const char *kts = keytype_str(ktype); + const char *kts = keytype_str(ktype, "unknown"); int (*packer)(const keypair *, struct rotbuf *) = NULL; size_t keypair_len; const struct keytype *kt = &keytypes[ktype]; @@ -661,25 +694,14 @@ static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsig packer = kt->packer; keypair_len = kt->packed_size; } else { - kts = "unknown"; packer = pack_private_only; keypair_len = id->keypairs[kp]->private_key_len; } if (packer == NULL) { - WARNF("no packer function for key type 0x%02x%s%s%s, omitted from keyring file", - ktype, - kts && *kts ? " (" : "", - kts ? kts : "", - kts && *kts ? ")" : "" - ); + WARNF("no packer function for key type 0x%02x(%s), omitted from keyring file", ktype, kts); } else { if (config.debug.keyring) - DEBUGF("pack key type = 0x%02x%s%s%s", - ktype, - kts && *kts ? " (" : "", - kts ? kts : "", - kts && *kts ? ")" : "" - ); + DEBUGF("pack key type = 0x%02x(%s)", ktype, kts); // First byte is the key type code. rotbuf_putc(&rbuf, ktype); // The next two bytes are the key pair length, for forward compatibility: so older software can @@ -704,13 +726,7 @@ static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsig // Ensure the correct number of bytes were written. unsigned packed = rotbuf_delta(&rbstart, &rbuf); if (packed != keypair_len) { - WHYF("key type 0x%02x%s%s%s packed wrong length (packed %u, expecting %u)", - ktype, - kts && *kts ? " (" : "", - kts ? kts : "", - kts && *kts ? ")" : "", - packed, (int)keypair_len - ); + WHYF("key type 0x%02x(%s) packed wrong length (packed %u, expecting %u)", ktype, kts, packed, (int)keypair_len); goto scram; } } @@ -763,6 +779,23 @@ static int cmp_keypair(const keypair *a, const keypair *b) return c; } +static int keyring_identity_add_keypair(keyring_identity *id, keypair *kp) +{ + assert(id->keypair_count < PKR_MAX_KEYPAIRS); + assert(kp != NULL); + int c = 1; + unsigned i = 0; + for (i = 0; i < id->keypair_count && (c = cmp_keypair(id->keypairs[i], kp)) < 0; ++i) + ; + if (c == 0) + return 0; // duplicate not inserted + unsigned j; + for (j = id->keypair_count++; j > i; --j) + id->keypairs[j] = id->keypairs[j - 1]; + id->keypairs[i] = kp; + return 1; +} + static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin) { /* Skip salt and MAC */ @@ -804,66 +837,53 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char keypair_len |= rotbuf_getc(&rbuf); break; } - if (rbuf.wrap) - break; + if (keypair_len > rotbuf_remain(&rbuf)) { + if (config.debug.keyring) + DEBUGF("invalid keypair length %u", keypair_len); + keyring_free_identity(id); + return NULL; + } // Create keyring entry to hold the key pair. Even entries of unknown type are stored, // so they can be dumped. keypair *kp = keyring_alloc_keypair(ktype, keypair_len); - if (kp == NULL) + if (kp == NULL) { + keyring_free_identity(id); return NULL; + } struct rotbuf rbstart = rbuf; if (ktype < NELS(keytypes) && kt->unpacker) { if (config.debug.keyring) - DEBUGF("unpack key type = 0x%02x at offset %u", ktype, (int)rotbuf_position(&rbo)); - // Create keyring entries to hold the key pair. - if ( (kt->private_key_size && (kp->private_key = emalloc(kt->private_key_size)) == NULL) - || (kt->public_key_size && (kp->public_key = emalloc(kt->public_key_size)) == NULL) - ) { - keyring_free_identity(id); - return NULL; - } - kp->type = ktype; - kp->private_key_len = kt->private_key_size; - kp->public_key_len = kt->public_key_size; - if (kt->unpacker(kt, kp, &rbuf) != 0) { + DEBUGF("unpack key type = 0x%02x(%s) at offset %u", ktype, keytype_str(ktype, "unknown"), (int)rotbuf_position(&rbo)); + if (kt->unpacker(kp, &rbuf) != 0) { // If there is an error, it is probably an empty slot. if (config.debug.keyring) DEBUGF("key type 0x%02x does not unpack", ktype); + keyring_free_keypair(kp); + keyring_free_identity(id); + return NULL; + } + // Ensure that the correct number of bytes was consumed. + size_t unpacked = rotbuf_delta(&rbstart, &rbuf); + if (unpacked != keypair_len) { + // If the number of bytes unpacked does not match the keypair length, it is probably an + // empty slot. + if (config.debug.keyring) + DEBUGF("key type 0x%02x unpacked wrong length (unpacked %u, expecting %u)", ktype, (int)unpacked, (int)keypair_len); + keyring_free_keypair(kp); keyring_free_identity(id); return NULL; } } else { if (config.debug.keyring) - DEBUGF("unsupported key type 0x%02x at offset %u, reading %u bytes as private key", ktype, (int)rotbuf_position(&rbo), (int)keypair_len); + DEBUGF("unsupported key type 0x%02x at offset %u, reading %u bytes as private key", ktype, (unsigned)rotbuf_position(&rbo), (unsigned)kp->private_key_len); + assert(kp->public_key_len == 0); + assert(kp->public_key == NULL); rotbuf_getbuf(&rbuf, kp->private_key, kp->private_key_len); } - // Ensure that the correct number of bytes was consumed. - size_t unpacked = rotbuf_delta(&rbstart, &rbuf); - if (unpacked != keypair_len) { - // If the number of bytes unpacked does not match the keypair length, it is probably an - // empty slot. - if (config.debug.keyring) - DEBUGF("key type 0x%02x unpacked wrong length (unpacked %u, expecting %u)", ktype, (int)unpacked, (int)keypair_len); - keyring_free_identity(id); - return NULL; - } // Got a valid key pair! Sort the key pairs by (key type, public key, private key) and weed // out duplicates. - { - int c = 1; - unsigned i = 0; - for (i = 0; i < id->keypair_count && (c = cmp_keypair(id->keypairs[i], id->keypairs[id->keypair_count])) < 0; ++i) - ; - if (c > 0) { - keypair *tmp = id->keypairs[id->keypair_count]; - unsigned j; - for (j = id->keypair_count; j > i; --j) - id->keypairs[j] = id->keypairs[j - 1]; - id->keypairs[i] = tmp; - } - if (c) - ++id->keypair_count; - } + if (!keyring_identity_add_keypair(id, kp)) + keyring_free_keypair(kp); } // If the buffer offset overshot, we got an invalid keypair code and length combination. if (rbuf.wrap > 1) { @@ -942,13 +962,12 @@ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, in goto kdp_safeexit; } /* compare hash to record */ - if (memcmp(hash,&slot[32],crypto_hash_sha512_BYTES)) - { - WHY("Slot is not valid (MAC mismatch)"); - dump("computed",hash,crypto_hash_sha512_BYTES); - dump("stored",&slot[32],crypto_hash_sha512_BYTES); - goto kdp_safeexit; - } + if (memcmp(hash,&slot[32],crypto_hash_sha512_BYTES)) { + WHYF("slot %u is not valid (MAC mismatch)", slot_number); + dump("computed",hash,crypto_hash_sha512_BYTES); + dump("stored",&slot[32],crypto_hash_sha512_BYTES); + goto kdp_safeexit; + } // add any unlocked subscribers to our memory table, flagged as local sid's int i=0; @@ -1028,13 +1047,46 @@ int keyring_enter_pin(keyring_file *k, const char *pin) OUT(); } +static unsigned test_slot(keyring_file *k, unsigned slot) +{ + assert(slot < KEYRING_BAM_BITS); + unsigned position = slot & (KEYRING_BAM_BITS - 1); + unsigned byte = position >> 3; + unsigned bit = position & 7; + return (k->bam->bitmap[byte] & (1 << bit)) ? 1 : 0; +} + +static void set_slot(keyring_file *k, unsigned slot, int bitvalue) +{ + assert(slot < KEYRING_BAM_BITS); + unsigned position = slot & (KEYRING_BAM_BITS - 1); + unsigned byte = position >> 3; + unsigned bit = position & 7; + if (bitvalue) + k->bam->bitmap[byte] |= (1 << bit); + else + k->bam->bitmap[byte] &= ~(1 << bit); +} + +/* Find free slot in keyring. Slot 0 in any slab is the BAM and possible keyring salt, so only + * search for space in slots 1 and above. TODO: Extend to handle more than one slab! + */ +static unsigned find_free_slot(keyring_file *k) +{ + unsigned slot; + for (slot = 1; slot < KEYRING_BAM_BITS; ++slot) + if (!test_slot(k, slot)) + return slot; + return 0; +} + /* Create a new identity in the specified context (which supplies the keyring pin) with the specified PKR pin. The crypto_box and crypto_sign key pairs are automatically created, and the PKR is packed and written to a hithero unallocated slot which is then marked full. Requires an explicit call to keyring_commit() */ -keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, const char *pin) +keyring_identity *keyring_create_identity(keyring_file *k, keyring_context *c, const char *pin) { if (config.debug.keyring) DEBUGF("k=%p", k); @@ -1055,22 +1107,10 @@ keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, co if (!(id->PKRPin = str_edup(pin))) goto kci_safeexit; - /* Find free slot in keyring. - Slot 0 in any slab is the BAM and possible keyring salt, so only search for - space in slots 1 and above. */ - /* XXX Only stores to first slab! */ - keyring_bam *b=k->bam; - for(id->slot=1;id->slotslot++) - { - int position=id->slot&(KEYRING_BAM_BITS-1); - int byte=position>>3; - int bit=position&7; - if (!(b->bitmap[byte]&(1<slot>=KEYRING_BAM_BITS) { - WHY("no free slots in first slab (and I don't know how to store in subsequent slabs yet"); + /* Find free slot in keyring. */ + id->slot = find_free_slot(k); + if (id->slot == 0) { + WHY("no free slots in first slab (no support for more than one slab)"); goto kci_safeexit; } @@ -1146,24 +1186,21 @@ keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, co urandombytes(id->keypairs[2]->private_key,id->keypairs[2]->private_key_len); /* Mark slot in use */ - int position=id->slot&(KEYRING_BAM_BITS-1); - int byte=position>>3; - int bit=position&7; - b->bitmap[byte]|=(1<slot, 1); /* Add identity to data structure */ c->identities[c->identity_count++]=id; // add new identity to in memory table id->subscriber = find_subscriber(id->keypairs[0]->public_key, SID_SIZE, 1); - if (id->subscriber){ + if (id->subscriber) { if (id->subscriber->reachable==REACHABLE_NONE) id->subscriber->reachable=REACHABLE_SELF; id->subscriber->identity = id; if (!my_subscriber) my_subscriber=id->subscriber; } - + /* Everything went fine */ return id; @@ -1718,6 +1755,17 @@ static int cmp_identity_ptrs(const keyring_identity *const *a, const keyring_ide return i == (*a)->keypair_count ? -1 : 1; } +static void keyring_dump_keypair(const keypair *kp, XPRINTF xpf, int include_secret) +{ + assert(kp->type != 0); + assert(kp->type < NELS(keytypes)); + xprintf(xpf, "type=%u(%s) ", kp->type, keytype_str(kp->type, "unknown")); + if (keytypes[kp->type].dumper) + keytypes[kp->type].dumper(kp, xpf, include_secret); + else + dump_raw_hex(kp, xpf, include_secret); +} + int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) { int cn, in, kp; @@ -1736,41 +1784,56 @@ int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) const keyring_identity *id = idx[i]; for (kp = 0; kp < id->keypair_count; ++kp) { keypair *keyp = id->keypairs[kp]; - assert(keyp->type != 0); - assert(keyp->type < NELS(keytypes)); - xprintf(xpf, "%u: type=%u", i, keyp->type); - const char *kts = keytype_str(keyp->type); - if (!kts || !kts[0]) - kts = "unknown"; - xprintf(xpf, "(%s) ", kts); - if (keytypes[keyp->type].dumper) - keytypes[keyp->type].dumper(keyp, xpf, include_secret); - else - dump_raw_hex(keyp, xpf, include_secret); + xprintf(xpf, "%u: ", i); + keyring_dump_keypair(keyp, xpf, include_secret); xprintf(xpf, "\n"); } } return 0; } -int keyring_load(keyring_file *k, unsigned pinc, const char **pinv, FILE *input) +int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE *input) { clearerr(input); while (1) { unsigned id; unsigned ktype; - int i = 0; + char ktypestr[100]; char content[1024]; content[0] = '\0'; - int n = fscanf(input, "%u: type=%u(%*[^)]) %n", &id, &ktype, &i); - fgets(content, sizeof content - 1, input); - DEBUGF("n=%d i=%d content=%s", n, i, alloca_str_toprint(content)); + int n = fscanf(input, "%u: type=%u(%99[^)]) ", &id, &ktype, ktypestr); if (n == EOF && (ferror(input) || feof(input))) break; - if (n != 2) + if (n != 3) return WHY("malformed input"); if (ktype == 0) return WHY("invalid input: key type = 0"); + fgets(content, sizeof content - 1, input); + // Strip trailing \n or CRLF + size_t contentlen = strlen(content); + if (contentlen && content[contentlen - 1] == '\n') { + content[--contentlen] = '\0'; + if (contentlen && content[contentlen - 1] == '\r') + content[--contentlen] = '\0'; + } + //DEBUGF("n=%d content=%s", n, alloca_str_toprint(content)); + keypair *kp = keyring_alloc_keypair(ktype, 0); + if (kp == NULL) + return -1; + int (*loader)(keypair *, const char *) = load_raw_hex; + if (strcmp(ktypestr, "unknown") != 0 && ktype < NELS(keytypes)) + loader = keytypes[ktype].loader; + if (loader(kp, content) == -1) + return -1; + /* + if (!keyring_identity_add_keypair(id, kp)) + keyring_free_keypair(kp); + */ + /* + fprintf(stderr, "%u: ", id); + keyring_dump_keypair(kp, XPRINTF_STDIO(stderr), 1); + fprintf(stderr, "\n"); + */ } if (ferror(input)) return WHYF_perror("fscanf"); diff --git a/serval.h b/serval.h index d9afa4db..f457ddd6 100644 --- a/serval.h +++ b/serval.h @@ -285,7 +285,7 @@ int keyring_commit(keyring_file *k); keyring_identity *keyring_create_identity(keyring_file *k,keyring_context *c, const char *pin); int keyring_seed(keyring_file *k); void keyring_identity_extract(const keyring_identity *id, const unsigned char **sidp, const char **didp, const char **namep); -int keyring_load(keyring_file *k, unsigned pinc, const char **pinv, FILE *input); +int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE *input); int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret); /* Make sure we have space to put bytes of the packet as we go along */ From f95e41374b3532f369a5a90eff607f54c1966851 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Wed, 4 Sep 2013 23:47:17 +0930 Subject: [PATCH 06/11] Implement keyring load command, not working yet Write new keyring test, fails with ERROR because assertGrep -F option is not supported. --- keyring.c | 239 ++++++++++++++++++++++++++------------------------ serval.h | 2 +- tests/keyring | 48 ++++++++-- 3 files changed, 166 insertions(+), 123 deletions(-) diff --git a/keyring.c b/keyring.c index 4264ca49..0f651318 100644 --- a/keyring.c +++ b/keyring.c @@ -173,6 +173,20 @@ keyring_file *keyring_open(const char *path, int writeable) return k; } +static void add_subscriber(keyring_identity *id, unsigned keypair) +{ + assert(keypair < id->keypair_count); + assert(id->keypairs[keypair]->type == KEYTYPE_CRYPTOBOX); + id->subscriber = find_subscriber(id->keypairs[keypair]->public_key, SID_SIZE, 1); + if (id->subscriber) { + if (id->subscriber->reachable == REACHABLE_NONE) + id->subscriber->reachable = REACHABLE_SELF; + id->subscriber->identity = id; + if (!my_subscriber) + my_subscriber = id->subscriber; + } +} + void keyring_free(keyring_file *k) { int i; @@ -394,12 +408,32 @@ struct keytype { size_t public_key_size; size_t private_key_size; size_t packed_size; + void (*creator)(keypair *); int (*packer)(const keypair *, struct rotbuf *); int (*unpacker)(keypair *, struct rotbuf *); void (*dumper)(const keypair *, XPRINTF, int); int (*loader)(keypair *, const char *); }; +static void create_cryptobox(keypair *kp) +{ + /* Filter out public keys that start with 0x0, as they are reserved for address + abbreviation. */ + do { + crypto_box_curve25519xsalsa20poly1305_keypair(kp->public_key, kp->private_key); + } while (kp->public_key[0] < 0x10); +} + +static void create_cryptosign(keypair *kp) +{ + crypto_sign_edwards25519sha512batch_keypair(kp->public_key, kp->private_key); +} + +static void create_rhizome(keypair *kp) +{ + urandombytes(kp->private_key, kp->private_key_len); +} + static int pack_private_only(const keypair *kp, struct rotbuf *rb) { rotbuf_putbuf(rb, kp->private_key, kp->private_key_len); @@ -571,6 +605,7 @@ const struct keytype keytypes[] = { .private_key_size = crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES, .public_key_size = crypto_box_curve25519xsalsa20poly1305_PUBLICKEYBYTES, .packed_size = crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES, + .creator = create_cryptobox, .packer = pack_private_only, .unpacker = unpack_private_derive_scalarmult_public, .dumper = dump_raw_hex, @@ -585,6 +620,7 @@ const struct keytype keytypes[] = { .private_key_size = crypto_sign_edwards25519sha512batch_SECRETKEYBYTES, .public_key_size = crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES, .packed_size = crypto_sign_edwards25519sha512batch_SECRETKEYBYTES + crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES, + .creator = create_cryptosign, .packer = pack_private_public, .unpacker = unpack_private_public, .dumper = dump_raw_hex, @@ -597,6 +633,7 @@ const struct keytype keytypes[] = { .private_key_size = 32, .public_key_size = 0, .packed_size = 32, + .creator = create_rhizome, .packer = pack_private_only, .unpacker = unpack_private_only, .dumper = dump_raw_hex, @@ -609,6 +646,7 @@ const struct keytype keytypes[] = { .private_key_size = 32, .public_key_size = 64, .packed_size = 32 + 64, + .creator = NULL, // not included in a newly created identity .packer = pack_did_name, .unpacker = unpack_did_name, .dumper = dump_did_name, @@ -644,8 +682,8 @@ static keypair *keyring_alloc_keypair(unsigned ktype, size_t len) kp->private_key_len = len; kp->public_key_len = 0; } - if ( (kp->private_key_len && (kp->private_key = emalloc_zero(kp->private_key_len)) == NULL) - || (kp->public_key_len && (kp->public_key = emalloc_zero(kp->public_key_len)) == NULL) + if ( (kp->private_key_len && (kp->private_key = emalloc(kp->private_key_len)) == NULL) + || (kp->public_key_len && (kp->public_key = emalloc(kp->public_key_len)) == NULL) ) { keyring_free_keypair(kp); return NULL; @@ -972,15 +1010,8 @@ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, in // add any unlocked subscribers to our memory table, flagged as local sid's int i=0; for (i=0;ikeypair_count;i++){ - if (id->keypairs[i]->type == KEYTYPE_CRYPTOBOX){ - id->subscriber = find_subscriber(id->keypairs[i]->public_key, SID_SIZE, 1); - if (id->subscriber){ - if (id->subscriber->reachable==REACHABLE_NONE) - id->subscriber->reachable=REACHABLE_SELF; - id->subscriber->identity = id; - if (!my_subscriber) - my_subscriber=id->subscriber; - } + if (id->keypairs[i]->type == KEYTYPE_CRYPTOBOX) { + add_subscriber(id, i); // only one key per identity supported break; } @@ -1080,6 +1111,18 @@ static unsigned find_free_slot(keyring_file *k) return 0; } +static void keyring_commit_identity(keyring_file *k, keyring_context *cx, keyring_identity *id) +{ + set_slot(k, id->slot, 1); + cx->identities[cx->identity_count++] = id; + unsigned keypair_sid; + for (keypair_sid = 0; keypair_sid < id->keypair_count; ++keypair_sid) + if (id->keypairs[keypair_sid]->type == KEYTYPE_CRYPTOBOX) + break; + if (keypair_sid < id->keypair_count) + add_subscriber(id, keypair_sid); +} + /* Create a new identity in the specified context (which supplies the keyring pin) with the specified PKR pin. The crypto_box and crypto_sign key pairs are automatically created, and the PKR @@ -1115,97 +1158,28 @@ keyring_identity *keyring_create_identity(keyring_file *k, keyring_context *c, c } /* Allocate key pairs */ - - /* crypto_box key pair */ - id->keypairs[0] = emalloc_zero(sizeof(keypair)); - if (!id->keypairs[0]) { - WHY("malloc failed preparing first key pair storage"); - goto kci_safeexit; - } - id->keypair_count=1; - id->keypairs[0]->type=KEYTYPE_CRYPTOBOX; - id->keypairs[0]->private_key_len=crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES; - id->keypairs[0]->private_key = emalloc(id->keypairs[0]->private_key_len); - if (!id->keypairs[0]->private_key) { - WHY("malloc failed preparing first private key storage"); - goto kci_safeexit; - } - id->keypairs[0]->public_key_len=crypto_box_curve25519xsalsa20poly1305_PUBLICKEYBYTES; - id->keypairs[0]->public_key = emalloc(id->keypairs[0]->public_key_len); - if (!id->keypairs[0]->public_key) { - WHY("malloc failed preparing first public key storage"); - goto kci_safeexit; - } - /* Filter out public keys that start with 0x0, as they are reserved for address - abbreviation. */ - id->keypairs[0]->public_key[0]=0; - while(id->keypairs[0]->public_key[0]<0x10) - crypto_box_curve25519xsalsa20poly1305_keypair(id->keypairs[0]->public_key, - id->keypairs[0]->private_key); - - /* crypto_sign key pair */ - id->keypairs[1] = emalloc_zero(sizeof(keypair)); - if (!id->keypairs[1]) { - WHY("malloc failed preparing second key pair storage"); - goto kci_safeexit; - } - id->keypair_count=2; - id->keypairs[1]->type=KEYTYPE_CRYPTOSIGN; - id->keypairs[1]->private_key_len=crypto_sign_edwards25519sha512batch_SECRETKEYBYTES; - id->keypairs[1]->private_key = emalloc(id->keypairs[1]->private_key_len); - if (!id->keypairs[1]->private_key) { - WHY("malloc failed preparing second private key storage"); - goto kci_safeexit; - } - id->keypairs[1]->public_key_len=crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES; - id->keypairs[1]->public_key = emalloc(id->keypairs[1]->public_key_len); - if (!id->keypairs[1]->public_key) { - WHY("malloc failed preparing second public key storage"); - goto kci_safeexit; + unsigned ktype; + for (ktype = 1; ktype < NELS(keytypes); ++ktype) { + if (keytypes[ktype].creator) { + keypair *kp = id->keypairs[id->keypair_count] = keyring_alloc_keypair(ktype, 0); + if (kp == NULL) + goto kci_safeexit; + keytypes[ktype].creator(kp); + ++id->keypair_count; + } } - crypto_sign_edwards25519sha512batch_keypair(id->keypairs[1]->public_key, - id->keypairs[1]->private_key); - - /* Rhizome Secret (for protecting Bundle Private Keys) */ - id->keypairs[2] = emalloc_zero(sizeof(keypair)); - if (!id->keypairs[2]) { - WHY("malloc failed preparing second key pair storage"); - goto kci_safeexit; - } - id->keypair_count=3; - id->keypairs[2]->type=KEYTYPE_RHIZOME; - id->keypairs[2]->private_key_len=32; - id->keypairs[2]->private_key = emalloc(id->keypairs[2]->private_key_len); - if (!id->keypairs[2]->private_key) { - WHY("malloc failed preparing second private key storage"); - goto kci_safeexit; - } - id->keypairs[2]->public_key_len=0; - id->keypairs[2]->public_key=NULL; - urandombytes(id->keypairs[2]->private_key,id->keypairs[2]->private_key_len); - - /* Mark slot in use */ - set_slot(k, id->slot, 1); - - /* Add identity to data structure */ - c->identities[c->identity_count++]=id; - - // add new identity to in memory table - id->subscriber = find_subscriber(id->keypairs[0]->public_key, SID_SIZE, 1); - if (id->subscriber) { - if (id->subscriber->reachable==REACHABLE_NONE) - id->subscriber->reachable=REACHABLE_SELF; - id->subscriber->identity = id; - if (!my_subscriber) - my_subscriber=id->subscriber; - } + /* Mark slot as occupied and internalise new identity. */ + assert(id->keypair_count > 0); + assert(id->keypairs[0]->type == KEYTYPE_CRYPTOBOX); + keyring_commit_identity(k, c, id); /* Everything went fine */ return id; kci_safeexit: - if (id) keyring_free_identity(id); + if (id) + keyring_free_identity(id); return NULL; } @@ -1296,7 +1270,7 @@ int keyring_set_did(keyring_identity *id, const char *did, const char *name) return WHY("Too many key pairs"); /* allocate if needed */ if (i >= id->keypair_count) { - if ((id->keypairs[i] = keyring_alloc_keypair(KEYTYPE_DID, keytypes[KEYTYPE_DID].packed_size)) == NULL) + if ((id->keypairs[i] = keyring_alloc_keypair(KEYTYPE_DID, 0)) == NULL) return -1; ++id->keypair_count; if (config.debug.keyring) @@ -1794,47 +1768,78 @@ int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE *input) { + assert(cn < k->context_count); + keyring_context *cx = k->contexts[cn]; clearerr(input); - while (1) { - unsigned id; + char line[1024]; + unsigned pini = 0; + keyring_identity *id = NULL; + unsigned last_idn = 0; + while (fgets(line, sizeof line - 1, input) != NULL) { + // Strip trailing \n or CRLF + size_t linelen = strlen(line); + if (linelen && line[linelen - 1] == '\n') { + line[--linelen] = '\0'; + if (linelen && line[linelen - 1] == '\r') + line[--linelen] = '\0'; + } else + return WHY("line too long"); + unsigned idn; unsigned ktype; - char ktypestr[100]; - char content[1024]; - content[0] = '\0'; - int n = fscanf(input, "%u: type=%u(%99[^)]) ", &id, &ktype, ktypestr); + int i, j; + int n = sscanf(line, "%u: type=%u (%n%*[^)]%n)", &idn, &ktype, &i, &j); if (n == EOF && (ferror(input) || feof(input))) break; - if (n != 3) - return WHY("malformed input"); + if (n != 2) + return WHYF("malformed input n=%u", n); if (ktype == 0) - return WHY("invalid input: key type = 0"); - fgets(content, sizeof content - 1, input); - // Strip trailing \n or CRLF - size_t contentlen = strlen(content); - if (contentlen && content[contentlen - 1] == '\n') { - content[--contentlen] = '\0'; - if (contentlen && content[contentlen - 1] == '\r') - content[--contentlen] = '\0'; - } - //DEBUGF("n=%d content=%s", n, alloca_str_toprint(content)); + return WHY("invalid input: ktype=0"); + const char *ktypestr = &line[i]; + line[j] = '\0'; + const char *content = &line[j + 1]; + //DEBUGF("n=%d i=%u ktypestr=%s j=%u content=%s", n, i, alloca_str_toprint(ktypestr), j, alloca_str_toprint(content)); keypair *kp = keyring_alloc_keypair(ktype, 0); if (kp == NULL) return -1; int (*loader)(keypair *, const char *) = load_raw_hex; if (strcmp(ktypestr, "unknown") != 0 && ktype < NELS(keytypes)) loader = keytypes[ktype].loader; - if (loader(kp, content) == -1) + if (loader(kp, content) == -1) { + keyring_free_keypair(kp); return -1; - /* + } + if (id == NULL || idn != last_idn) { + last_idn = idn; + // TODO: do not commit any identities until entire dump is parsed + // TODO: discard duplicate identities + if (id) + keyring_commit_identity(k, cx, id); + if ((id = emalloc_zero(sizeof(keyring_identity))) == NULL) { + keyring_free_keypair(kp); + return -1; + } + if ((id->PKRPin = str_edup(pini < pinc ? pinv[pini++] : "")) == NULL) { + keyring_free_keypair(kp); + keyring_free_identity(id); + return -1; + } + /* Find free slot in keyring. */ + if ((id->slot = find_free_slot(k)) == 0) { + keyring_free_keypair(kp); + keyring_free_identity(id); + return WHY("no free slot"); + } + } if (!keyring_identity_add_keypair(id, kp)) keyring_free_keypair(kp); - */ /* fprintf(stderr, "%u: ", id); keyring_dump_keypair(kp, XPRINTF_STDIO(stderr), 1); fprintf(stderr, "\n"); */ } + if (id) + keyring_commit_identity(k, cx, id); if (ferror(input)) return WHYF_perror("fscanf"); return 0; diff --git a/serval.h b/serval.h index f457ddd6..268802dd 100644 --- a/serval.h +++ b/serval.h @@ -255,7 +255,7 @@ void keyring_free_context(keyring_context *c); void keyring_free_identity(keyring_identity *id); int keyring_identity_mac(keyring_context *c,keyring_identity *id, unsigned char *pkrsalt,unsigned char *mac); -#define KEYTYPE_CRYPTOBOX 0x01 +#define KEYTYPE_CRYPTOBOX 0x01 // must be lowest #define KEYTYPE_CRYPTOSIGN 0x02 #define KEYTYPE_RHIZOME 0x03 /* DIDs aren't really keys, but the keyring is a real handy place to keep them, diff --git a/tests/keyring b/tests/keyring index 09032faf..d9729042 100755 --- a/tests/keyring +++ b/tests/keyring @@ -25,11 +25,19 @@ shopt -s extglob setup() { setup_servald - executeOk_servald config \ - set log.console.level debug \ - set debug.keyring on - executeOk_servald keyring list - assert_keyring_list 0 + setup_instances +A + set_instance +A +} + +setup_instances() { + for arg; do + set_instance $arg + executeOk_servald config \ + set log.console.level debug \ + set debug.keyring on + executeOk_servald keyring list + assert_keyring_list 0 + done } assert_keyring_list() { @@ -138,6 +146,36 @@ teardown_KeyringAutoCreate() { report_servald_server } +doc_Load="Load keyring entries from a keyring dump" +setup_Load() { + setup_servald + setup_instances +A +B + set_instance +A + executeOk_servald keyring add '' + executeOk_servald keyring add '' + executeOk_servald keyring dump dA + set_instance +B + executeOk_servald keyring add '' + executeOk_servald keyring dump dB + set_instance +A + tfw_cat dA dB + assert ! cmp dA dB +} +test_Load() { + set_instance +B + executeOk_servald keyring load dA + tfw_cat --stderr + executeOk_servald keyring dump dBA + tfw_cat dBA + while read line; do + tfw_log -F "${line#[0-9]}" dBA + assertGrep -- -F "${line#[0-9]}" dBA + done < dA + while read line; do + assertGrep -- -F "${line#[0-9]}" dBA + done < dB +} + doc_CompatibleBack1="Can read old keyring file (1)" setup_CompatibleBack1() { setup_servald From 55efc73b7e600889ff455ab89381738a3b27fada Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Thu, 5 Sep 2013 09:19:14 +0930 Subject: [PATCH 07/11] Improve test framework: assertGrep --fixed-strings option --- testframework.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/testframework.sh b/testframework.sh index b2839f28..083f77b7 100644 --- a/testframework.sh +++ b/testframework.sh @@ -982,8 +982,6 @@ assertGrep() { fi _tfw_dump_on_fail "$1" _tfw_get_content "$1" || return $? - local s=s - local message _tfw_assert_grep "${_tfw_opt_line_msg:+$_tfw_opt_line_msg of }$1" "$_tfw_tmp/content" "$2" || _tfw_failexit } @@ -1222,6 +1220,7 @@ _tfw_getopts() { _tfw_opt_line= _tfw_opt_line_sed= _tfw_opt_line_msg= + _tfw_opt_grepopts=() _tfw_getopts_shift=0 local oo _tfw_shopt oo -s extglob @@ -1241,6 +1240,7 @@ _tfw_getopts() { wait_until:--timeout=*) _tfw_error "invalid value: $1";; wait_until:--sleep=@(+([0-9])?(.+([0-9]))|*([0-9]).+([0-9]))) _tfw_opt_sleep="${1#*=}";; wait_until:--sleep=*) _tfw_error "invalid value: $1";; + *grep:--fixed-strings) _tfw_opt_grepopts+=(-F);; assertcontentgrep:--matches=+([0-9])) _tfw_opt_matches="${1#*=}";; assertcontentgrep:--matches=*) _tfw_error "invalid value: $1";; assertcontent*:--line=+([0-9])) _tfw_opt_line="${1#*=}"; _tfw_opt_line_msg="line $_tfw_opt_line";; @@ -1398,7 +1398,7 @@ _tfw_assert_grep() { _tfw_error "$file is not readable" ret=$? else - local matches=$(( $($GREP --regexp="$pattern" "$file" | wc -l) + 0 )) + local matches=$(( $($GREP "${_tfw_opt_grepopts[@]}" --regexp="$pattern" "$file" | wc -l) + 0 )) local done=false local ret=0 local info="$matches match"$([ $matches -ne 1 ] && echo "es") From 4b0550afd114e85344e0a3e08d4f650e105ba441 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Thu, 5 Sep 2013 09:19:45 +0930 Subject: [PATCH 08/11] Fix new keyring load test --- tests/keyring | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/keyring b/tests/keyring index d9729042..c0dd3815 100755 --- a/tests/keyring +++ b/tests/keyring @@ -168,11 +168,10 @@ test_Load() { executeOk_servald keyring dump dBA tfw_cat dBA while read line; do - tfw_log -F "${line#[0-9]}" dBA - assertGrep -- -F "${line#[0-9]}" dBA + assertGrep --fixed-strings dBA "${line#[0-9]}" done < dA while read line; do - assertGrep -- -F "${line#[0-9]}" dBA + assertGrep --fixed-strings dBA "${line#[0-9]}" done < dB } From 8a300c2520dfcfcfe27339a62c20f78e08414ed3 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Thu, 5 Sep 2013 16:34:01 +0930 Subject: [PATCH 09/11] Get new keyring dump command and test working --- commandline.c | 19 ++- keyring.c | 349 +++++++++++++++++++++++++++++++------------------- serval.h | 2 - tests/keyring | 6 +- 4 files changed, 235 insertions(+), 141 deletions(-) diff --git a/commandline.c b/commandline.c index 0498ebf0..4467d2c9 100644 --- a/commandline.c +++ b/commandline.c @@ -1791,10 +1791,21 @@ int app_keyring_load(const struct cli_parsed *parsed, struct cli_context *contex if (!k) return -1; FILE *fp = path && strcmp(path, "-") != 0 ? fopen(path, "r") : stdin; - if (fp == NULL) - return WHYF_perror("fopen(%s, \"r\")", alloca_str_toprint(path)); - int ret = keyring_load(k, 0, pinc, pinv, fp); - return ret; + if (fp == NULL) { + WHYF_perror("fopen(%s, \"r\")", alloca_str_toprint(path)); + keyring_free(k); + return -1; + } + if (keyring_load(k, 0, pinc, pinv, fp) == -1) { + keyring_free(k); + return -1; + } + if (keyring_commit(k) == -1) { + keyring_free(k); + return WHY("Could not write new identity"); + } + keyring_free(k); + return 0; } int app_keyring_list(const struct cli_parsed *parsed, struct cli_context *context) diff --git a/keyring.c b/keyring.c index 0f651318..87a100f9 100644 --- a/keyring.c +++ b/keyring.c @@ -29,6 +29,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. #include "overlay_address.h" static void keyring_free_keypair(keypair *kp); +static int keyring_identity_mac(const keyring_identity *id, unsigned char *pkrsalt, unsigned char *mac); static int _keyring_open(keyring_file *k, const char *path, const char *mode) { @@ -290,31 +291,30 @@ int keyring_enter_keyringpin(keyring_file *k, const char *pin) { if (config.debug.keyring) DEBUGF("k=%p", k); - if (!k) return WHY("k is null"); - if (k->context_count>=KEYRING_MAX_CONTEXTS) + if (!k) + return WHY("k is null"); + if (k->context_count >= KEYRING_MAX_CONTEXTS) return WHY("Too many loaded contexts already"); - if (k->context_count<1) + if (k->context_count < 1) return WHY("Cannot enter PIN without keyring salt being available"); int cn; for (cn = 0; cn < k->context_count; ++cn) if (strcmp(k->contexts[cn]->KeyRingPin, pin) == 0) return 1; - k->contexts[k->context_count] = emalloc_zero(sizeof(keyring_context)); - if (!k->contexts[k->context_count]) - return WHY("Could not allocate new keyring context structure"); - keyring_context *c=k->contexts[k->context_count]; - /* Store pin */ - c->KeyRingPin = pin ? str_edup(pin) : str_edup(""); - /* Get salt from the zeroeth context */ - c->KeyRingSalt = emalloc(k->contexts[0]->KeyRingSaltLen); - if (!c->KeyRingSalt) { + keyring_context *c = emalloc_zero(sizeof(keyring_context)); + if (c == NULL) + return -1; + /* Store pin and copy salt from the zeroeth context */ + c->KeyRingSaltLen = k->contexts[0]->KeyRingSaltLen; + if ( ((c->KeyRingPin = str_edup(pin ? pin : "")) == NULL) + || ((c->KeyRingSalt = emalloc(c->KeyRingSaltLen)) == NULL) + ) { + keyring_free_context(c); free(c); - k->contexts[k->context_count]=NULL; - return WHY("Could not copy keyring salt from context zero"); + return -1; } - c->KeyRingSaltLen=k->contexts[0]->KeyRingSaltLen; - bcopy(&k->contexts[0]->KeyRingSalt[0],&c->KeyRingSalt[0],c->KeyRingSaltLen); - k->context_count++; + bcopy(k->contexts[0]->KeyRingSalt, c->KeyRingSalt, c->KeyRingSaltLen); + k->contexts[k->context_count++] = c; return 0; } @@ -329,6 +329,8 @@ int keyring_munge_block(unsigned char *block,int len /* includes the first 96 by unsigned char *KeyRingSalt,int KeyRingSaltLen, const char *KeyRingPin, const char *PKRPin) { + if (config.debug.keyring) + DEBUGF("KeyRingPin=%s PKRPin=%s", alloca_str_toprint(KeyRingPin), alloca_str_toprint(PKRPin)); int exit_code=1; unsigned char hashKey[crypto_hash_sha512_BYTES]; unsigned char hashNonce[crypto_hash_sha512_BYTES]; @@ -424,6 +426,22 @@ static void create_cryptobox(keypair *kp) } while (kp->public_key[0] < 0x10); } +/* Compute public key from private key. + * + * Public key calculation as below is taken from section 3 of: + * http://cr.yp.to/highspeed/naclcrypto-20090310.pdf + * + * This can take a while on a mobile phone since it involves a scalarmult operation, so searching + * through all slots for a pin could take a while (perhaps 1 second per pin:slot cominbation). This + * is both good and bad. The other option is to store the public key as well, which would make + * entering a pin faster, but would also make trying an incorrect pin faster, thus simplifying some + * brute-force attacks. We need to make a decision between speed/convenience and security here. + */ +static void _derive_scalarmult_public(unsigned char *public, const unsigned char *private) +{ + crypto_scalarmult_curve25519_base(public, private); +} + static void create_cryptosign(keypair *kp) { crypto_sign_edwards25519sha512batch_keypair(kp->public_key, kp->private_key); @@ -447,7 +465,7 @@ static int pack_private_public(const keypair *kp, struct rotbuf *rb) return 0; } -static void dump_raw_hex(const keypair *kp, XPRINTF xpf, int include_secret) +static void dump_private_public(const keypair *kp, XPRINTF xpf, int include_secret) { if (kp->public_key_len) xprintf(xpf, " pub=%s", alloca_tohex(kp->public_key, kp->public_key_len)); @@ -455,42 +473,121 @@ static void dump_raw_hex(const keypair *kp, XPRINTF xpf, int include_secret) xprintf(xpf, " sec=%s", alloca_tohex(kp->private_key, kp->private_key_len)); } -static int load_raw_hex(keypair *kp, const char *text) +static int _load_decode_hex(const char **hex, unsigned char **buf, size_t *len) { + const char *end = NULL; + size_t hexlen = strn_fromhex(NULL, -1, *hex, &end); + if (hexlen == 0 || end == NULL || (*end != ' ' && *end != '\0')) + return WHY("malformed hex value"); + if (*len == 0) { + assert(*buf == NULL); + *len = hexlen; + if ((*buf = emalloc_zero(*len)) == NULL) + return -1; + } + else if (hexlen != *len) + return WHYF("invalid hex value, incorrect length (expecting %u bytes, got %u)", *len, hexlen); + strn_fromhex(*buf, *len, *hex, hex); + assert(*hex == end); + return 0; +} + +static int load_private_public(keypair *kp, const char *text) +{ + assert(kp->public_key_len != 0); + assert(kp->public_key != NULL); + assert(kp->private_key_len != 0); + assert(kp->private_key != NULL); + const char *t = text; + int got_pub = 0; + int got_sec = 0; + while (*t) { + while (isspace(*t)) + ++t; + if (str_startswith(t, "pub=", &t)) { + if (_load_decode_hex(&t, &kp->public_key, &kp->public_key_len) == -1) + WHY("cannot decode pub= field"); + else + got_pub = 1; + } + else if (str_startswith(t, "sec=", &t)) { + if (_load_decode_hex(&t, &kp->private_key, &kp->private_key_len) == -1) + WHY("cannot decode sec= field"); + else + got_sec = 1; + } + else if (*t) + return WHYF("unsupported dump field: %s", t); + } + if (!got_sec) + return WHY("missing sec= field"); + if (!got_pub) + return WHY("missing pub= field"); + return 0; +} + +static int load_private(keypair *kp, const char *text) +{ + assert(kp->private_key_len != 0); + assert(kp->private_key != NULL); + const char *t = text; + int got_sec = 0; + while (*t) { + while (isspace(*t)) + ++t; + if (str_startswith(t, "sec=", &t)) { + if (_load_decode_hex(&t, &kp->private_key, &kp->private_key_len) == -1) + WHY("cannot decode sec= field"); + else + got_sec = 1; + } else if (str_startswith(t, "pub=", &t)) { + WARN("skipping pub= field"); + while (*t && !isspace(*t)) + ++t; + } + else if (*t) + return WHYF("unsupported dump field: %s", t); + } + if (!got_sec) + return WHY("missing sec= field"); + return 0; +} + +static int load_cryptobox(keypair *kp, const char *text) +{ + if (load_private(kp, text) == -1) + return -1; + _derive_scalarmult_public(kp->public_key, kp->private_key); + return 0; +} + +static int load_private_only(keypair *kp, const char *text) +{ + assert(kp->public_key_len == 0); + assert(kp->public_key == NULL); + return load_private(kp, text); +} + +static int load_unknown(keypair *kp, const char *text) +{ + assert(kp->private_key_len == 0); + assert(kp->private_key == NULL); + assert(kp->public_key_len == 0); + assert(kp->public_key == NULL); const char *t = text; while (*t) { while (isspace(*t)) ++t; if (str_startswith(t, "pub=", &t)) { - const char *e = NULL; - size_t len = strn_fromhex(NULL, -1, t, &e); - if (len == 0 || e == NULL || (*e != ' ' && *e != '\0')) - return WHY("malformed pub= hex value"); - if (kp->public_key_len == 0) { - assert(kp->public_key == NULL); - kp->public_key_len = len; - if ((kp->public_key = emalloc_zero(len)) == NULL) - return -1; - } else if (len != kp->public_key_len) - return WHY("invalid pub= hex value, incorrect length"); - strn_fromhex(kp->public_key, kp->public_key_len, t, &t); + if (_load_decode_hex(&t, &kp->public_key, &kp->public_key_len) == -1) + WHY("cannot decode pub= field"); } else if (str_startswith(t, "sec=", &t)) { - const char *e = NULL; - size_t len = strn_fromhex(NULL, -1, t, &e); - if (len == 0 || e == NULL || (*e != ' ' && *e != '\0')) - return WHY("malformed sec= hex value"); - if (kp->private_key_len == 0) { - assert(kp->private_key == NULL); - kp->private_key_len = len; - if ((kp->private_key = emalloc_zero(len)) == NULL) - return -1; - } else if (len != kp->private_key_len) - return WHY("invalid sec= hex value, incorrect length"); - strn_fromhex(kp->private_key, kp->private_key_len, t, &t); + if (_load_decode_hex(&t, &kp->private_key, &kp->private_key_len) == -1) + WHY("cannot decode sec= field"); } else if (*t) - return WHYF("unsupported dump content: %s", t); + return WHYF("unsupported dump field: %s", t); } return 0; } @@ -508,23 +605,11 @@ static int unpack_private_only(keypair *kp, struct rotbuf *rb) return 0; } -static int unpack_private_derive_scalarmult_public(keypair *kp, struct rotbuf *rb) +static int unpack_cryptobox(keypair *kp, struct rotbuf *rb) { rotbuf_getbuf(rb, kp->private_key, kp->private_key_len); - /* Compute public key from private key. - * - * Public key calculation as below is taken from section 3 of: - * http://cr.yp.to/highspeed/naclcrypto-20090310.pdf - * - * This can take a while on a mobile phone since it involves a scalarmult operation, so searching - * through all slots for a pin could take a while (perhaps 1 second per pin:slot cominbation). - * This is both good and bad. The other option is to store the public key as well, which would - * make entering a pin faster, but would also make trying an incorrect pin faster, thus - * simplifying some brute-force attacks. We need to make a decision between speed/convenience - * and security here. - */ if (!rb->wrap) - crypto_scalarmult_curve25519_base(kp->public_key, kp->private_key); + _derive_scalarmult_public(kp->public_key, kp->private_key); return 0; } @@ -607,9 +692,9 @@ const struct keytype keytypes[] = { .packed_size = crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES, .creator = create_cryptobox, .packer = pack_private_only, - .unpacker = unpack_private_derive_scalarmult_public, - .dumper = dump_raw_hex, - .loader = load_raw_hex + .unpacker = unpack_cryptobox, + .dumper = dump_private_public, + .loader = load_cryptobox }, [KEYTYPE_CRYPTOSIGN] = { /* The NaCl API does not expose any method to derive a cryptosign public key from its private @@ -623,8 +708,8 @@ const struct keytype keytypes[] = { .creator = create_cryptosign, .packer = pack_private_public, .unpacker = unpack_private_public, - .dumper = dump_raw_hex, - .loader = load_raw_hex + .dumper = dump_private_public, + .loader = load_private_public }, [KEYTYPE_RHIZOME] = { /* The Rhizome Secret (a large, unguessable number) is stored in the private key field, and @@ -636,8 +721,8 @@ const struct keytype keytypes[] = { .creator = create_rhizome, .packer = pack_private_only, .unpacker = unpack_private_only, - .dumper = dump_raw_hex, - .loader = load_raw_hex + .dumper = dump_private_public, + .loader = load_private_only }, [KEYTYPE_DID] = { /* The DID is stored in unpacked form in the private key field, and the name in nul-terminated @@ -691,15 +776,15 @@ static keypair *keyring_alloc_keypair(unsigned ktype, size_t len) return kp; } -static int keyring_pack_identity(keyring_context *c, keyring_identity *id, unsigned char packed[KEYRING_PAGE_SIZE]) +static int keyring_pack_identity(const keyring_identity *id, unsigned char packed[KEYRING_PAGE_SIZE]) { /* Convert an identity to a KEYRING_PAGE_SIZE bytes long block that consists of 32 bytes of random * salt, a 64 byte (512 bit) message authentication code (MAC) and the list of key pairs. */ if (urandombytes(packed, PKR_SALT_BYTES) == -1) return WHY("Could not generate salt"); /* Calculate MAC */ - keyring_identity_mac(c, id, packed /* pkr salt */, - packed + PKR_SALT_BYTES /* write mac in after salt */); + if (keyring_identity_mac(id, packed /* pkr salt */, packed + PKR_SALT_BYTES /* write mac in after salt */) == -1) + return -1; /* There was a known plain-text opportunity here: byte 96 must be 0x01, and some other bytes are * likely deducible, e.g., the location of the trailing 0x00 byte can probably be guessed with * confidence. Payload rotation will frustrate this attack. @@ -935,8 +1020,7 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char return id; } -int keyring_identity_mac(keyring_context *c, keyring_identity *id, - unsigned char *pkrsalt,unsigned char *mac) +static int keyring_identity_mac(const keyring_identity *id, unsigned char *pkrsalt, unsigned char *mac) { //assert(id->keypair_count >= 1); unsigned char work[65536]; @@ -952,10 +1036,14 @@ int keyring_identity_mac(keyring_context *c, keyring_identity *id, ofs += __len; \ } APPEND(&pkrsalt[0], 32); + if (id->keypairs[0]->type != KEYTYPE_CRYPTOBOX) + return WHY("first keypair is not type CRYPTOBOX"); APPEND(id->keypairs[0]->private_key, id->keypairs[0]->private_key_len); APPEND(id->keypairs[0]->public_key, id->keypairs[0]->public_key_len); APPEND(id->PKRPin, strlen(id->PKRPin)); #undef APPEND + if (config.debug.keyring) + dump("work", work, ofs); crypto_hash_sha512(mac, work, ofs); return 0; } @@ -970,44 +1058,36 @@ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, in { int exit_code=1; unsigned char slot[KEYRING_PAGE_SIZE]; - unsigned char hash[crypto_hash_sha512_BYTES]; keyring_identity *id=NULL; /* 1. Read slot. */ if (fseeko(k->file,slot_number*KEYRING_PAGE_SIZE,SEEK_SET)) return WHY_perror("fseeko"); - if (fread(&slot[0],KEYRING_PAGE_SIZE,1,k->file)!=1) + if (fread(slot, KEYRING_PAGE_SIZE, 1, k->file) != 1) return WHY_perror("fread"); /* 2. Decrypt data from slot. */ - if (keyring_munge_block(slot,KEYRING_PAGE_SIZE, - k->contexts[0]->KeyRingSalt, - k->contexts[0]->KeyRingSaltLen, - c->KeyRingPin,pin)) { + if (keyring_munge_block(slot, KEYRING_PAGE_SIZE, c->KeyRingSalt, c->KeyRingSaltLen, c->KeyRingPin, pin)) { WHYF("keyring_munge_block() failed, slot=%u", slot_number); goto kdp_safeexit; } - /* 3. Unpack contents of slot into a new identity in the provided context. */ if (config.debug.keyring) DEBUGF("unpack slot %u", slot_number); if (((id = keyring_unpack_identity(slot, pin)) == NULL) || id->keypair_count < 1) goto kdp_safeexit; // Not a valid slot id->slot = slot_number; - /* 4. Verify that slot is self-consistent (check MAC) */ - if (keyring_identity_mac(k->contexts[0],id,&slot[0],hash)) { - WHY("could not calculate MAC for identity"); + unsigned char hash[crypto_hash_sha512_BYTES]; + if (keyring_identity_mac(id, slot, hash)) goto kdp_safeexit; - } /* compare hash to record */ - if (memcmp(hash,&slot[32],crypto_hash_sha512_BYTES)) { + if (memcmp(hash, &slot[PKR_SALT_BYTES], crypto_hash_sha512_BYTES)) { WHYF("slot %u is not valid (MAC mismatch)", slot_number); dump("computed",hash,crypto_hash_sha512_BYTES); - dump("stored",&slot[32],crypto_hash_sha512_BYTES); + dump("stored",&slot[PKR_SALT_BYTES],crypto_hash_sha512_BYTES); goto kdp_safeexit; } - - // add any unlocked subscribers to our memory table, flagged as local sid's + // Add any unlocked subscribers to our memory table, flagged as local SIDs. int i=0; for (i=0;ikeypair_count;i++){ if (id->keypairs[i]->type == KEYTYPE_CRYPTOBOX) { @@ -1016,10 +1096,8 @@ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, in break; } } - - /* Well, it's all fine, so add the id into the context and return */ + /* All fine, so add the id into the context and return. */ c->identities[c->identity_count++]=id; - return 0; kdp_safeexit: @@ -1123,11 +1201,10 @@ static void keyring_commit_identity(keyring_file *k, keyring_context *cx, keyrin add_subscriber(id, keypair_sid); } -/* Create a new identity in the specified context (which supplies the keyring pin) - with the specified PKR pin. - The crypto_box and crypto_sign key pairs are automatically created, and the PKR - is packed and written to a hithero unallocated slot which is then marked full. - Requires an explicit call to keyring_commit() +/* Create a new identity in the specified context (which supplies the keyring pin) with the + * specified PKR pin. The crypto_box and crypto_sign key pairs are automatically created, and the + * PKR is packed and written to a hithero unallocated slot which is then marked full. Requires an + * explicit call to keyring_commit() */ keyring_identity *keyring_create_identity(keyring_file *k, keyring_context *c, const char *pin) { @@ -1137,7 +1214,7 @@ keyring_identity *keyring_create_identity(keyring_file *k, keyring_context *c, c if (!k) { WHY("keyring is NULL"); return NULL; } if (!k->bam) { WHY("keyring lacks BAM (not to be confused with KAPOW)"); return NULL; } if (!c) { WHY("keyring context is NULL"); return NULL; } - if (c->identity_count>=KEYRING_MAX_IDENTITIES) + if (c->identity_count>=KEYRING_MAX_IDENTITIES) { WHY("keyring context has too many identities"); return NULL; } if (!pin) pin=""; @@ -1187,8 +1264,10 @@ int keyring_commit(keyring_file *k) { if (config.debug.keyring) DEBUGF("k=%p", k); - if (!k) return WHY("keyring was NULL"); - if (k->context_count<1) return WHY("Keyring has no contexts"); + if (!k) + return WHY("keyring was NULL"); + if (k->context_count < 1) + return WHY("keyring has no contexts"); unsigned errorCount = 0; /* Write all BAMs */ keyring_bam *b; @@ -1205,46 +1284,46 @@ int keyring_commit(keyring_file *k) } } /* For each identity in each context, write the record to disk. - This re-salts every identity as it is re-written, and the pin + This re-salts every identity as it is re-written, and the pin for each identity and context is used, so changing a keypair or pin - is as simple as updating the keyring_identity or related structure, + is as simple as updating the keyring_identity or related structure, and then calling this function. */ - int cn,in; - for(cn=0;cncontext_count;cn++) - for(in=0;incontexts[cn]->identity_count;in++) - { - unsigned char pkr[KEYRING_PAGE_SIZE]; - if (keyring_pack_identity(k->contexts[cn], - k->contexts[cn]->identities[in], - pkr)) + unsigned cn; + for (cn = 0; cn < k->context_count; ++cn) { + if (config.debug.keyring) + DEBUGF("cn = %u", cn); + const keyring_context *cx = k->contexts[cn]; + unsigned in; + for (in = 0; in < cx->identity_count; ++in) { + if (config.debug.keyring) + DEBUGF("in = %u", in); + const keyring_identity *id = cx->identities[in]; + unsigned char pkr[KEYRING_PAGE_SIZE]; + if (keyring_pack_identity(id, pkr)) + errorCount++; + else { + /* Now crypt and store block */ + /* Crypt */ + if (keyring_munge_block(pkr, KEYRING_PAGE_SIZE, cx->KeyRingSalt, cx->KeyRingSaltLen, cx->KeyRingPin, id->PKRPin)) { + WHY("keyring_munge_block() failed"); errorCount++; - else { - /* Now crypt and store block */ - /* Crypt */ - if (keyring_munge_block(pkr, - KEYRING_PAGE_SIZE, - k->contexts[cn]->KeyRingSalt, - k->contexts[cn]->KeyRingSaltLen, - k->contexts[cn]->KeyRingPin, - k->contexts[cn]->identities[in]->PKRPin)) { - WHY("keyring_munge_block() failed"); + } else { + /* Store */ + off_t file_offset = KEYRING_PAGE_SIZE * id->slot; + if (file_offset == 0) { + if (config.debug.keyring) + DEBUGF("ID cn=%d in=%d has slot=0", cn, in); + } else if (fseeko(k->file, file_offset, SEEK_SET) == -1) { + WHYF_perror("fseeko(%d, %ld, SEEK_SET)", fileno(k->file), (long)file_offset); + errorCount++; + } else if (fwrite(pkr, KEYRING_PAGE_SIZE, 1, k->file) != 1) { + WHYF_perror("fwrite(%p, %ld, 1, %d)", pkr, (long)KEYRING_PAGE_SIZE, fileno(k->file)); errorCount++; - } else { - /* Store */ - off_t file_offset = KEYRING_PAGE_SIZE * k->contexts[cn]->identities[in]->slot; - if (!file_offset) { - if (config.debug.keyring) - DEBUGF("ID cn=%d in=%d has slot=0", cn, in); - } else if (fseeko(k->file, file_offset, SEEK_SET) == -1) { - WHYF_perror("fseeko(%d, %ld, SEEK_SET)", fileno(k->file), (long)file_offset); - errorCount++; - } else if (fwrite(pkr, KEYRING_PAGE_SIZE, 1, k->file) != 1) { - WHYF_perror("fwrite(%p, %ld, 1, %d)", pkr, (long)KEYRING_PAGE_SIZE, fileno(k->file)); - errorCount++; - } } } } + } + } if (fflush(k->file) == -1) { WHYF_perror("fflush(%d)", fileno(k->file)); errorCount++; @@ -1737,7 +1816,7 @@ static void keyring_dump_keypair(const keypair *kp, XPRINTF xpf, int include_sec if (keytypes[kp->type].dumper) keytypes[kp->type].dumper(kp, xpf, include_secret); else - dump_raw_hex(kp, xpf, include_secret); + dump_private_public(kp, xpf, include_secret); } int keyring_dump(keyring_file *k, XPRINTF xpf, int include_secret) @@ -1801,7 +1880,7 @@ int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE keypair *kp = keyring_alloc_keypair(ktype, 0); if (kp == NULL) return -1; - int (*loader)(keypair *, const char *) = load_raw_hex; + int (*loader)(keypair *, const char *) = load_unknown; if (strcmp(ktypestr, "unknown") != 0 && ktype < NELS(keytypes)) loader = keytypes[ktype].loader; if (loader(kp, content) == -1) { @@ -1830,8 +1909,14 @@ int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE return WHY("no free slot"); } } - if (!keyring_identity_add_keypair(id, kp)) + if (id->keypair_count < PKR_MAX_KEYPAIRS) { + if (!keyring_identity_add_keypair(id, kp)) + keyring_free_keypair(kp); + } else { keyring_free_keypair(kp); + keyring_free_identity(id); + return WHY("too many key pairs"); + } /* fprintf(stderr, "%u: ", id); keyring_dump_keypair(kp, XPRINTF_STDIO(stderr), 1); diff --git a/serval.h b/serval.h index 268802dd..81b442c5 100644 --- a/serval.h +++ b/serval.h @@ -253,8 +253,6 @@ typedef struct keyring_file { void keyring_free(keyring_file *k); void keyring_free_context(keyring_context *c); void keyring_free_identity(keyring_identity *id); -int keyring_identity_mac(keyring_context *c,keyring_identity *id, - unsigned char *pkrsalt,unsigned char *mac); #define KEYTYPE_CRYPTOBOX 0x01 // must be lowest #define KEYTYPE_CRYPTOSIGN 0x02 #define KEYTYPE_RHIZOME 0x03 diff --git a/tests/keyring b/tests/keyring index c0dd3815..cab96cf2 100755 --- a/tests/keyring +++ b/tests/keyring @@ -153,10 +153,10 @@ setup_Load() { set_instance +A executeOk_servald keyring add '' executeOk_servald keyring add '' - executeOk_servald keyring dump dA + executeOk_servald keyring dump --secret dA set_instance +B executeOk_servald keyring add '' - executeOk_servald keyring dump dB + executeOk_servald keyring dump --secret dB set_instance +A tfw_cat dA dB assert ! cmp dA dB @@ -165,7 +165,7 @@ test_Load() { set_instance +B executeOk_servald keyring load dA tfw_cat --stderr - executeOk_servald keyring dump dBA + executeOk_servald keyring dump --secret dBA tfw_cat dBA while read line; do assertGrep --fixed-strings dBA "${line#[0-9]}" From befb658958ac623a1a5ffac81902305e669c8cd6 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Thu, 5 Sep 2013 16:34:52 +0930 Subject: [PATCH 10/11] All keyring commands now close (free) the keyring --- commandline.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/commandline.c b/commandline.c index 4467d2c9..0e540963 100644 --- a/commandline.c +++ b/commandline.c @@ -1743,8 +1743,10 @@ int app_keyring_create(const struct cli_parsed *parsed, struct cli_context *cont { if (config.debug.verbose) DEBUG_cli_parsed(parsed); - if (!keyring_open_instance()) + keyring_file *k = keyring_open_instance(); + if (!k) return -1; + keyring_free(k); return 0; } @@ -1760,11 +1762,18 @@ int app_keyring_dump(const struct cli_parsed *parsed, struct cli_context *contex if (!k) return -1; FILE *fp = path ? fopen(path, "w") : stdout; - if (fp == NULL) - return WHYF_perror("fopen(%s, \"w\")", alloca_str_toprint(path)); + if (fp == NULL) { + WHYF_perror("fopen(%s, \"w\")", alloca_str_toprint(path)); + keyring_free(k); + return -1; + } int ret = keyring_dump(k, XPRINTF_STDIO(fp), include_secret); - if (fp != stdout && fclose(fp) == EOF) - return WHYF_perror("fclose(%s)", alloca_str_toprint(path)); + if (fp != stdout && fclose(fp) == EOF) { + WHYF_perror("fclose(%s)", alloca_str_toprint(path)); + keyring_free(k); + return -1; + } + keyring_free(k); return ret; } @@ -1828,6 +1837,7 @@ int app_keyring_list(const struct cli_parsed *parsed, struct cli_context *contex cli_put_string(context, name, "\n"); } } + keyring_free(k); return 0; } From 18e2916cec7dc50531cd97eb9d580b0663bd3bdc Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Sat, 7 Sep 2013 04:03:28 +0930 Subject: [PATCH 11/11] Add more keyring load tests, fix duplicate identity bugs --- keyring.c | 120 +++++++++++++++++++++++++++++--------------------- tests/keyring | 74 +++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 50 deletions(-) diff --git a/keyring.c b/keyring.c index 87a100f9..871aa677 100644 --- a/keyring.c +++ b/keyring.c @@ -902,6 +902,9 @@ static int cmp_keypair(const keypair *a, const keypair *b) return c; } +/* Ensure that regardless of the order in the keyring file or loaded dump, keypairs are always + * stored in memory in ascending order of (key type, public key, private key). + */ static int keyring_identity_add_keypair(keyring_identity *id, keypair *kp) { assert(id->keypair_count < PKR_MAX_KEYPAIRS); @@ -909,7 +912,8 @@ static int keyring_identity_add_keypair(keyring_identity *id, keypair *kp) int c = 1; unsigned i = 0; for (i = 0; i < id->keypair_count && (c = cmp_keypair(id->keypairs[i], kp)) < 0; ++i) - ; + if (i) + assert(cmp_keypair(id->keypairs[i - 1], id->keypairs[i]) < 0); if (c == 0) return 0; // duplicate not inserted unsigned j; @@ -1022,7 +1026,6 @@ static keyring_identity *keyring_unpack_identity(unsigned char *slot, const char static int keyring_identity_mac(const keyring_identity *id, unsigned char *pkrsalt, unsigned char *mac) { - //assert(id->keypair_count >= 1); unsigned char work[65536]; unsigned ofs = 0; #define APPEND(buf, len) { \ @@ -1036,14 +1039,12 @@ static int keyring_identity_mac(const keyring_identity *id, unsigned char *pkrsa ofs += __len; \ } APPEND(&pkrsalt[0], 32); - if (id->keypairs[0]->type != KEYTYPE_CRYPTOBOX) + if (id->keypair_count == 0 || id->keypairs[0]->type != KEYTYPE_CRYPTOBOX) return WHY("first keypair is not type CRYPTOBOX"); APPEND(id->keypairs[0]->private_key, id->keypairs[0]->private_key_len); APPEND(id->keypairs[0]->public_key, id->keypairs[0]->public_key_len); APPEND(id->PKRPin, strlen(id->PKRPin)); #undef APPEND - if (config.debug.keyring) - dump("work", work, ofs); crypto_hash_sha512(mac, work, ofs); return 0; } @@ -1056,7 +1057,6 @@ static int keyring_identity_mac(const keyring_identity *id, unsigned char *pkrsa */ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, int slot_number) { - int exit_code=1; unsigned char slot[KEYRING_PAGE_SIZE]; keyring_identity *id=NULL; @@ -1108,7 +1108,7 @@ int keyring_decrypt_pkr(keyring_file *k, keyring_context *c, const char *pin, in keyring_free_identity(id); id = NULL; } - return exit_code; + return 1; } /* Try all valid slots with the PIN and see if we find any identities with that PIN. @@ -1121,42 +1121,60 @@ int keyring_enter_pin(keyring_file *k, const char *pin) if (!k) RETURN(-1); if (!pin) pin=""; - int slot; - int identitiesFound=0; + unsigned identitiesFound = 0; - for(slot=0;slotfile_size/KEYRING_PAGE_SIZE;slot++) { - /* slot zero is the BAM and salt, so skip it */ - if (slot&(KEYRING_BAM_BITS-1)) { - /* Not a BAM slot, so examine */ - off_t file_offset=slot*KEYRING_PAGE_SIZE; + // Check if PIN is already entered. + { + unsigned c; + for (c = 0; c < k->context_count; ++c) { + keyring_context *cx = k->contexts[c]; + unsigned i; + for (i = 0; i < cx->identity_count; ++i) { + keyring_identity *id = cx->identities[i]; + if (strcmp(id->PKRPin, pin) == 0) + ++identitiesFound; + } + } + } + // If PIN is already entered, don't enter it again. + if (identitiesFound == 0) { + unsigned slot; + for(slot=0;slotfile_size/KEYRING_PAGE_SIZE;slot++) { + /* slot zero is the BAM and salt, so skip it */ + if (slot&(KEYRING_BAM_BITS-1)) { + /* Not a BAM slot, so examine */ + off_t file_offset=slot*KEYRING_PAGE_SIZE; - /* See if this part of the keyring file is organised */ - keyring_bam *b=k->bam; - while (b&&(file_offset>=b->file_offset+KEYRING_SLAB_SIZE)) - b=b->next; - if (!b) continue; + /* See if this part of the keyring file is organised */ + keyring_bam *b=k->bam; + while (b&&(file_offset>=b->file_offset+KEYRING_SLAB_SIZE)) + b=b->next; + if (!b) continue; - /* Now see if slot is marked in-use. No point checking unallocated slots, - especially since the cost can be upto a second of CPU time on a phone. */ - int position=slot&(KEYRING_BAM_BITS-1); - int byte=position>>3; - int bit=position&7; - if (b->bitmap[byte]&(1<context_count;c++) - if (keyring_decrypt_pkr(k,k->contexts[c],pin?pin:"",slot) == 0) - ++identitiesFound; + /* Now see if slot is marked in-use. No point checking unallocated slots, + especially since the cost can be upto a second of CPU time on a phone. */ + int position=slot&(KEYRING_BAM_BITS-1); + int byte=position>>3; + int bit=position&7; + if (b->bitmap[byte]&(1<context_count;c++) + if (keyring_decrypt_pkr(k,k->contexts[c],pin?pin:"",slot) == 0) + ++identitiesFound; + } } } } /* Tell the caller how many identities we found */ + if (config.debug.keyring) + DEBUGF("identitiesFound=%u", identitiesFound); RETURN(identitiesFound); OUT(); } -static unsigned test_slot(keyring_file *k, unsigned slot) +static unsigned test_slot(const keyring_file *k, unsigned slot) { assert(slot < KEYRING_BAM_BITS); unsigned position = slot & (KEYRING_BAM_BITS - 1); @@ -1180,7 +1198,7 @@ static void set_slot(keyring_file *k, unsigned slot, int bitvalue) /* Find free slot in keyring. Slot 0 in any slab is the BAM and possible keyring salt, so only * search for space in slots 1 and above. TODO: Extend to handle more than one slab! */ -static unsigned find_free_slot(keyring_file *k) +static unsigned find_free_slot(const keyring_file *k) { unsigned slot; for (slot = 1; slot < KEYRING_BAM_BITS; ++slot) @@ -1189,16 +1207,27 @@ static unsigned find_free_slot(keyring_file *k) return 0; } -static void keyring_commit_identity(keyring_file *k, keyring_context *cx, keyring_identity *id) +static unsigned keyring_identity_keypair_sid(const keyring_identity *id) { + unsigned i; + for (i = 0; i < id->keypair_count; ++i) + if (id->keypairs[i]->type == KEYTYPE_CRYPTOBOX) + break; + assert(i < id->keypair_count); + return i; +} + +static int keyring_commit_identity(keyring_file *k, keyring_context *cx, keyring_identity *id) +{ + unsigned keypair_sid = keyring_identity_keypair_sid(id); + unsigned i; + for (i = 0; i < cx->identity_count; ++i) + if (cmp_keypair(cx->identities[i]->keypairs[keyring_identity_keypair_sid(cx->identities[i])], id->keypairs[keypair_sid]) == 0) + return 0; set_slot(k, id->slot, 1); cx->identities[cx->identity_count++] = id; - unsigned keypair_sid; - for (keypair_sid = 0; keypair_sid < id->keypair_count; ++keypair_sid) - if (id->keypairs[keypair_sid]->type == KEYTYPE_CRYPTOBOX) - break; - if (keypair_sid < id->keypair_count) - add_subscriber(id, keypair_sid); + add_subscriber(id, keypair_sid); + return 1; } /* Create a new identity in the specified context (which supplies the keyring pin) with the @@ -1245,10 +1274,9 @@ keyring_identity *keyring_create_identity(keyring_file *k, keyring_context *c, c ++id->keypair_count; } } + assert(id->keypair_count > 0); /* Mark slot as occupied and internalise new identity. */ - assert(id->keypair_count > 0); - assert(id->keypairs[0]->type == KEYTYPE_CRYPTOBOX); keyring_commit_identity(k, c, id); /* Everything went fine */ @@ -1889,8 +1917,6 @@ int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE } if (id == NULL || idn != last_idn) { last_idn = idn; - // TODO: do not commit any identities until entire dump is parsed - // TODO: discard duplicate identities if (id) keyring_commit_identity(k, cx, id); if ((id = emalloc_zero(sizeof(keyring_identity))) == NULL) { @@ -1902,7 +1928,6 @@ int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE keyring_free_identity(id); return -1; } - /* Find free slot in keyring. */ if ((id->slot = find_free_slot(k)) == 0) { keyring_free_keypair(kp); keyring_free_identity(id); @@ -1917,11 +1942,6 @@ int keyring_load(keyring_file *k, int cn, unsigned pinc, const char **pinv, FILE keyring_free_identity(id); return WHY("too many key pairs"); } - /* - fprintf(stderr, "%u: ", id); - keyring_dump_keypair(kp, XPRINTF_STDIO(stderr), 1); - fprintf(stderr, "\n"); - */ } if (id) keyring_commit_identity(k, cx, id); diff --git a/tests/keyring b/tests/keyring index cab96cf2..5b4c27bf 100755 --- a/tests/keyring +++ b/tests/keyring @@ -167,12 +167,86 @@ test_Load() { tfw_cat --stderr executeOk_servald keyring dump --secret dBA tfw_cat dBA + assert [ $(cat dBA | wc -l) -eq $(( $(cat dA | wc -l) + $(cat dB | wc -l) )) ] while read line; do assertGrep --fixed-strings dBA "${line#[0-9]}" done < dA while read line; do assertGrep --fixed-strings dBA "${line#[0-9]}" done < dB + set_instance +A + executeOk_servald keyring load dB + tfw_cat --stderr + executeOk_servald keyring dump --secret dAB + assert cmp dAB dBA +} + +doc_LoadAtomic="Load is atomic: all entries are loaded or none" +setup_LoadAtomic() { + setup_Load + echo blah >>dA +} +test_LoadAtomic() { + set_instance +B + execute --exit-status=255 $servald keyring load dA + executeOk_servald keyring dump --secret dB2 + tfw_cat dB2 + assert cmp dB2 dB +} + +doc_LoadDuplicates="Load de-duplicates keyring entries by SID" +setup_LoadDuplicates() { + setup + executeOk_servald keyring add '' + executeOk_servald keyring dump --secret dA + tfw_cat dA +} +test_LoadDuplicates() { + executeOk_servald keyring load dA + tfw_cat --stderr + executeOk_servald keyring dump --secret dAA + tfw_cat dAA + assert cmp dA dAA +} + +doc_LoadPins="Load keyring entries with PIN arguments" +setup_LoadPins() { + setup_servald + setup_instances +A +B + set_instance +A + executeOk_servald keyring add '' + executeOk_servald keyring add '' + executeOk_servald keyring add '' + executeOk_servald keyring dump --secret dA + set_instance +B + executeOk_servald keyring add '' + executeOk_servald keyring dump --secret dB + set_instance +A + tfw_cat dA dB + assert ! cmp dA dB +} +test_LoadPins() { + set_instance +B + executeOk_servald keyring load dA pin1 '' pin3 + tfw_cat --stderr + for pin in '' pin1 pin3; do + executeOk_servald keyring dump --entry-pin="$pin" --secret dBA + tfw_cat --stderr dBA + let n=0 + while read line; do + case "$pin=$line" in + pin1=0:* | *=1:* | pin3=2:* ) + assertGrep --fixed-strings dBA "${line#[0-9]}" + let ++n + ;; + esac + done < dA + while read line; do + assertGrep --fixed-strings dBA "${line#[0-9]}" + let ++n + done < dB + assert [ $(cat dBA | wc -l) -eq $n ] + done } doc_CompatibleBack1="Can read old keyring file (1)"