ExternalVendorCode/serval-dna
1
0
Fork 0
mirror of https://github.com/servalproject/serval-dna.git synced 2025-02-06 11:09:13 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix keyring SEGV bug

Browse Source 
Sometimes, when adding a PIN to a keyring (eg, when opening a keyring
file), a SEGV would be caused while trying to validate the MAC for an
entry which apparently had zero keypair entries.

Changed some keyring struct 'int' fields to 'unsigned int', to ensure
that comparison logic behaves as expected.

Refactored some keyring code for more clarity and code maintenance
safety.

Added TODO comment about keyring file format non-back-compatibility.
This commit is contained in:
Andrew Bettison 2013-03-07 14:27:33 +10:30
parent 4ea748c5e2
commit b7185a294f
2 changed files with 62 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

105
keyring.c
View File

@ -347,10 +347,11 @@ int keyring_munge_block(unsigned char *block,int len /* includes the first 96 by
#endif #endif
/* Generate key and nonce hashes from the various inputs */ /* Generate key and nonce hashes from the various inputs */
int ofs; unsigned ofs;
#define APPEND(buf, len) { \ #define APPEND(buf, len) { \
assert(ofs <= sizeof work); \
unsigned __len = (len); \ unsigned __len = (len); \
if (ofs + __len >= sizeof work) { \ if (__len > sizeof work - ofs) { \
WHY("Input too long"); \ WHY("Input too long"); \
goto kmb_safeexit; \ goto kmb_safeexit; \
} \ } \
@ -395,7 +396,7 @@ int keyring_munge_block(unsigned char *block,int len /* includes the first 96 by
int keyring_pack_identity(keyring_context *c,keyring_identity *i, int keyring_pack_identity(keyring_context *c,keyring_identity *i,
unsigned char packed[KEYRING_PAGE_SIZE]) unsigned char packed[KEYRING_PAGE_SIZE])
{ {
int ofs=0; unsigned ofs=0;
int exit_code=-1; int exit_code=-1;
/* Convert an identity to a KEYRING_PAGE_SIZE bytes long block that /* Convert an identity to a KEYRING_PAGE_SIZE bytes long block that
@ -530,7 +531,7 @@ keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin)
{ {
/* Skip salt and MAC */ /* Skip salt and MAC */
int i; int i;
int ofs; unsigned ofs;
keyring_identity *id = emalloc_zero(sizeof(keyring_identity)); keyring_identity *id = emalloc_zero(sizeof(keyring_identity));
if (!id) { WHY("malloc of identity failed"); return NULL; } if (!id) { WHY("malloc of identity failed"); return NULL; }
if (!slot) { WHY("slot is null"); return NULL; } if (!slot) { WHY("slot is null"); return NULL; }
@ -571,8 +572,14 @@ keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin)
keyring_free_identity(id); keyring_free_identity(id);
return NULL; return NULL;
} }
kp->type=slot_byte(ofs); kp->type = slot_byte(ofs++);
switch(kp->type) { /* TODO The keyring format is not back-compatible, ie, old software cannot cope with a
* keyring from newer software that stores new key types. The simple solution to this is to
* encode the length in the type byte, or add an extra length byte, using a representation
* that is not extensible, ie, is not subject to change. This will allow the software to
* skip unrecognised key types and pick up the ones it recognises.
*/
switch (kp->type) {
case KEYTYPE_CRYPTOBOX: case KEYTYPE_CRYPTOBOX:
kp->private_key_len=crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES; kp->private_key_len=crypto_box_curve25519xsalsa20poly1305_SECRETKEYBYTES;
kp->public_key_len=crypto_box_curve25519xsalsa20poly1305_PUBLICKEYBYTES; kp->public_key_len=crypto_box_curve25519xsalsa20poly1305_PUBLICKEYBYTES;
@ -581,28 +588,23 @@ keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin)
kp->private_key_len=crypto_sign_edwards25519sha512batch_SECRETKEYBYTES; kp->private_key_len=crypto_sign_edwards25519sha512batch_SECRETKEYBYTES;
kp->public_key_len=crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES; kp->public_key_len=crypto_sign_edwards25519sha512batch_PUBLICKEYBYTES;
break; break;
case KEYTYPE_RHIZOME: case KEYTYPE_RHIZOME:
kp->private_key_len=32; kp->public_key_len=0; kp->private_key_len=32; kp->public_key_len=0;
break; break;
case KEYTYPE_DID: case KEYTYPE_DID:
kp->private_key_len=32; kp->public_key_len=64; kp->private_key_len=32; kp->public_key_len=64;
break; break;
} }
kp->private_key = emalloc(kp->private_key_len); if (kp->private_key_len && (kp->private_key = emalloc(kp->private_key_len)) == NULL) {
if (!kp->private_key) {
WHY("malloc of private key storage failed");
keyring_free_identity(id); keyring_free_identity(id);
return NULL; return NULL;
} }
for(i=0;i<kp->private_key_len;i++) kp->private_key[i]=slot_byte(ofs+1+i); for (i = 0; i < kp->private_key_len; ++i)
kp->public_key = emalloc(kp->public_key_len); kp->private_key[i] = slot_byte(ofs++);
if (!kp->public_key) { if (kp->public_key_len && (kp->public_key = emalloc(kp->public_key_len)) == NULL) {
WHY("malloc of public key storage failed");
keyring_free_identity(id); keyring_free_identity(id);
return NULL; return NULL;
} }
/* Hop over the private key and token that we have read */
ofs+=1+kp->private_key_len;
switch(kp->type) { switch(kp->type) {
case KEYTYPE_CRYPTOBOX: case KEYTYPE_CRYPTOBOX:
/* Compute public key from private key. /* Compute public key from private key.
@ -629,10 +631,10 @@ keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin)
So we just copy it out. We use the same code for extracting the So we just copy it out. We use the same code for extracting the
public key for a DID (i.e, subscriber name) public key for a DID (i.e, subscriber name)
*/ */
for(i=0;i<kp->public_key_len;i++) kp->public_key[i]=slot_byte(ofs+i); for (i = 0; i < kp->public_key_len; ++i)
ofs+=kp->public_key_len; kp->public_key[i] = slot_byte(ofs++);
break; break;
case KEYTYPE_RHIZOME: case KEYTYPE_RHIZOME:
/* no public key value for these, just do nothing */ /* no public key value for these, just do nothing */
break; break;
} }
@ -642,22 +644,23 @@ keyring_identity *keyring_unpack_identity(unsigned char *slot, const char *pin)
/* Invalid data, so invalid record. Free and return failure. /* Invalid data, so invalid record. Free and return failure.
We don't complain about this, however, as it is the natural We don't complain about this, however, as it is the natural
effect of trying a pin on an incorrect keyring slot. */ effect of trying a pin on an incorrect keyring slot. */
keyring_free_identity(id); keyring_free_identity(id);
return NULL; return NULL;
} }
} }
return id; return id;
} }
int keyring_identity_mac(keyring_context *c,keyring_identity *id, int keyring_identity_mac(keyring_context *c, keyring_identity *id,
unsigned char *pkrsalt,unsigned char *mac) unsigned char *pkrsalt,unsigned char *mac)
{ {
//assert(id->keypair_count >= 1);
unsigned char work[65536]; unsigned char work[65536];
int ofs = 0; unsigned ofs = 0;
#define APPEND(buf, len) { \ #define APPEND(buf, len) { \
assert(ofs <= sizeof work); \
unsigned __len = (len); \ unsigned __len = (len); \
if (ofs + __len >= sizeof work) { \ if (__len > sizeof work - ofs) { \
bzero(work, ofs); \ bzero(work, ofs); \
return WHY("Input too long"); \ return WHY("Input too long"); \
} \ } \
@ -685,7 +688,7 @@ int keyring_decrypt_pkr(keyring_file *k,keyring_context *c,
int exit_code=1; int exit_code=1;
unsigned char slot[KEYRING_PAGE_SIZE]; unsigned char slot[KEYRING_PAGE_SIZE];
unsigned char hash[crypto_hash_sha512_BYTES]; unsigned char hash[crypto_hash_sha512_BYTES];
keyring_identity *id=NULL; keyring_identity *id=NULL;
/* 1. Read slot. */ /* 1. Read slot. */
if (fseeko(k->file,slot_number*KEYRING_PAGE_SIZE,SEEK_SET)) if (fseeko(k->file,slot_number*KEYRING_PAGE_SIZE,SEEK_SET))
@ -699,16 +702,14 @@ int keyring_decrypt_pkr(keyring_file *k,keyring_context *c,
c->KeyRingPin,pin)) { c->KeyRingPin,pin)) {
WHY("keyring_munge_block() failed"); WHY("keyring_munge_block() failed");
goto kdp_safeexit; goto kdp_safeexit;
}
/* 3. Unpack contents of slot into a new identity in the provided context. */
id=keyring_unpack_identity(slot,pin);
if (!id) {
// Don't complain, because this happens routinely when trying pins against slots.
// WHY("keyring_unpack_identity() failed");
goto kdp_safeexit;
} }
id->slot=slot_number;
/* 3. Unpack contents of slot into a new identity in the provided context. */
if ((id = keyring_unpack_identity(slot, pin)) == NULL)
goto kdp_safeexit; // Not a valid slot
if (id->keypair_count < 1)
goto kdp_safeexit; // Not a valid slot
id->slot = slot_number;
/* 4. Verify that slot is self-consistent (check MAC) */ /* 4. Verify that slot is self-consistent (check MAC) */
if (keyring_identity_mac(k->contexts[0],id,&slot[0],hash)) { if (keyring_identity_mac(k->contexts[0],id,&slot[0],hash)) {
@ -717,7 +718,7 @@ int keyring_decrypt_pkr(keyring_file *k,keyring_context *c,
} }
/* compare hash to record */ /* compare hash to record */
if (memcmp(hash,&slot[32],crypto_hash_sha512_BYTES)) if (memcmp(hash,&slot[32],crypto_hash_sha512_BYTES))
{ {
WHY("Slot is not valid (MAC mismatch)"); WHY("Slot is not valid (MAC mismatch)");
dump("computed",hash,crypto_hash_sha512_BYTES); dump("computed",hash,crypto_hash_sha512_BYTES);
dump("stored",&slot[32],crypto_hash_sha512_BYTES); dump("stored",&slot[32],crypto_hash_sha512_BYTES);
@ -749,7 +750,10 @@ int keyring_decrypt_pkr(keyring_file *k,keyring_context *c,
/* Clean up any potentially sensitive data before exiting */ /* Clean up any potentially sensitive data before exiting */
bzero(slot,KEYRING_PAGE_SIZE); bzero(slot,KEYRING_PAGE_SIZE);
bzero(hash,crypto_hash_sha512_BYTES); bzero(hash,crypto_hash_sha512_BYTES);
if (id) keyring_free_identity(id); id=NULL; if (id) {
keyring_free_identity(id);
id = NULL;
}
return exit_code; return exit_code;
} }
@ -812,6 +816,8 @@ int keyring_enter_pin(keyring_file *k, const char *pin)
*/ */
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);
/* Check obvious abort conditions early */ /* Check obvious abort conditions early */
if (!k) { WHY("keyring is NULL"); return NULL; } if (!k) { WHY("keyring is NULL"); return NULL; }
if (!k->bam) { WHY("keyring lacks BAM (not to be confused with KAPOW)"); return NULL; } if (!k->bam) { WHY("keyring lacks BAM (not to be confused with KAPOW)"); return NULL; }
@ -954,8 +960,7 @@ int keyring_commit(keyring_file *k)
DEBUGF("k=%p", k); DEBUGF("k=%p", k);
if (!k) return WHY("keyring was NULL"); if (!k) return WHY("keyring was NULL");
if (k->context_count<1) return WHY("Keyring has no contexts"); if (k->context_count<1) return WHY("Keyring has no contexts");
unsigned errorCount=0; unsigned errorCount = 0;
/* Write all BAMs */ /* Write all BAMs */
keyring_bam *b; keyring_bam *b;
for (b = k->bam; b; b = b->next) { for (b = k->bam; b; b = b->next) {
@ -970,7 +975,6 @@ int keyring_commit(keyring_file *k)
errorCount++; errorCount++;
} }
} }
/* For each identity in each context, write the record to disk. /* 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 for each identity and context is used, so changing a keypair or pin
@ -1012,7 +1016,10 @@ int keyring_commit(keyring_file *k)
} }
} }
} }
if (fflush(k->file) == -1) {
WHYF_perror("fflush(%d)", fileno(k->file));
errorCount++;
}
return errorCount ? WHYF("%u errors commiting keyring to disk", errorCount) : 0; return errorCount ? WHYF("%u errors commiting keyring to disk", errorCount) : 0;
} }
@ -1163,9 +1170,8 @@ unsigned char *keyring_find_sas_private(keyring_file *k,unsigned char *sid,
{ {
/* SAS key is invalid (perhaps because it was a pre 0.90 format one), /* SAS key is invalid (perhaps because it was a pre 0.90 format one),
so replace it */ so replace it */
DEBUGF("SAS key is invalid -- regenerating."); WARN("SAS key is invalid -- regenerating.");
crypto_sign_edwards25519sha512batch_keypair(sas_public, crypto_sign_edwards25519sha512batch_keypair(sas_public, sas_private);
sas_private);
keyring_commit(k); keyring_commit(k);
} }
if (config.debug.keyring) if (config.debug.keyring)
@ -1405,7 +1411,6 @@ int keyring_seed(keyring_file *k)
/* nothing to do if there is already an identity */ /* nothing to do if there is already an identity */
if (k->contexts[0]->identity_count) if (k->contexts[0]->identity_count)
return 0; return 0;
int i; int i;
char did[65]; char did[65];
/* Securely generate random telephone number */ /* Securely generate random telephone number */
@ -1415,11 +1420,21 @@ int keyring_seed(keyring_file *k)
did[0]='2'+(((unsigned char)did[0])%8); did[0]='2'+(((unsigned char)did[0])%8);
/* Then add 10 more digits, which is what we do in the mobile phone software */ /* Then add 10 more digits, which is what we do in the mobile phone software */
for(i=1;i<11;i++) did[i]='0'+(((unsigned char)did[i])%10); did[11]=0; for(i=1;i<11;i++) did[i]='0'+(((unsigned char)did[i])%10); did[11]=0;
keyring_identity *id=keyring_create_identity(k,k->contexts[0],""); keyring_identity *id=keyring_create_identity(k,k->contexts[0],"");
if (!id) return WHY("Could not create new identity"); if (!id) return WHY("Could not create new identity");
if (keyring_set_did(id, did, "")) return WHY("Could not set DID of new identity"); if (keyring_set_did(id, did, "")) return WHY("Could not set DID of new identity");
if (keyring_commit(k)) return WHY("Could not commit new identity to keyring file"); if (keyring_commit(k)) return WHY("Could not commit new identity to keyring file");
{
const unsigned char *sid_binary = NULL;
const char *did = NULL;
const char *name = NULL;
keyring_identity_extract(id, &sid_binary, &did, &name);
INFOF("Seeded keyring with identity: did=%s name=%s sid=%s",
did ? did : "(null)",
alloca_str_toprint(name),
sid_binary ? alloca_tohex_sid(sid_binary) : "(null)"
);
}
return 0; return 0;
} }

5
serval.h
View File

@ -210,7 +210,7 @@ typedef struct keyring_identity {
char *PKRPin; char *PKRPin;
struct subscriber *subscriber; struct subscriber *subscriber;
unsigned int slot; unsigned int slot;
int keypair_count; unsigned int keypair_count;
keypair *keypairs[PKR_MAX_KEYPAIRS]; keypair *keypairs[PKR_MAX_KEYPAIRS];
} keyring_identity; } keyring_identity;
@ -223,8 +223,7 @@ typedef struct keyring_context {
char *KeyRingPin; char *KeyRingPin;
unsigned char *KeyRingSalt; unsigned char *KeyRingSalt;
int KeyRingSaltLen; int KeyRingSaltLen;
unsigned int identity_count;
int identity_count;
keyring_identity *identities[KEYRING_MAX_IDENTITIES]; keyring_identity *identities[KEYRING_MAX_IDENTITIES];
} keyring_context; } keyring_context;