fix hmac-secret with Get Next Assertion

canokeys · Oct 16, 2023 · e4297fe · e4297fe
1 parent 3cf127b
commit e4297fe
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 12 deletions.
diff --git a/applets/ctap/ctap-internal.h b/applets/ctap/ctap-internal.h
@@ -39,6 +39,7 @@
 #define CTAP_LARGE_BLOBS           0x0C
 #define CTAP_CONFIG                0x0D
 #define CTAP_CRED_MANAGE_LEGACY    0x41
+#define CTAP_INVALID_CMD           0xFF
 
 // Parsed params
 #define PARAM_CLIENT_DATA_HASH       (1 << 0)

diff --git a/applets/ctap/ctap-parser.c b/applets/ctap/ctap-parser.c
@@ -528,12 +528,12 @@ uint8_t parse_ga_extensions(CTAP_get_assertion *ga, CborValue *val) {
            ga->ext_hmac_secret_salt_enc_len != HMAC_SECRET_SALT_SIZE / 2) ||
           (ga->ext_hmac_secret_pin_protocol == 2 && ga->ext_hmac_secret_salt_enc_len != HMAC_SECRET_SALT_SIZE + HMAC_SECRET_SALT_IV_SIZE &&
            ga->ext_hmac_secret_salt_enc_len != HMAC_SECRET_SALT_SIZE / 2 + HMAC_SECRET_SALT_IV_SIZE)) {
-        ERR_MSG("Invalid hmac_secret_salt_len\n");
+        ERR_MSG("Invalid hmac_secret_salt_enc_len %hhu\n", ga->ext_hmac_secret_salt_enc_len);
         return CTAP1_ERR_INVALID_LENGTH;
       }
