dont return index >= ctap_rk_size()

Fixes issue found by @My1: https://github.com/solokeys/solo/issues/407

STABLE_VERSION

@@ -1 +1 @@
-3.2.0
+4.0.0

fido2/ctap.c

@@ -461,6 +461,7 @@ static int ctap_make_extensions(CTAP_extensions * ext, uint8_t * ext_encoder_buf
         // Generate credRandom
         crypto_sha256_hmac_init(CRYPTO_TRANSPORT_KEY2, 0, credRandom);
         crypto_sha256_update((uint8_t*)&ext->hmac_secret.credential->id, sizeof(CredentialId));
+        crypto_sha256_update(&getAssertionState.user_verified, 1);
         crypto_sha256_hmac_final(CRYPTO_TRANSPORT_KEY2, 0, credRandom);
 
         // Decrypt saltEnc
@@ -1034,29 +1035,30 @@ uint8_t ctap_add_user_entity(CborEncoder * map, CTAP_userEntity * user, int is_v
     CborEncoder entity;
     int dispname = (user->name[0] != 0) && is_verified;
     int ret;
+    int map_size = 1;
 
     if (dispname)
-        ret = cbor_encoder_create_map(map, &entity, 4);
+    {
-    else
+        map_size = strlen((const char *)user->icon) > 0 ? 4 : 3;
-        ret = cbor_encoder_create_map(map, &entity, 1);
+    }
+    ret = cbor_encoder_create_map(map, &entity, map_size);
     check_ret(ret);
 
-    {
+    ret = cbor_encode_text_string(&entity, "id", 2);
-        ret = cbor_encode_text_string(&entity, "id", 2);
+    check_ret(ret);
-        check_ret(ret);
 
-        ret = cbor_encode_byte_string(&entity, user->id, user->id_size);
+    ret = cbor_encode_byte_string(&entity, user->id, user->id_size);
-        check_ret(ret);
+    check_ret(ret);
-    }
 
     if (dispname)
     {
-
+        if (strlen((const char *)user->icon) > 0)
-        ret = cbor_encode_text_string(&entity, "icon", 4);
+        {
-        check_ret(ret);
+            ret = cbor_encode_text_string(&entity, "icon", 4);
-
+            check_ret(ret);
-        ret = cbor_encode_text_stringz(&entity, (const char *)user->icon);
+            ret = cbor_encode_text_stringz(&entity, (const char *)user->icon);
-        check_ret(ret);
+            check_ret(ret);
+        }
 
         ret = cbor_encode_text_string(&entity, "name", 4);
         check_ret(ret);
@@ -1374,7 +1376,7 @@ uint8_t ctap_cred_metadata(CborEncoder * encoder)
 uint8_t ctap_cred_rp(CborEncoder * encoder, int rk_ind, int rp_count)
 {
     CTAP_residentKey rk;
-    load_nth_valid_rk(rk_ind, &rk);
+    ctap_load_rk(rk_ind, &rk);
 
     CborEncoder map;
     size_t map_size = rp_count > 0 ? 3 : 2;
@@ -1423,7 +1425,7 @@ uint8_t ctap_cred_rp(CborEncoder * encoder, int rk_ind, int rp_count)
 uint8_t ctap_cred_rk(CborEncoder * encoder, int rk_ind, int rk_count)
 {
     CTAP_residentKey rk;
-    load_nth_valid_rk(rk_ind, &rk);
+    ctap_load_rk(rk_ind, &rk);
 
     uint32_t cred_protect = read_metadata_from_masked_credential(&rk.id);
     if ( cred_protect == 0 || cred_protect > 3 ) 
@@ -1523,62 +1525,106 @@ static int credentialId_to_rk_index(CredentialId * credId){
     return -1;
 }
 
-// Return 1 if Left(rpIdHash, 16) has been counted in rpHashes.
+// Load the next valid resident key of a different rpIdHash
-static int8_t _rk_counted(uint8_t rpHashes [50][16], uint8_t * hash, int unique_count)
+static int scan_for_next_rp(int index){
-{
+    CTAP_residentKey rk;
-    int i = 0;
+    uint8_t nextRpIdHash[32];
-    for (; i < unique_count; i++)
+
+    if (index == -1)
     {
-        if (memcmp(rpHashes[i], hash, 16) == 0) {
+        ctap_load_rk(0, &rk);
-            return 1;
+        if (ctap_rk_is_valid(&rk))
+        {
+            return 0;
+        }
+        else
+        {
+            index = 0;
         }
     }
-    return 0;
-}
 
-static uint8_t count_unique_rks()
+    int occurs_previously;
-{
+    do {
-    CTAP_residentKey rk;
+        occurs_previously = 0;
-    unsigned int unique_count = 0;
-    unsigned int i;
-    uint8_t rpHashes [50][16];
-    memset(rpHashes, 0, sizeof(rpHashes));
 
-    for(i = 0; i < ctap_rk_size(); i++)
+        index++;
-    {
+        if ((unsigned int)index >= ctap_rk_size()) 
-        ctap_load_rk(i, &rk);
-        if ( ctap_rk_is_valid(&rk) )
         {
-            if (! _rk_counted(rpHashes, rk.id.rpIdHash, unique_count)) 
+            return -1;
+        }
+
+        ctap_load_rk(index, &rk);
+        memmove(nextRpIdHash, rk.id.rpIdHash, 32);
+
+        if (!ctap_rk_is_valid(&rk))
+        {
+            occurs_previously = 1;
+            continue;
+        } else {
+        }
+
+        // Check if we have scanned the rpIdHash before.
+        int i;
+        for (i = 0; i < index; i++)
+        {
+            ctap_load_rk(i, &rk);
+            if (memcmp(rk.id.rpIdHash, nextRpIdHash, 32) == 0)
             {
-                memmove(rpHashes[unique_count], rk.id.rpIdHash, 16);
+                occurs_previously = 1;
-                unique_count += 1;
+                break;
-                if (unique_count >= ctap_rk_size())
-                {
-                    return unique_count;
-                }
             }
         }
-    }
+
-    return unique_count;
+    } while (occurs_previously);
+
+    return index;
 }
 
+// Load the next valid resident key of the same rpIdHash
+static int scan_for_next_rk(int index, uint8_t * initialRpIdHash){
+    CTAP_residentKey rk;
+    uint8_t lastRpIdHash[32];
+
+    if (initialRpIdHash != NULL) {
+        memmove(lastRpIdHash, initialRpIdHash, 32);
+        index = -1;
+    }
+    else
+    {
+        ctap_load_rk(index, &rk);
+        memmove(lastRpIdHash, rk.id.rpIdHash, 32);
+    }
+
+    do
+    {
+        index++;
+        if ((unsigned int)index >= ctap_rk_size()) 
+        {
+            return -1;
+        }
+        ctap_load_rk(index, &rk);
+    }
+    while ( memcmp( rk.id.rpIdHash, lastRpIdHash, 32 ) != 0 );
+
+    return index;
+}
+
+
+
 uint8_t ctap_cred_mgmt(CborEncoder * encoder, uint8_t * request, int length)
 {
     CTAP_credMgmt CM;
-    CTAP_residentKey rk;
     int i = 0;
-    // use the same index for both RP and RK commands, it make things simpler
+
+    // RP / RK pointers
+    static int curr_rp_ind = 0;
     static int curr_rk_ind = 0;
-    // keep the rpIdHash specified in CM_cmdRKBegin cause it's not present in CM_cmdRKNext
+
-    static uint8_t rpIdHash[32];
+    // flags that authenticate whether *Begin was before *Next
-    // flag that authenticated RPBegin was received
     static bool rp_auth = false;
-    // flag that authenticated RKBegin was received
     static bool rk_auth = false;
-    // number of stored RPs
+
     int rp_count = 0;
-    // number of RKs with the specified rpIdHash
     int rk_count = 0;
 
     int ret = ctap_parse_cred_mgmt(&CM, request, length);
@@ -1592,78 +1638,51 @@ uint8_t ctap_cred_mgmt(CborEncoder * encoder, uint8_t * request, int length)
     if (STATE.rk_stored == 0 && CM.cmd != CM_cmdMetadata)
     {
         printf2(TAG_ERR,"No resident keys\n");
-        return CTAP2_ERR_NO_CREDENTIALS;
+        return 0;
     }
     if (CM.cmd == CM_cmdRPBegin)
     {
-        curr_rk_ind = 0;
+        curr_rk_ind = -1;
-        rp_count = count_unique_rks();
         rp_auth = true;
         rk_auth = false;
+        curr_rp_ind = scan_for_next_rp(-1);
+
+        // Count total unique RP's
+        while (curr_rp_ind >= 0)
+        {
+            curr_rp_ind = scan_for_next_rp(curr_rp_ind);
+            rp_count++;
+        }
+
+        // Reset scan
+        curr_rp_ind = scan_for_next_rp(-1);
+
+        printf1(TAG_MC, "RP Begin @%d.  %d total.\n", curr_rp_ind, rp_count);
     }
     else if (CM.cmd == CM_cmdRKBegin)
     {
-        curr_rk_ind = 0;
+        curr_rk_ind = scan_for_next_rk(0, CM.subCommandParams.rpIdHash);
         rk_auth = true;
-        rp_auth = false;
+
-        // store the specified hash, we will need it for CM_cmdRKNext
+        // Count total RK's associated to RP
-        memcpy(rpIdHash, CM.subCommandParams.rpIdHash, 32);
+        while (curr_rk_ind >= 0)
-        // count how many RKs have this hash
-        printf1(TAG_GREEN, "There are %d total creds\n", STATE.rk_stored);
-        printf1(TAG_GREEN, "true rpidHash:"); dump_hex1(TAG_GREEN, rpIdHash, 32);
-        for (i = 0; i < STATE.rk_stored; i++)
         {
-            load_nth_valid_rk(i, &rk);
+            curr_rk_ind = scan_for_next_rk(curr_rk_ind, NULL);
-            if (memcmp(rk.id.rpIdHash, rpIdHash, 32) == 0)
+            rk_count++;
-            {
-                rk_count++;
-            }
         }
+
+        // Reset scan
+        curr_rk_ind = scan_for_next_rk(0, CM.subCommandParams.rpIdHash);
+        printf1(TAG_MC, "Cred Begin @%d.  %d total.\n", curr_rk_ind, rk_count);
     }
     else if (CM.cmd != CM_cmdRKNext && CM.cmd != CM_cmdRPNext)
     {
         rk_auth = false;
         rp_auth = false;
-        curr_rk_ind = 0;
+        curr_rk_ind = -1;
+        curr_rp_ind = -1;
     }
 
-    printf1(TAG_GREEN, "(0x%02x) CHECK %d\n", CM.cmd, curr_rk_ind);
-    if (load_nth_valid_rk(curr_rk_ind, &rk) < 0)
-    {
-        printf2(TAG_ERR,"No more resident keys\n");
-        rk_auth = false;
-        rp_auth = false;
-        return CTAP2_ERR_NO_CREDENTIALS;
-    }
-    if (CM.cmd == CM_cmdRPNext && !rp_auth)
-    {
-        printf2(TAG_ERR, "RPNext without RPBegin\n");
-        return CTAP2_ERR_NO_CREDENTIALS;
-    }
-    if (CM.cmd == CM_cmdRKNext && !rk_auth)
-    {
-        printf2(TAG_ERR, "RKNext without RKBegin\n");
-        return CTAP2_ERR_NO_CREDENTIALS;
-    }
-    if (CM.cmd == CM_cmdRKBegin || CM.cmd == CM_cmdRKNext)
-    {
-        load_nth_valid_rk(curr_rk_ind, &rk);
-        // skip resident keys with different rpIdHash
-        while (memcmp(rk.id.rpIdHash, rpIdHash, 32) != 0)
-        {
-            curr_rk_ind++;
-            if (load_nth_valid_rk(curr_rk_ind, &rk) < 0)
-            {
-                break;
-            }
-        }
-        if (curr_rk_ind == STATE.rk_stored)
-        {
-            printf2(TAG_ERR,"No more resident keys with this rpIdHash\n");
-            rk_auth = false;
-            return CTAP2_ERR_NO_CREDENTIALS;
-        }
-    }
     switch (CM.cmd)
     {
         case CM_cmdMetadata:
@@ -1673,17 +1692,32 @@ uint8_t ctap_cred_mgmt(CborEncoder * encoder, uint8_t * request, int length)
             break;
         case CM_cmdRPBegin:
         case CM_cmdRPNext:
-            printf1(TAG_CM, "CM_cmdRPBegin %d/%d\n", curr_rk_ind, rp_count);
+            printf1(TAG_CM, "Get RP %d\n", curr_rp_ind);
-            ret = ctap_cred_rp(encoder, curr_rk_ind, rp_count);
+            if (curr_rp_ind < 0 || !rp_auth) {
+                rp_auth = false;
+                rk_auth = false;
+                return CTAP2_ERR_NO_CREDENTIALS;
+            }
+
+            ret = ctap_cred_rp(encoder, curr_rp_ind, rp_count);
             check_ret(ret);
-            curr_rk_ind++;
+            curr_rp_ind = scan_for_next_rp(curr_rp_ind);
+
             break;
         case CM_cmdRKBegin:
         case CM_cmdRKNext:
-            printf1(TAG_CM, "CM_cmdRKBegin %d/%d\n", curr_rk_ind, rp_count);
+            printf1(TAG_CM, "Get Cred %d\n", curr_rk_ind);
+            if (curr_rk_ind < 0 || !rk_auth) {
+                rp_auth = false;
+                rk_auth = false;
+                return CTAP2_ERR_NO_CREDENTIALS;
+            }
+
             ret = ctap_cred_rk(encoder, curr_rk_ind, rk_count);
             check_ret(ret);
-            curr_rk_ind++;
+
+            curr_rk_ind = scan_for_next_rk(curr_rk_ind, NULL);
+
             break;
         case CM_cmdRKDelete:
             printf1(TAG_CM, "CM_cmdRKDelete\n");
@@ -2192,6 +2226,7 @@ uint8_t ctap_request(uint8_t * pkt_raw, int length, CTAP_RESPONSE * resp)
     {
         case CTAP_MAKE_CREDENTIAL:
         case CTAP_GET_ASSERTION:
+        case CTAP_CBOR_CRED_MGMT:
         case CTAP_CBOR_CRED_MGMT_PRE:
             if (ctap_device_locked())
             {
@@ -2274,6 +2309,7 @@ uint8_t ctap_request(uint8_t * pkt_raw, int length, CTAP_RESPONSE * resp)
                 status = CTAP2_ERR_NOT_ALLOWED;
             }
             break;
+        case CTAP_CBOR_CRED_MGMT:
         case CTAP_CBOR_CRED_MGMT_PRE:
             printf1(TAG_CTAP,"CTAP_CBOR_CRED_MGMT_PRE\n");
             status = ctap_cred_mgmt(&encoder, pkt_raw, length);

fido2/ctap.h

@@ -16,6 +16,7 @@
 #define CTAP_CLIENT_PIN             0x06
 #define CTAP_RESET                  0x07
 #define GET_NEXT_ASSERTION          0x08
+#define CTAP_CBOR_CRED_MGMT         0x0A
 #define CTAP_VENDOR_FIRST           0x40
 #define CTAP_CBOR_CRED_MGMT_PRE     0x41
 #define CTAP_VENDOR_LAST            0xBF

fido2/ctap_parse.c

@@ -1103,7 +1103,7 @@ uint8_t ctap_parse_cred_mgmt(CTAP_credMgmt * CM, uint8_t * request, int length)
     ret = cbor_value_get_map_length(&it, &map_length);
     check_ret(ret);
 
-    printf1(TAG_CM, "CM map has %d elements\n", map_length);
+    printf1(TAG_PARSE, "CM map has %d elements\n", map_length);
 
     for (i = 0; i < map_length; i++)
     {
@@ -1122,7 +1122,7 @@ uint8_t ctap_parse_cred_mgmt(CTAP_credMgmt * CM, uint8_t * request, int length)
         switch(key)
         {
             case CM_cmd:
-                printf1(TAG_CM, "CM_cmd\n");
+                printf1(TAG_PARSE, "CM_cmd\n");
                 if (cbor_value_get_type(&map) == CborIntegerType)
                 {
                     ret = cbor_value_get_int_checked(&map, &CM->cmd);
@@ -1135,12 +1135,12 @@ uint8_t ctap_parse_cred_mgmt(CTAP_credMgmt * CM, uint8_t * request, int length)
                 }
                 break;
             case CM_subCommandParams:
-                printf1(TAG_CM, "CM_subCommandParams\n");
+                printf1(TAG_PARSE, "CM_subCommandParams\n");
                 ret = parse_cred_mgmt_subcommandparams(&map, CM);
                 check_ret(ret);
                 break;
             case CM_pinProtocol:
-                printf1(TAG_CM, "CM_pinProtocol\n");
+                printf1(TAG_PARSE, "CM_pinProtocol\n");
                 if (cbor_value_get_type(&map) == CborIntegerType)
                 {
                     ret = cbor_value_get_int_checked(&map, &CM->pinProtocol);
@@ -1152,7 +1152,7 @@ uint8_t ctap_parse_cred_mgmt(CTAP_credMgmt * CM, uint8_t * request, int length)
                 }
                 break;
             case CM_pinAuth:
-                printf1(TAG_CM, "CM_pinAuth\n");
+                printf1(TAG_PARSE, "CM_pinAuth\n");
                 ret = parse_fixed_byte_string(&map, CM->pinAuth, 16);
                 check_retr(ret);
                 CM->pinAuthPresent = 1;