Merge pull request #171 from solokeys/fix_cbor

Fix cbor

fido2/ctap.c

@@ -117,41 +117,12 @@ uint8_t ctap_get_info(CborEncoder * encoder)
             check_ret(ret);
         }
 
-        ret = cbor_encode_uint(&map, RESP_maxMsgSize);
-        check_ret(ret);
-        {
-            ret = cbor_encode_int(&map, CTAP_MAX_MESSAGE_SIZE);
-            check_ret(ret);
-        }
-
-        ret = cbor_encode_uint(&map, RESP_pinProtocols);
-        check_ret(ret);
-        {
-            ret = cbor_encoder_create_array(&map, &pins, 1);
-            check_ret(ret);
-            {
-                ret = cbor_encode_int(&pins, 1);
-                check_ret(ret);
-            }
-            ret = cbor_encoder_close_container(&map, &pins);
-            check_ret(ret);
-        }
-
-
-
         ret = cbor_encode_uint(&map, RESP_options);
         check_ret(ret);
         {
             ret = cbor_encoder_create_map(&map, &options,4);
             check_ret(ret);
             {
-                ret = cbor_encode_text_string(&options, "plat", 4);
-                check_ret(ret);
-                {
-                    ret = cbor_encode_boolean(&options, 0);     // Not attached to platform
-                    check_ret(ret);
-                }
-
                 ret = cbor_encode_text_string(&options, "rk", 2);
                 check_ret(ret);
                 {
@@ -175,6 +146,15 @@ uint8_t ctap_get_info(CborEncoder * encoder)
                 //     ret = cbor_encode_boolean(&options, 0);
                 //     check_ret(ret);
                 // }
+
+                ret = cbor_encode_text_string(&options, "plat", 4);
+                check_ret(ret);
+                {
+                    ret = cbor_encode_boolean(&options, 0);     // Not attached to platform
+                    check_ret(ret);
+                }
+
+
                 ret = cbor_encode_text_string(&options, "clientPin", 9);
                 check_ret(ret);
                 {
@@ -188,6 +168,30 @@ uint8_t ctap_get_info(CborEncoder * encoder)
             check_ret(ret);
         }
 
+        ret = cbor_encode_uint(&map, RESP_maxMsgSize);
+        check_ret(ret);
+        {
+            ret = cbor_encode_int(&map, CTAP_MAX_MESSAGE_SIZE);
+            check_ret(ret);
+        }
+
+        ret = cbor_encode_uint(&map, RESP_pinProtocols);
+        check_ret(ret);
+        {
+            ret = cbor_encoder_create_array(&map, &pins, 1);
+            check_ret(ret);
+            {
+                ret = cbor_encode_int(&pins, 1);
+                check_ret(ret);
+            }
+            ret = cbor_encoder_close_container(&map, &pins);
+            check_ret(ret);
+        }
+
+
+
+
+
 
     }
     ret = cbor_encoder_close_container(encoder, &map);
@@ -730,6 +734,14 @@ uint8_t ctap_make_credential(CborEncoder * encoder, uint8_t * request, int lengt
     CborEncoder map;
     ret = cbor_encoder_create_map(encoder, &map, 3);
     check_ret(ret);
+
+    {
+        ret = cbor_encode_int(&map,RESP_fmt);
+        check_ret(ret);
+        ret = cbor_encode_text_stringz(&map, "packed");
+        check_ret(ret);
+    }
+
     uint32_t auth_data_sz = sizeof(auth_data_buf);
 
     ret = ctap_make_auth_data(&MC.rp, &map, auth_data_buf, &auth_data_sz,
@@ -763,13 +775,6 @@ uint8_t ctap_make_credential(CborEncoder * encoder, uint8_t * request, int lengt
     ret = ctap_add_attest_statement(&map, sigder, sigder_sz);
     check_retr(ret);
 
-    {
-        ret = cbor_encode_int(&map,RESP_fmt);
-        check_ret(ret);
-        ret = cbor_encode_text_stringz(&map, "packed");
-        check_ret(ret);
-    }
-
     ret = cbor_encoder_close_container(encoder, &map);
     check_ret(ret);
     return CTAP1_ERR_SUCCESS;
@@ -797,13 +802,6 @@ static uint8_t ctap_add_credential_descriptor(CborEncoder * map, CTAP_credential
     ret = cbor_encoder_create_map(map, &desc, 2);
     check_ret(ret);
 
-    {
-        ret = cbor_encode_text_string(&desc, "type", 4);
-        check_ret(ret);
-
-        ret = cbor_encode_text_string(&desc, "public-key", 10);
-        check_ret(ret);
-    }
     {
         ret = cbor_encode_text_string(&desc, "id", 2);
         check_ret(ret);
@@ -812,6 +810,15 @@ static uint8_t ctap_add_credential_descriptor(CborEncoder * map, CTAP_credential
         check_ret(ret);
     }
 
+    {
+        ret = cbor_encode_text_string(&desc, "type", 4);
+        check_ret(ret);
+
+        ret = cbor_encode_text_string(&desc, "public-key", 10);
+        check_ret(ret);
+    }
+
+
     ret = cbor_encoder_close_container(map, &desc);
     check_ret(ret);
 
@@ -843,6 +850,13 @@ uint8_t ctap_add_user_entity(CborEncoder * map, CTAP_userEntity * user)
 
     if (dispname)
     {
+
+        ret = cbor_encode_text_string(&entity, "icon", 4);
+        check_ret(ret);
+
+        ret = cbor_encode_text_stringz(&entity, (const char *)user->icon);
+        check_ret(ret);
+
         ret = cbor_encode_text_string(&entity, "name", 4);
         check_ret(ret);
 
@@ -855,13 +869,6 @@ uint8_t ctap_add_user_entity(CborEncoder * map, CTAP_userEntity * user)
         ret = cbor_encode_text_stringz(&entity, (const char *)user->displayName);
         check_ret(ret);
 
-        ret = cbor_encode_text_string(&entity, "icon", 4);
-        check_ret(ret);
-
-        ret = cbor_encode_text_stringz(&entity, (const char *)user->icon);
-        check_ret(ret);
-
-
     }
 
     ret = cbor_encoder_close_container(map, &entity);
@@ -997,22 +1004,22 @@ static CTAP_credentialDescriptor * pop_credential()
 }
 
 // adds 2 to map, or 3 if add_user is true
-uint8_t ctap_end_get_assertion(CborEncoder * map, CTAP_credentialDescriptor * cred, uint8_t * auth_data_buf, uint8_t * clientDataHash, int add_user)
+uint8_t ctap_end_get_assertion(CborEncoder * map, CTAP_credentialDescriptor * cred, uint8_t * auth_data_buf, unsigned int auth_data_buf_sz, uint8_t * clientDataHash)
 {
     int ret;
     uint8_t sigbuf[64];
     uint8_t sigder[72];
     int sigder_sz;
 
-    if (add_user)
+    ret = ctap_add_credential_descriptor(map, cred);  // 1
-    {
-        printf1(TAG_GREEN, "adding user details to output\r\n");
-        ret = ctap_add_user_entity(map, &cred->credential.user);
     check_retr(ret);
-    }
 
-    ret = ctap_add_credential_descriptor(map, cred);
+    {
-    check_retr(ret);
+        ret = cbor_encode_int(map,RESP_authData);  // 2
+        check_ret(ret);
+        ret = cbor_encode_byte_string(map, auth_data_buf, auth_data_buf_sz);
+        check_ret(ret);
+    }
 
     crypto_ecc256_load_key((uint8_t*)&cred->credential.id, sizeof(CredentialId), NULL, 0);
 
@@ -1028,11 +1035,20 @@ uint8_t ctap_end_get_assertion(CborEncoder * map, CTAP_credentialDescriptor * cr
     }
 
     {
-        ret = cbor_encode_int(map, RESP_signature);
+        ret = cbor_encode_int(map, RESP_signature);  // 3
         check_ret(ret);
         ret = cbor_encode_byte_string(map, sigder, sigder_sz);
         check_ret(ret);
     }
+
+    if (cred->credential.user.id_size)
+    {
+        printf1(TAG_GREEN, "adding user details to output\r\n");
+        ret = ctap_add_user_entity(map, &cred->credential.user);  // 4
+        check_retr(ret);
+    }
+
+
     return 0;
 }
 
@@ -1051,9 +1067,8 @@ uint8_t ctap_get_next_assertion(CborEncoder * encoder)
     }
 
     auth_data_update_count(&authData);
-    int add_user_info = cred->credential.user.id_size;
 
-    if (add_user_info)
+    if (cred->credential.user.id_size)
     {
         printf1(TAG_GREEN, "adding user info to assertion response\r\n");
         ret = cbor_encoder_create_map(encoder, &map, 4);
@@ -1064,18 +1079,9 @@ uint8_t ctap_get_next_assertion(CborEncoder * encoder)
         ret = cbor_encoder_create_map(encoder, &map, 3);
     }
 
-
     check_ret(ret);
     printf1(TAG_RED, "RPID hash: "); dump_hex1(TAG_RED, authData.rpIdHash, 32);
 
-    {
-        ret = cbor_encode_int(&map,RESP_authData);
-        check_ret(ret);
-        ret = cbor_encode_byte_string(&map, (uint8_t *)&authData, sizeof(CTAP_authDataHeader));
-        check_ret(ret);
-    }
-
-
     // if only one account for this RP, null out the user details
     if (!getAssertionState.user_verified)
     {
@@ -1083,8 +1089,7 @@ uint8_t ctap_get_next_assertion(CborEncoder * encoder)
         memset(cred->credential.user.name, 0, USER_NAME_LIMIT);
     }
 
-
+    ret = ctap_end_get_assertion(&map, cred, (uint8_t *)&authData, sizeof(CTAP_authDataHeader), getAssertionState.clientDataHash);
-    ret = ctap_end_get_assertion(&map, cred, (uint8_t *)&authData, getAssertionState.clientDataHash, add_user_info);
     check_retr(ret);
 
     ret = cbor_encoder_close_container(encoder, &map);
@@ -1127,13 +1132,12 @@ uint8_t ctap_get_assertion(CborEncoder * encoder, uint8_t * request, int length)
     printf1(TAG_GA, "ALLOW_LIST has %d creds\n", GA.credLen);
     int validCredCount = ctap_filter_invalid_credentials(&GA);
 
-    int add_user_info = GA.creds[validCredCount - 1].credential.user.id_size;
     if (validCredCount > 1)
     {
        map_size += 1;
     }
 
-    if (add_user_info)
+    if (GA.creds[validCredCount - 1].credential.user.id_size)
     {
         map_size += 1;
     }
@@ -1166,63 +1170,60 @@ uint8_t ctap_get_assertion(CborEncoder * encoder, uint8_t * request, int length)
         printf1(TAG_GA,"CRED ID (# %d)\n", GA.creds[j].credential.id.count);
     }
 
-    if (validCredCount > 1)
+
-    {
-        ret = cbor_encode_int(&map, RESP_numberOfCredentials);
-        check_ret(ret);
-        ret = cbor_encode_int(&map, validCredCount);
-        check_ret(ret);
-    }
 
     CTAP_credentialDescriptor * cred = &GA.creds[validCredCount - 1];
 
     GA.extensions.hmac_secret.credential = &cred->credential;
 
+    uint32_t auth_data_buf_sz = sizeof(auth_data_buf);
+
 #ifdef ENABLE_U2F_EXTENSIONS
     if ( is_extension_request((uint8_t*)&GA.creds[validCredCount - 1].credential.id, sizeof(CredentialId)) )
     {
-        ret = cbor_encode_int(&map,RESP_authData);
+        ret = cbor_encode_int(&map,RESP_authData);  // 2
         check_ret(ret);
         memset(auth_data_buf,0,sizeof(CTAP_authDataHeader));
-        ret = cbor_encode_byte_string(&map, auth_data_buf, sizeof(CTAP_authDataHeader));
+        auth_data_buf_sz = sizeof(CTAP_authDataHeader);
-        check_ret(ret);
     }
     else
 #endif
     {
-        uint32_t len = sizeof(auth_data_buf);
+
-        ret = ctap_make_auth_data(&GA.rp, &map, auth_data_buf, &len, NULL);
+        ret = ctap_make_auth_data(&GA.rp, &map, auth_data_buf, &auth_data_buf_sz, NULL);
         check_retr(ret);
 
         ((CTAP_authData *)auth_data_buf)->head.flags &= ~(1 << 2);
         ((CTAP_authData *)auth_data_buf)->head.flags |= (getAssertionState.user_verified << 2);
 
         {
-            unsigned int ext_encoder_buf_size = sizeof(auth_data_buf) - len;
+            unsigned int ext_encoder_buf_size = sizeof(auth_data_buf) - auth_data_buf_sz;
-            uint8_t * ext_encoder_buf = auth_data_buf + len;
+            uint8_t * ext_encoder_buf = auth_data_buf + auth_data_buf_sz;
 
             ret = ctap_make_extensions(&GA.extensions, ext_encoder_buf, &ext_encoder_buf_size);
             check_retr(ret);
             if (ext_encoder_buf_size)
             {
                 ((CTAP_authData *)auth_data_buf)->head.flags |= (1 << 7);
-                len += ext_encoder_buf_size;
+                auth_data_buf_sz += ext_encoder_buf_size;
             }
         }
 
-        {
-            ret = cbor_encode_int(&map,RESP_authData);
-            check_ret(ret);
-            ret = cbor_encode_byte_string(&map, auth_data_buf, len);
-            check_ret(ret);
-        }
     }
 
     save_credential_list((CTAP_authDataHeader*)auth_data_buf, GA.clientDataHash, GA.creds, validCredCount-1);   // skip last one
 
-    ret = ctap_end_get_assertion(&map, cred, auth_data_buf, GA.clientDataHash, add_user_info);
+    ret = ctap_end_get_assertion(&map, cred, auth_data_buf, auth_data_buf_sz, GA.clientDataHash);  // 1,2,3,4
     check_retr(ret);
 
+    if (validCredCount > 1)
+    {
+        ret = cbor_encode_int(&map, RESP_numberOfCredentials);  // 5
+        check_ret(ret);
+        ret = cbor_encode_int(&map, validCredCount);
+        check_ret(ret);
+    }
+
     ret = cbor_encoder_close_container(encoder, &map);
     check_ret(ret);
 

tools/testing/tests/fido2.py

@@ -2,8 +2,9 @@ from __future__ import print_function, absolute_import, unicode_literals
 import time
 from random import randint
 import array
+from functools import cmp_to_key
 
-
+from fido2 import cbor
 from fido2.ctap import CtapError
 
 from fido2.ctap2 import ES256, PinProtocolV1
@@ -33,9 +34,119 @@ def VerifyAttestation(attest, data):
     verifier().verify(attest.att_statement, attest.auth_data, data.hash)
 
 
+def cbor_key_to_representative(key):
+    if isinstance(key, int):
+        if key >= 0:
+            return (0, key)
+        return (1, -key)
+    elif isinstance(key, bytes):
+        return (2, key)
+    elif isinstance(key, str):
+        return (3, key)
+    else:
+        raise ValueError(key)
+
+
+def cbor_str_cmp(a, b):
+    if isinstance(a, str) or isinstance(b, str):
+        a = a.encode("utf8")
+        b = b.encode("utf8")
+
+    if len(a) == len(b):
+        for x, y in zip(a, b):
+            if x != y:
+                return x - y
+        return 0
+    else:
+        return len(a) - len(b)
+
+
+def cmp_cbor_keys(a, b):
+    a = cbor_key_to_representative(a)
+    b = cbor_key_to_representative(b)
+    if a[0] != b[0]:
+        return a[0] - b[0]
+    if a[0] in (2, 3):
+        return cbor_str_cmp(a[1], b[1])
+    else:
+        return (a[1] > b[1]) - (a[1] < b[1])
+
+
+def TestCborKeysSorted(cbor_obj):
+    # Cbor canonical ordering of keys.
+    # https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#ctap2-canonical-cbor-encoding-form
+
+    if isinstance(cbor_obj, bytes):
+        cbor_obj = cbor.loads(cbor_obj)[0]
+
+    if isinstance(cbor_obj, dict):
+        l = [x for x in cbor_obj]
+    else:
+        l = cbor_obj
+
+    l_sorted = sorted(l[:], key=cmp_to_key(cmp_cbor_keys))
+
+    for i in range(len(l)):
+
+        if not isinstance(l[i], (str, int)):
+            raise ValueError(f"Cbor map key {l[i]} must be int or str for CTAP2")
+
+        if l[i] != l_sorted[i]:
+            raise ValueError(f"Cbor map item {i}: {l[i]} is out of order")
+
+    return l
+
+
+# hot patch cbor map parsing to test the order of keys in map
+_load_map_old = cbor.load_map
+
+
+def _load_map_new(ai, data):
+    values, data = _load_map_old(ai, data)
+    TestCborKeysSorted(values)
+    return values, data
+
+
+cbor.load_map = _load_map_new
+cbor._DESERIALIZERS[5] = _load_map_new
+
+
 class FIDO2Tests(Tester):
     def __init__(self, tester=None):
         super().__init__(tester)
+        self.self_test()
+
+    def self_test(self,):
+        cbor_key_list_sorted = [
+            0,
+            1,
+            1,
+            2,
+            3,
+            -1,
+            -2,
+            "b",
+            "c",
+            "aa",
+            "aaa",
+            "aab",
+            "baa",
+            "bbb",
+        ]
+        with Test("Self test CBOR sorting"):
+            TestCborKeysSorted(cbor_key_list_sorted)
+
+        with Test("Self test CBOR sorting integers", catch=ValueError):
+            TestCborKeysSorted([1, 0])
+
+        with Test("Self test CBOR sorting major type", catch=ValueError):
+            TestCborKeysSorted([-1, 0])
+
+        with Test("Self test CBOR sorting strings", catch=ValueError):
+            TestCborKeysSorted(["bb", "a"])
+
+        with Test("Self test CBOR sorting same length strings", catch=ValueError):
+            TestCborKeysSorted(["ab", "aa"])
 
     def run(self,):
         self.test_fido2()
@@ -229,6 +340,8 @@ class FIDO2Tests(Tester):
     def test_get_info(self,):
         with Test("Get info"):
             info = self.ctap.get_info()
+            print(bytes(info))
+            print(cbor.loads(bytes(info)))
 
         with Test("Check FIDO2 string is in VERSIONS field"):
             assert "FIDO_2_0" in info.versions
@@ -274,6 +387,7 @@ class FIDO2Tests(Tester):
             key_params,
             expectedError=CtapError.ERR.SUCCESS,
         )
+
         allow_list = [
             {
                 "id": prev_reg.auth_data.credential_data.credential_id,

tools/testing/tests/tester.py

@@ -28,14 +28,21 @@ class Packet(object):
 
 
 class Test:
-    def __init__(self, msg):
+    def __init__(self, msg, catch=None):
         self.msg = msg
+        self.catch = catch
 
     def __enter__(self,):
         print(self.msg)
 
     def __exit__(self, a, b, c):
+        if self.catch is None:
             print("Pass")
+        elif isinstance(b, self.catch):
+            print("Pass")
+            return b
+        else:
+            raise RuntimeError(f"Expected exception {self.catch} did not occur.")
 
 
 class Tester: