Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EdDSA/Minisign signatures to #397 (sign-hash/sign-file CTAP command) #583

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
7 changes: 5 additions & 2 deletions fido2/crypto.c
Original file line number Diff line number Diff line change
Expand Up @@ -424,8 +424,11 @@ void crypto_ed25519_sign(uint8_t * data1, int len1, uint8_t * data2, int len2, u
const int len = len1 + len2; // 0 <= len <= 1024
uint8_t data[len1 + len2];

memcpy(data, data1, len1);
memcpy(data + len1, data2, len2);
memcpy(data, data1, len1);
if (len2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check, memcpy with zero length is fine

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally yes, but I call it with data2 == NULL, which would be UB even with len2 == 0. I could call it passing data1 twice, but I think this is cleaner.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the check should be if (data2 != NULL)

Copy link
Author

@stevenwdv stevenwdv Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also possible, but personally I think it looks weird because this would make the following a legal call:

crypto_ed25519_sign(some_data, some_len, NULL, 100, buf)

Although to be fair, with the current conditional the following is legal:

crypto_ed25519_sign(some_data, some_len, (uint8_t*)123, 0, buf)

We could have if (data2 || len2) (or maybe if (data && len2) if we want to be lenient), but in any case it will only matter with invalid parameters.
Or maybe I'm missing an advantage of if (data2)?

{
memcpy(data + len1, data2, len2);
}

#if defined(STM32L432xx)

Expand Down
77 changes: 77 additions & 0 deletions fido2/ctap.c
Original file line number Diff line number Diff line change
Expand Up @@ -1429,6 +1429,76 @@ uint8_t ctap_get_next_assertion(CborEncoder * encoder)
return 0;
}

uint8_t ctap_sign_hash(CborEncoder * encoder, uint8_t * request, int length)
{
CTAP_signHash SH;
CborEncoder map;
uint8_t sigbuf[64];
uint8_t sigder[72];

int ret = ctap_parse_sign_hash(&SH, request, length);
if (ret != 0)
{
printf2(TAG_ERR,"error, ctap_parse_sign_hash failed\n");
return ret;
}
if (ctap_is_pin_set() == 1)
{
ret = verify_pin_auth_ex(SH.pinAuth, SH.hash, SH.hash_len);
check_retr(ret);
}
ret = ctap2_user_presence_test();
check_retr(ret);
ret = cbor_encoder_create_map(encoder, &map, SH.trusted_comment_present ? 2 : 1);
check_ret(ret);

unsigned int cred_size = get_credential_id_size(SH.cred.type);

int32_t cose_alg = read_cose_alg_from_masked_credential(&SH.cred.credential.id);
if (cose_alg == COSE_ALG_EDDSA)
stevenwdv marked this conversation as resolved.
Show resolved Hide resolved
{
crypto_ed25519_load_key((uint8_t*)&SH.cred.credential.id, cred_size);

crypto_ed25519_sign(SH.hash, SH.hash_len, NULL, 0, sigder);

ret = cbor_encode_int(&map, 1);
check_ret(ret);
ret = cbor_encode_byte_string(&map, sigder, 64);
check_ret(ret);

if (SH.trusted_comment_present)
{
crypto_ed25519_sign(sigder, 64, SH.trusted_comment, SH.trusted_comment_len, sigder);

ret = cbor_encode_int(&map, 2);
check_ret(ret);
ret = cbor_encode_byte_string(&map, sigder, 64);
check_ret(ret);
}
}
else
{
if (SH.trusted_comment_present)
{
printf2(TAG_ERR,"error, trusted comment is only supported with EdDSA\n");
return CTAP2_ERR_INVALID_OPTION;
}
crypto_ecc256_load_key((uint8_t*)&SH.cred.credential.id, cred_size, NULL, 0);
crypto_ecc256_sign(SH.hash, SH.hash_len, sigbuf);
stevenwdv marked this conversation as resolved.
Show resolved Hide resolved
int sigder_sz = ctap_encode_der_sig(sigbuf,sigder);
printf1(TAG_SH,"der sig [%d]: ", sigder_sz); dump_hex1(TAG_SH, sigder, sigder_sz);

ret = cbor_encode_int(&map, 1);
check_ret(ret);
ret = cbor_encode_byte_string(&map, sigder, sigder_sz);
check_ret(ret);
}

ret = cbor_encoder_close_container(encoder, &map);
check_ret(ret);
return 0;
}

uint8_t ctap_cred_metadata(CborEncoder * encoder)
{
CborEncoder map;
Expand Down Expand Up @@ -2304,6 +2374,7 @@ uint8_t ctap_request(uint8_t * pkt_raw, int length, CTAP_RESPONSE * resp)
{
case CTAP_MAKE_CREDENTIAL:
case CTAP_GET_ASSERTION:
case CTAP_SOLO_SIGN:
case CTAP_CBOR_CRED_MGMT:
case CTAP_CBOR_CRED_MGMT_PRE:
if (ctap_device_locked())
Expand Down Expand Up @@ -2387,6 +2458,12 @@ uint8_t ctap_request(uint8_t * pkt_raw, int length, CTAP_RESPONSE * resp)
status = CTAP2_ERR_NOT_ALLOWED;
}
break;
case CTAP_SOLO_SIGN:
printf1(TAG_CTAP,"CTAP_SOLO_SIGN\n");
status = ctap_sign_hash(&encoder, pkt_raw, length);
resp->length = cbor_encoder_get_buffer_size(&encoder, buf);
dump_hex1(TAG_DUMP, buf, resp->length);
break;
case CTAP_CBOR_CRED_MGMT:
case CTAP_CBOR_CRED_MGMT_PRE:
printf1(TAG_CTAP,"CTAP_CBOR_CRED_MGMT_PRE\n");
Expand Down
21 changes: 21 additions & 0 deletions fido2/ctap.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define CTAP_CBOR_CRED_MGMT 0x0A
#define CTAP_VENDOR_FIRST 0x40
#define CTAP_CBOR_CRED_MGMT_PRE 0x41
#define CTAP_SOLO_SIGN 0x50
#define CTAP_VENDOR_LAST 0xBF

#define MC_clientDataHash 0x01
Expand All @@ -39,6 +40,10 @@
#define GA_pinAuth 0x06
#define GA_pinProtocol 0x07

#define SH_hash 0x01
#define SH_credential 0x02
#define SH_pinAuth 0x03
#define SH_trustedComment 0x04
#define CM_cmd 0x01
#define CM_cmdMetadata 0x01
#define CM_cmdRPBegin 0x02
Expand Down Expand Up @@ -124,6 +129,8 @@
#define DISPLAY_NAME_LIMIT 32 // Must be minimum of 64 bytes but can be more.
#define ICON_LIMIT 128 // Must be minimum of 64 bytes but can be more.
#define CTAP_MAX_MESSAGE_SIZE 1200
#define SIGN_HASH_HASH_MAX_SIZE 64 //up to 512 bits
#define SIGN_HASH_TRUSTED_COMMENT_MAX_SIZE 128

#define CREDENTIAL_RK_FLASH_PAD 2 // size of RK should be 8-byte aligned to store in flash easily.
#define CREDENTIAL_TAG_SIZE 16
Expand Down Expand Up @@ -322,6 +329,20 @@ typedef struct

} CTAP_getAssertion;

typedef struct
{
uint8_t pinAuth[16];

uint8_t hash[SIGN_HASH_HASH_MAX_SIZE];
uint8_t hash_len;

uint8_t trusted_comment[SIGN_HASH_TRUSTED_COMMENT_MAX_SIZE];
uint8_t trusted_comment_len;
uint8_t trusted_comment_present;

CTAP_credentialDescriptor cred;
} CTAP_signHash;

typedef struct
{
int cmd;
Expand Down
105 changes: 105 additions & 0 deletions fido2/ctap_parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,111 @@ uint8_t parse_allow_list(CTAP_getAssertion * GA, CborValue * it)
return 0;
}

uint8_t ctap_parse_sign_hash(CTAP_signHash * SH, uint8_t * request, int length)
{
int key;
size_t i, map_length;
CborParser parser;
CborValue it,map;

memset(SH, 0, sizeof(CTAP_signHash));
int ret = cbor_parser_init(request, length, CborValidateCanonicalFormat, &parser, &it);
check_ret(ret);

CborType type = cbor_value_get_type(&it);
if (type != CborMapType)
{
printf2(TAG_ERR,"Error, expecting cbor map\n");
return CTAP2_ERR_INVALID_CBOR_TYPE;
}

ret = cbor_value_enter_container(&it,&map);
check_ret(ret);

ret = cbor_value_get_map_length(&it, &map_length);
check_ret(ret);

printf1(TAG_SH, "SH map has %d elements\n", map_length);

for (i = 0; i < map_length; i++)
{
type = cbor_value_get_type(&map);
if (type != CborIntegerType)
{
printf2(TAG_ERR,"Error, expecting int for map key\n");
return CTAP2_ERR_INVALID_CBOR_TYPE;
}
ret = cbor_value_get_int_checked(&map, &key);
check_ret(ret);

ret = cbor_value_advance(&map);
check_ret(ret);

switch(key)
{
case SH_hash:
printf1(TAG_SH, "SH_hash\n");

if (cbor_value_get_type(&map) == CborByteStringType)
{
size_t hash_len = SIGN_HASH_HASH_MAX_SIZE;
ret = cbor_value_copy_byte_string(&map, SH->hash, &hash_len, NULL);
if ((hash_len != 32 && hash_len != 64) || ret == CborErrorOutOfMemory)
{
printf2(TAG_ERR,"Error, invalid hash length, should be 32/64 B\n");
return CTAP1_ERR_INVALID_LENGTH;
}
check_ret(ret);
SH->hash_len = hash_len;
}
else
{
printf2(TAG_ERR, "error, CborByteStringType expected for hash\r\n");
return CTAP2_ERR_INVALID_CBOR_TYPE;
}

printf1(TAG_SH," "); dump_hex1(TAG_SH, SH->hash, SH->hash_len);
break;

case SH_trustedComment:
printf1(TAG_SH, "SH_trustedComment\n");
if (cbor_value_get_type(&map) == CborByteStringType)
{
size_t trusted_comment_len = SIGN_HASH_TRUSTED_COMMENT_MAX_SIZE;
ret = cbor_value_copy_byte_string(&map, SH->trusted_comment, &trusted_comment_len, NULL);
if (ret == CborErrorOutOfMemory)
{
printf2(TAG_ERR,"Error, trusted comment too large\n");
return CTAP2_ERR_LIMIT_EXCEEDED;
}
check_ret(ret);
SH->trusted_comment_len = trusted_comment_len;
SH->trusted_comment_present = 1;
}
else
{
printf2(TAG_ERR, "error, CborByteStringType expected for trusted comment\r\n");
return CTAP2_ERR_INVALID_CBOR_TYPE;
}
break;

case SH_credential:
printf1(TAG_SH, "SH_credential\n");
ret = parse_credential_descriptor(&map, &SH->cred);
check_ret(ret);
break;
case SH_pinAuth:
printf1(TAG_SH, "SH_pinAuth\n");
ret = parse_fixed_byte_string(&map, SH->pinAuth, 16);
check_retr(ret);
break;
}
ret = cbor_value_advance(&map);
check_ret(ret);
}
return 0;
}

static uint8_t parse_cred_mgmt_subcommandparams(CborValue * val, CTAP_credMgmt * CM)
{
size_t map_length;
Expand Down
1 change: 1 addition & 0 deletions fido2/ctap_parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ uint8_t parse_cose_key(CborValue * it, COSE_key * cose);

uint8_t ctap_parse_make_credential(CTAP_makeCredential * MC, CborEncoder * encoder, uint8_t * request, int length);
uint8_t ctap_parse_get_assertion(CTAP_getAssertion * GA, uint8_t * request, int length);
uint8_t ctap_parse_sign_hash(CTAP_signHash * SH, uint8_t * request, int length);
uint8_t ctap_parse_cred_mgmt(CTAP_credMgmt * CM, uint8_t * request, int length);
uint8_t ctap_parse_client_pin(CTAP_clientPin * CP, uint8_t * request, int length);
uint8_t parse_credential_descriptor(CborValue * arr, CTAP_credentialDescriptor * cred);
Expand Down
1 change: 1 addition & 0 deletions fido2/log.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ struct logtag tagtable[] = {
{TAG_NFC_APDU, "NAPDU"},
{TAG_CCID, "CCID"},
{TAG_CM, "CRED_MGMT"},
{TAG_SH, "SH"},
};


Expand Down
1 change: 1 addition & 0 deletions fido2/log.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ typedef enum
TAG_NFC_APDU = (1 << 20),
TAG_CCID = (1 << 21),
TAG_CM = (1 << 22),
TAG_SH = (1 << 23),

TAG_NO_TAG = (1UL << 30),
TAG_FILENO = (1UL << 31)
Expand Down