Merge pull request #102 from canokeys/dev

Block CTAP commands come from the second interface
canokeys · Dec 7, 2024 · 9834767 · 9834767
2 parents 7cb3350 + f25fb2d
commit 9834767
Show file tree

Hide file tree

Showing 9 changed files with 72 additions and 42 deletions.
diff --git a/applets/ctap/ctap.c b/applets/ctap/ctap.c
@@ -38,8 +38,8 @@
 
 #define WAIT(timeout_response)                                                                                         \
   do {                                                                                                                 \
-    if (is_nfc()) break;                                                                                               \
-    switch (wait_for_user_presence(WAIT_ENTRY_CTAPHID)) {                                                              \
+    if (is_nfc()) break;\
+    switch (wait_for_user_presence(current_cmd_src == CTAP_SRC_HID ? WAIT_ENTRY_CTAPHID : WAIT_ENTRY_CCID)) {                                                              \
     case USER_PRESENCE_CANCEL:                                                                                         \
       return CTAP2_ERR_KEEPALIVE_CANCEL;                                                                               \
     case USER_PRESENCE_TIMEOUT:                                                                                        \
@@ -50,20 +50,23 @@
 #define KEEPALIVE()                                                                                                    \
   do {                                                                                                                 \
     if (is_nfc()) break;                                                                                               \
-    send_keepalive_during_processing(WAIT_ENTRY_CTAPHID);                                                              \
+    send_keepalive_during_processing(current_cmd_src == CTAP_SRC_HID ? WAIT_ENTRY_CTAPHID : WAIT_ENTRY_CCID);                                                              \
   } while (0)
 
 static const uint8_t aaguid[] = {0x24, 0x4e, 0xb2, 0x9e, 0xe0, 0x90, 0x4e, 0x49,
                                  0x81, 0xfe, 0x1f, 0x20, 0xf8, 0xd3, 0xb8, 0xf4};
 
 // pin & command states
 static uint8_t consecutive_pin_counter, last_cmd;
+// source of APDU in process 
+static ctap_src_t current_cmd_src;
 // SM2 attr
 CTAP_sm2_attr ctap_sm2_attr;
 
 uint8_t ctap_install(uint8_t reset) {
   consecutive_pin_counter = 3;
   last_cmd = CTAP_INVALID_CMD;
+  current_cmd_src = CTAP_SRC_NONE;
   cp_initialize();
   if (!reset && get_file_size(LB_FILE) >= 0) {
     if (read_attr(CTAP_CERT_FILE, SM2_ATTR, &ctap_sm2_attr, sizeof(ctap_sm2_attr)) < 0) return CTAP2_ERR_UNHANDLED_REQUEST;
@@ -2158,6 +2161,9 @@ static uint8_t ctap_large_blobs(CborEncoder *encoder, const uint8_t *params, siz
 
 int ctap_process_cbor(uint8_t *req, size_t req_len, uint8_t *resp, size_t *resp_len) {
   if (req_len-- == 0) return -1;
+  if (current_cmd_src != CTAP_SRC_NONE) return -1;
+  // Must set current_cmd_src to CTAP_SRC_NONE before return
+  current_cmd_src = CTAP_SRC_HID;
 
   cp_pin_uv_auth_token_usage_timer_observer();
 
@@ -2227,11 +2233,15 @@ int ctap_process_cbor(uint8_t *req, size_t req_len, uint8_t *resp, size_t *resp_
   if (*resp != 0) { // do not allow GET_NEXT_ASSERTION if error occurs
     last_cmd = CTAP_INVALID_CMD;
   }
+  current_cmd_src = CTAP_SRC_NONE;
   return 0;
 }
 
-int ctap_process_apdu(const CAPDU *capdu, RAPDU *rapdu) {
+int ctap_process_apdu_with_src(const CAPDU *capdu, RAPDU *rapdu, ctap_src_t src) {
   int ret = 0;
+  if (current_cmd_src != CTAP_SRC_NONE) EXCEPT(SW_UNABLE_TO_PROCESS);
+  // Must set current_cmd_src to CTAP_SRC_NONE before return
+  current_cmd_src = src;
   LL = 0;
   SW = SW_NO_ERROR;
   if (CLA == 0x80) {
@@ -2243,6 +2253,7 @@ int ctap_process_apdu(const CAPDU *capdu, RAPDU *rapdu) {
       // len is the actual len written to RDATA
       LL = len;
     } else {
+      current_cmd_src = CTAP_SRC_NONE;
       EXCEPT(SW_INS_NOT_SUPPORTED);
     }
   } else if (CLA == 0x00) {
@@ -2262,11 +2273,15 @@ int ctap_process_apdu(const CAPDU *capdu, RAPDU *rapdu) {
       case CTAP_INS_MSG:
         break;
       default:
+        current_cmd_src = CTAP_SRC_NONE;
         EXCEPT(SW_INS_NOT_SUPPORTED);
     }
-  } else
+  } else {
+    current_cmd_src = CTAP_SRC_NONE;
     EXCEPT(SW_CLA_NOT_SUPPORTED);
+  }
 
+  current_cmd_src = CTAP_SRC_NONE;
   if (ret < 0)
     EXCEPT(SW_UNABLE_TO_PROCESS);
   else

diff --git a/include/common.h b/include/common.h
@@ -9,6 +9,8 @@
 #include <string.h>
 
 #define APDU_BUFFER_SIZE 1340
+#define TOUCH_EXPIRE_TIME 1000
+#define TOUCH_AFTER_PWRON 1500
 
 #ifdef DEBUG_OUTPUT
 #include <crypto-util.h>

diff --git a/include/ctap.h b/include/ctap.h
@@ -5,13 +5,22 @@
 #include <apdu.h>
 #include <stdint.h>
 
+typedef enum {
+    CTAP_SRC_NONE,
+    CTAP_SRC_CCID,
+    CTAP_SRC_HID,
+} ctap_src_t;
+
 uint8_t ctap_install(uint8_t reset);
 int ctap_install_private_key(const CAPDU *capdu, RAPDU *rapdu);
 int ctap_install_cert(const CAPDU *capdu, RAPDU *rapdu);
 int ctap_read_sm2_config(const CAPDU *capdu, RAPDU *rapdu);
 int ctap_write_sm2_config(const CAPDU *capdu, RAPDU *rapdu);
 int ctap_process_cbor(uint8_t *req, size_t req_len, uint8_t *resp, size_t *resp_len);
-int ctap_process_apdu(const CAPDU *capdu, RAPDU *rapdu);
+int ctap_process_apdu_with_src(const CAPDU *capdu, RAPDU *rapdu, ctap_src_t src);
+static int ctap_process_apdu(const CAPDU *capdu, RAPDU *rapdu) {
+    return ctap_process_apdu_with_src(capdu, rapdu, CTAP_SRC_CCID);
+}
 int ctap_wink(void);
 
 #endif // CANOKEY_CORE_FIDO2_FIDO2_H_
diff --git a/interfaces/USB/class/ctaphid/ctaphid.c b/interfaces/USB/class/ctaphid/ctaphid.c
@@ -103,7 +103,7 @@ static void CTAPHID_Execute_Msg(void) {
   RDATA = channel.data;
   DBG_MSG("C: ");
   PRINT_HEX(channel.data, channel.bcnt_total);
-  ctap_process_apdu(capdu, rapdu);
+  ctap_process_apdu_with_src(capdu, rapdu, CTAP_SRC_HID);
   channel.data[LL] = HI(SW);
   channel.data[LL + 1] = LO(SW);
   DBG_MSG("R: ");

diff --git a/interfaces/USB/class/kbdhid/kbdhid.c b/interfaces/USB/class/kbdhid/kbdhid.c
@@ -143,6 +143,7 @@ uint8_t KBDHID_Loop(void) {
     const uint8_t touch = get_touch_result();
     if (touch != TOUCH_NO) {
       const int len = pass_handle_touch(touch, key_sequence);
+      set_touch_result(TOUCH_NO);
       if (len <= 0) {
         DBG_MSG("Do nothing\n");
         return 0;
@@ -151,7 +152,6 @@ uint8_t KBDHID_Loop(void) {
       key_seq_position = 0;
       state = KBDHID_Typing;
       DBG_MSG("Start typing %s\n", key_sequence);
-      set_touch_result(TOUCH_NO);
     }
   } else {
     KBDHID_TypeKeySeq();

diff --git a/src/apdu.c b/src/apdu.c
@@ -241,7 +241,7 @@ void process_apdu(CAPDU *capdu, RAPDU *rapdu) {
         break;
       }
 #endif
-      ctap_process_apdu(capdu, &rapdu_chaining.rapdu);
+      ctap_process_apdu_with_src(capdu, &rapdu_chaining.rapdu, CTAP_SRC_CCID);
       rapdu->len = LE;
       apdu_output(&rapdu_chaining, rapdu);
       break;

diff --git a/src/device.c b/src/device.c
@@ -26,8 +26,8 @@ void device_loop(void) {
 bool device_allow_kbd_touch(void) {
   uint32_t now = device_get_tick();
   if (!device_is_blinking() &&      // applets are not waiting for touch
-      now > 2000   &&               // ignore touch for the first 2 seconds
-      now - 1000 > last_blink &&
+      now > TOUCH_AFTER_PWRON &&    // ignore touch for some time after power-on
+      now - TOUCH_EXPIRE_TIME > last_blink &&
       get_touch_result() != TOUCH_NO
   ) {
     DBG_MSG("now=%lu last_blink=%lu\n", now, last_blink);
@@ -46,12 +46,7 @@ uint8_t get_touch_result(void) {
 void set_touch_result(uint8_t result) { touch_result = result; }
 
 uint8_t wait_for_user_presence(uint8_t entry) {
-  start_blinking(0);
-  uint32_t start = device_get_tick();
-  uint32_t last = start;
-  DBG_MSG("start %u\n", start);
 
-  wait_status_t shallow = wait_status;
   if (wait_status == WAIT_NONE) {
     switch (entry) {
     case WAIT_ENTRY_CCID:
@@ -61,41 +56,42 @@ uint8_t wait_for_user_presence(uint8_t entry) {
       wait_status = WAIT_CTAPHID;
       break;
     }
-  } else
-    wait_status = WAIT_DEEP;
+  } else {
+    // New user presence test is denied while a test is ongoing
+    DBG_MSG("Denied\n");
+    return USER_PRESENCE_TIMEOUT;
+  }
+
+  uint32_t start = device_get_tick();
+  uint32_t last = start;
+  DBG_MSG("start %u\n", start);
   while (get_touch_result() == TOUCH_NO) {
-    if (wait_status == WAIT_DEEP_TOUCHED || wait_status == WAIT_DEEP_CANCEL) break;
-    if (wait_status == WAIT_CTAPHID) CCID_Loop();
-    if (CTAPHID_Loop(wait_status != WAIT_CCID) == LOOP_CANCEL) {
+    // Keep blinking, in case other applet stops it 
+    start_blinking(0);
+    // Nested CCID processing is not allowed
+    if (entry != WAIT_ENTRY_CCID) CCID_Loop();
+    if (CTAPHID_Loop(entry == WAIT_ENTRY_CTAPHID) == LOOP_CANCEL) {
       DBG_MSG("Cancelled by host\n");
-      if (wait_status != WAIT_DEEP) {
-        stop_blinking();
-        wait_status = WAIT_NONE; // namely shallow
-      } else
-        wait_status = WAIT_DEEP_CANCEL;
+      stop_blinking();
+      wait_status = WAIT_NONE;
       return USER_PRESENCE_CANCEL;
     }
     uint32_t now = device_get_tick();
     if (now - start >= 30000) {
       DBG_MSG("timeout at %u\n", now);
-      if (wait_status != WAIT_DEEP) stop_blinking();
-      wait_status = shallow;
+      stop_blinking();
+      wait_status = WAIT_NONE;
       return USER_PRESENCE_TIMEOUT;
     }
     if (now - last >= 100) {
       last = now;
-      if (wait_status != WAIT_CCID) CTAPHID_SendKeepAlive(KEEPALIVE_STATUS_UPNEEDED);
+      if (entry == WAIT_ENTRY_CTAPHID) CTAPHID_SendKeepAlive(KEEPALIVE_STATUS_UPNEEDED);
     }
   }
+  // Consume this touch event
   set_touch_result(TOUCH_NO);
-  if (wait_status != WAIT_DEEP) stop_blinking();
-  if (wait_status == WAIT_DEEP)
-    wait_status = WAIT_DEEP_TOUCHED;
-  else if (wait_status == WAIT_DEEP_CANCEL) {
-    wait_status = WAIT_NONE;
-    return USER_PRESENCE_TIMEOUT;
-  } else
-    wait_status = WAIT_NONE;
+  stop_blinking();
+  wait_status = WAIT_NONE;
   return USER_PRESENCE_OK;
 }
 
@@ -149,8 +145,9 @@ static void toggle_led(void) {
 
 void device_update_led(void) {
   uint32_t now = device_get_tick();
-  if (now > blink_timeout) stop_blinking();
-  if (device_is_blinking() && now >= last_blink && now - last_blink >= blink_interval) {
+  if (now > blink_timeout) {
+    stop_blinking();
+  } else if (device_is_blinking() && now >= last_blink && now - last_blink >= blink_interval) {
     last_blink = now;
     toggle_led();
   }

diff --git a/test-real/test-piv.sh b/test-real/test-piv.sh
@@ -97,6 +97,9 @@ rsa_tests() {
     PIVSignDec 9e # PIN not required for key 9e
     for s in 9a 9c 9d; do PIVSignDec $s 1; done
 
+    if (( $1 == RSA2048 )); then
+        return
+    fi
     out=$(pkcs15-tool --reader "$RDID" --read-certificate 04 | openssl x509 -text)
     assertContains 'CERT' "$out" 'CN = CertAtSlot9e'
     echo -n hello >$TEST_TMP_DIR/hello.txt
@@ -214,10 +217,14 @@ test_FactoryReset() {
 }
 
 test_FillData() {
-    openssl req -x509 -newkey rsa:2048 -keyout $TEST_TMP_DIR/key.pem -out $TEST_TMP_DIR/cert.pem -days 365 -nodes -subj "/CN=www.example.com"
+    openssl req -x509 -newkey rsa:4096 -keyout $TEST_TMP_DIR/key.pem -out $TEST_TMP_DIR/cert.pem -days 365 -nodes -subj "/CN=www.example.com"
     assertEquals 'openssl gen key' 0 $?
+    openssl rand -base64 -out $TEST_TMP_DIR/rand-pi 242
+    YPT -a write-object --id 0x5fc109 -i $TEST_TMP_DIR/rand-pi -f base64
+    YPT -a write-object --id 0x5fc108 -i $TEST_TMP_DIR/rand-pi -f base64
+    YPT -a write-object --id 0x5fc103 -i $TEST_TMP_DIR/rand-pi -f base64
     for s in 9a 9c 9d 9e 82 83; do
-        PIVImportKeyCert $s $TEST_TMP_DIR/key.pem ../test-via-pcsc/long-cert.pem
+        PIVImportKeyCert $s $TEST_TMP_DIR/key.pem  $TEST_TMP_DIR/cert.pem
         assertEquals 'import-key' 0 $?
     done
     YPT -a status

diff --git a/virt-card/fabrication.c b/virt-card/fabrication.c
@@ -94,7 +94,7 @@ static void fake_fido_personalization() {
   sm2_attr.enabled = 1;
 
   capdu.ins = ADMIN_INS_WRITE_CTAP_SM2_CONFIG;
-  capdu.data = &sm2_attr;
+  capdu.data = (uint8_t *) &sm2_attr;
   capdu.lc = sizeof(sm2_attr);
   admin_process_apdu(&capdu, &rapdu);
   assert(rapdu.sw == 0x9000);