-      if ((ga->ext_hmac_secret_pin_protocol == 1 && len != HMAC_SECRET_SALT_AUTH_SIZE_P1) ||
-          (ga->ext_hmac_secret_pin_protocol == 2 && len != HMAC_SECRET_SALT_AUTH_SIZE_P2)) {
-        ERR_MSG("Invalid hmac_secret_auth_size\n");
+      if ((ga->ext_hmac_secret_pin_protocol == 1 && ga->ext_hmac_secret_salt_auth_len != HMAC_SECRET_SALT_AUTH_SIZE_P1) ||
+          (ga->ext_hmac_secret_pin_protocol == 2 && ga->ext_hmac_secret_salt_auth_len != HMAC_SECRET_SALT_AUTH_SIZE_P2)) {
+        ERR_MSG("Invalid hmac_secret_salt_auth_len %hhu\n", ga->ext_hmac_secret_salt_auth_len);
         return CTAP1_ERR_INVALID_LENGTH;
       }
       ga->parsed_params |= PARAM_HMAC_SECRET;

diff --git a/applets/ctap/ctap.c b/applets/ctap/ctap.c
@@ -55,7 +55,7 @@ static uint8_t consecutive_pin_counter, last_cmd;
 
 uint8_t ctap_install(uint8_t reset) {
   consecutive_pin_counter = 3;
-  last_cmd = 0xff;
+  last_cmd = CTAP_INVALID_CMD;
   cp_initialize();
   if (!reset && get_file_size(CTAP_CERT_FILE) >= 0) {
     DBG_MSG("CTAP initialized\n");
@@ -1011,6 +1011,8 @@ static uint8_t ctap_get_assertion(CborEncoder *encoder, uint8_t *params, size_t
   // Process hmac-secret extension
   if (ga.parsed_params & PARAM_HMAC_SECRET) {
     if (credential_counter == 0) {
+      // If "up" is set to false, authenticator returns CTAP2_ERR_UNSUPPORTED_OPTION.
+      if (!up) return CTAP2_ERR_UNSUPPORTED_OPTION;
       ret = cp_decapsulate(ga.ext_hmac_secret_key_agreement, ga.ext_hmac_secret_pin_protocol);
       CHECK_PARSER_RET(ret);
       DBG_MSG("Shared secret: ");
@@ -1026,7 +1028,7 @@ static uint8_t ctap_get_assertion(CborEncoder *encoder, uint8_t *params, size_t
         return CTAP2_ERR_UNHANDLED_REQUEST;
       }
     }
-    uint8_t hmac_secret_output[HMAC_SECRET_SALT_SIZE];
+    uint8_t hmac_secret_output[HMAC_SECRET_SALT_IV_SIZE + HMAC_SECRET_SALT_SIZE];
     DBG_MSG("hmac-secret-salt: ");
     PRINT_HEX(ga.ext_hmac_secret_salt_enc, ga.ext_hmac_secret_pin_protocol == 1
                                                ? ga.ext_hmac_secret_salt_enc_len
@@ -1044,17 +1046,17 @@ static uint8_t ctap_get_assertion(CborEncoder *encoder, uint8_t *params, size_t
     if (cp_encrypt(ga.ext_hmac_secret_key_agreement, hmac_secret_output,
                    ga.ext_hmac_secret_pin_protocol == 1 ? ga.ext_hmac_secret_salt_enc_len
                                                         : ga.ext_hmac_secret_salt_enc_len - HMAC_SECRET_SALT_IV_SIZE,
-                   ga.ext_hmac_secret_salt_enc, ga.ext_hmac_secret_pin_protocol) < 0)
+                   hmac_secret_output, ga.ext_hmac_secret_pin_protocol) < 0)
       return CTAP2_ERR_UNHANDLED_REQUEST;
     DBG_MSG("hmac-secret output: ");
-    PRINT_HEX(ga.ext_hmac_secret_salt_enc, ga.ext_hmac_secret_salt_enc_len);
+    PRINT_HEX(hmac_secret_output, ga.ext_hmac_secret_salt_enc_len);
     if (credential_counter + 1 == number_of_credentials) { // encryption key will not be used any more
       memzero(ga.ext_hmac_secret_key_agreement, sizeof(ga.ext_hmac_secret_key_agreement));
     }
 
     ret = cbor_encode_text_stringz(&map, "hmac-secret");
     CHECK_CBOR_RET(ret);
-    ret = cbor_encode_byte_string(&map, ga.ext_hmac_secret_salt_enc, ga.ext_hmac_secret_salt_enc_len);
+    ret = cbor_encode_byte_string(&map, hmac_secret_output, ga.ext_hmac_secret_salt_enc_len);
     CHECK_CBOR_RET(ret);
   }
   ret = cbor_encoder_close_container(&extension_encoder, &map);
@@ -2146,6 +2148,9 @@ int ctap_process_cbor(uint8_t *req, size_t req_len, uint8_t *resp, size_t *resp_
       break;
   }
   last_cmd = cmd;
+  if (*resp != 0) { // do not allow GET_NEXT_ASSERTION if error occurs
+    last_cmd = CTAP_INVALID_CMD;
+  }
   return 0;
 }
 

diff --git a/applets/ctap/secret.c b/applets/ctap/secret.c
@@ -125,18 +125,20 @@ int cp_encrypt(const uint8_t *key, const uint8_t *in, size_t in_size, uint8_t *o
   block_cipher_config cfg = {.block_size = 16, .mode = CBC, .iv = iv, .encrypt = aes256_enc, .decrypt = aes256_dec};
   cfg.in_size = in_size;
   cfg.in = in;
+  cfg.out = out;
   if (pin_protocol == 1) {
     memzero(iv, sizeof(iv));
     cfg.key = key;
-    cfg.out = out;
   } else {
     random_buffer(iv, sizeof(iv));
     cfg.key = key + SHARED_SECRET_SIZE_HMAC;
-    cfg.out = out + sizeof(iv);
   }
   int ret = block_cipher_enc(&cfg);
-  if (pin_protocol == 2)
+  if (pin_protocol == 2) {
+    // "in" and "out" arguments can be the same pointer 
+    memmove(out + sizeof(iv), out, in_size);
     memcpy(out, iv, sizeof(iv));
+  }
   return ret;
 }