Vulnerability: Insufficient checks in the USB packet handling of the ShapeShift KeepKey hardware wallet before firmware 6.2.2 allow out-of-bounds writes in the .bss segment via crafted messages. The vulnerability could allow code execution or other forms of impact. It can be triggered by unauthenticated attackers and the interface is reachable via WebUSB.
Vulnerability ID: CVE-2019-18671
CWE: CWE-787
Severity score: 10
Project: https://github.com/keepkey/keepkey-firmware
Published: 2019-12-06
File: lib/board/messages.c
Commit message: board: factor out tiny_dispatch

And add stronger checks on what tiny_msg's are allowed to be decoded.
Commit author: keepkeyjon

Vulnerable version (before fix):
 static msg_failure_t msg_failure; static msg_debug_link_get_state_t msg_debug_link_get_state; #endif  /* Tiny messages */ static bool msg_tiny_flag = false; static CONFIDENTIAL uint8_t msg_tiny[MSG_TINY_BFR_SZ]; static uint16_t msg_tiny_id = MSG_TINY_TYPE_ERROR; /* Default to error type */  /* Allow mapped messages to reset message stack.  This variable by itself doesn't  * do much but messages down the line can use it to determine for to gracefully  * exit from a message should the message stack been reset static void dispatch(const MessagesMap_t *entry, uint8_t *msg, uint32_t msg_size     entry->process_func(decode_buffer); }  /*  * tiny_dispatch() - Process received tiny messages  *  * INPUT  *     - entry: pointer to message entry  *     - msg: pointer to received message buffer  *     - msg_size: size of message  * OUTPUT  *     none  *  */ static void tiny_dispatch(const MessagesMap_t *entry, uint8_t *msg, uint32_t msg_size) {     if (!pb_parse(entry, msg, msg_size, msg_tiny)) {         call_msg_failure_handler(FailureType_Failure_UnexpectedMessage,                                  "Could not parse tiny protocol buffer message");         return;     }      msg_tiny_id = entry->msg_id; }  /*  * raw_dispatch() - Process messages that will not be parsed by protocol buffers  * and should be manually parsed at message function void usb_rx_helper(const uint8_t *buf, size_t length, MessageMapType type)     entry = NULL; }  void handle_usb_rx(const void *msg, size_t len) {     if (msg_tiny_flag) {         uint8_t buf[64];         memcpy(buf, msg, sizeof(buf));          uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;         uint32_t msgSize = buf[8]        |                 ((uint32_t)buf[7]) <<  8 |                 ((uint32_t)buf[6]) << 16 |                 ((uint32_t)buf[5]) << 24;          if (msgSize > 64 - 9) {             (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");             return;         }          // Determine callback handler and message map type.         const MessagesMap_t *entry = message_map_entry(NORMAL_MSG, msgId, IN_MSG);         if (!entry) {             (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");             return;         }          tiny_dispatch(entry, buf + 9, msgSize);     } else {         usb_rx_helper(msg, len, NORMAL_MSG);     } void handle_usb_rx(const void *msg, size_t len) void handle_debug_usb_rx(const void *msg, size_t len) {     if (msg_tiny_flag) {         uint8_t buf[64];         memcpy(buf, msg, sizeof(buf));          uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;         uint32_t msgSize = buf[8]        |                 ((uint32_t)buf[7]) <<  8 |                 ((uint32_t)buf[6]) << 16 |                 ((uint32_t)buf[5]) << 24;          if (msgSize > 64 - 9) {             (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");             return;         }          // Determine callback handler and message map type.         const MessagesMap_t *entry = message_map_entry(DEBUG_MSG, msgId, IN_MSG);         if (!entry) {             (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");             return;         }          tiny_dispatch(entry, buf + 9, msgSize);     } else {         usb_rx_helper(msg, len, DEBUG_MSG);     }

Fixed version (after fix):
 static msg_failure_t msg_failure; static msg_debug_link_get_state_t msg_debug_link_get_state; #endif  /* Allow mapped messages to reset message stack.  This variable by itself doesn't  * do much but messages down the line can use it to determine for to gracefully  * exit from a message should the message stack been reset static void dispatch(const MessagesMap_t *entry, uint8_t *msg, uint32_t msg_size     entry->process_func(decode_buffer); }  /*  * raw_dispatch() - Process messages that will not be parsed by protocol buffers  * and should be manually parsed at message function void usb_rx_helper(const uint8_t *buf, size_t length, MessageMapType type)     entry = NULL; }  /* Tiny messages */ static bool msg_tiny_flag = false; static CONFIDENTIAL uint8_t msg_tiny[MSG_TINY_BFR_SZ]; static uint16_t msg_tiny_id = MSG_TINY_TYPE_ERROR; /* Default to error type */  _Static_assert(sizeof(msg_tiny) >= sizeof(Cancel), "msg_tiny too tiny"); _Static_assert(sizeof(msg_tiny) >= sizeof(Initialize), "msg_tiny too tiny"); _Static_assert(sizeof(msg_tiny) >= sizeof(PassphraseAck), "msg_tiny too tiny"); _Static_assert(sizeof(msg_tiny) >= sizeof(ButtonAck), "msg_tiny too tiny"); _Static_assert(sizeof(msg_tiny) >= sizeof(PinMatrixAck), "msg_tiny too tiny"); #if DEBUG_LINK _Static_assert(sizeof(msg_tiny) >= sizeof(DebugLinkDecision),                "msg_tiny too tiny"); _Static_assert(sizeof(msg_tiny) >= sizeof(DebugLinkGetState),                "msg_tiny too tiny"); #endif  static void msg_read_tiny(const uint8_t *msg, size_t len) {     if (len != 64)         return;      uint8_t buf[64];     memcpy(buf, msg, sizeof(buf));      if (buf[0] != '?' || buf[1] != '#' || buf[2] != '#') {         (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");         return;     }      uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;     uint32_t msgSize = buf[8]        |             ((uint32_t)buf[7]) <<  8 |             ((uint32_t)buf[6]) << 16 |             ((uint32_t)buf[5]) << 24;      if (msgSize > 64 - 9) {         (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");         return;     }      const pb_field_t *fields = NULL;     pb_istream_t stream = pb_istream_from_buffer(buf + 9, msgSize);      switch (msgId) {     case MessageType_MessageType_PinMatrixAck:         fields = PinMatrixAck_fields;         break;     case MessageType_MessageType_ButtonAck:         fields = ButtonAck_fields;         break;     case MessageType_MessageType_PassphraseAck:         fields = PassphraseAck_fields;         break;     case MessageType_MessageType_Cancel:         fields = Cancel_fields;         break;     case MessageType_MessageType_Initialize:         fields = Initialize_fields;         break; #if DEBUG_LINK     case MessageType_MessageType_DebugLinkDecision:         fields = DebugLinkDecision_fields;         break;     case MessageType_MessageType_DebugLinkGetState:         fields = DebugLinkGetState_fields;         break; #endif     }      if (fields) {         bool status = pb_decode(&stream, fields, msg_tiny);         if (status) {             msg_tiny_id = msgId;         } else {             (*msg_failure)(FailureType_Failure_SyntaxError, stream.errmsg);             msg_tiny_id = 0xffff;         }     } else {         (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");         msg_tiny_id = 0xffff;     } }  void handle_usb_rx(const void *msg, size_t len) {     if (msg_tiny_flag) {         msg_read_tiny(msg, len);     } else {         usb_rx_helper(msg, len, NORMAL_MSG);     } void handle_usb_rx(const void *msg, size_t len) void handle_debug_usb_rx(const void *msg, size_t len) {     if (msg_tiny_flag) {         msg_read_tiny(msg, len);     } else {         usb_rx_helper(msg, len, DEBUG_MSG);     }

Patch:
patch: @@ -37,11 +37,6 @@ static msg_failure_t msg_failure;
 static msg_debug_link_get_state_t msg_debug_link_get_state;
 #endif
 
-/* Tiny messages */
-static bool msg_tiny_flag = false;
-static CONFIDENTIAL uint8_t msg_tiny[MSG_TINY_BFR_SZ];
-static uint16_t msg_tiny_id = MSG_TINY_TYPE_ERROR; /* Default to error type */
-
 /* Allow mapped messages to reset message stack.  This variable by itself doesn't
  * do much but messages down the line can use it to determine for to gracefully
  * exit from a message should the message stack been reset
@@ -147,28 +142,6 @@ static void dispatch(const MessagesMap_t *entry, uint8_t *msg, uint32_t msg_size
     entry->process_func(decode_buffer);
 }
 
-/*
- * tiny_dispatch() - Process received tiny messages
- *
- * INPUT
- *     - entry: pointer to message entry
- *     - msg: pointer to received message buffer
- *     - msg_size: size of message
- * OUTPUT
- *     none
- *
- */
-static void tiny_dispatch(const MessagesMap_t *entry, uint8_t *msg, uint32_t msg_size)
-{
-    if (!pb_parse(entry, msg, msg_size, msg_tiny)) {
-        call_msg_failure_handler(FailureType_Failure_UnexpectedMessage,
-                                 "Could not parse tiny protocol buffer message");
-        return;
-    }
-
-    msg_tiny_id = entry->msg_id;
-}
-
 /*
  * raw_dispatch() - Process messages that will not be parsed by protocol buffers
  * and should be manually parsed at message function
@@ -337,31 +310,93 @@ void usb_rx_helper(const uint8_t *buf, size_t length, MessageMapType type)
     entry = NULL;
 }
 
-void handle_usb_rx(const void *msg, size_t len)
-{
-    if (msg_tiny_flag) {
-        uint8_t buf[64];
-        memcpy(buf, msg, sizeof(buf));
-
-        uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;
-        uint32_t msgSize = buf[8]        |
-                ((uint32_t)buf[7]) <<  8 |
-                ((uint32_t)buf[6]) << 16 |
-                ((uint32_t)buf[5]) << 24;
-
-        if (msgSize > 64 - 9) {
-            (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");
-            return;
-        }
+/* Tiny messages */
+static bool msg_tiny_flag = false;
+static CONFIDENTIAL uint8_t msg_tiny[MSG_TINY_BFR_SZ];
+static uint16_t msg_tiny_id = MSG_TINY_TYPE_ERROR; /* Default to error type */
 
-        // Determine callback handler and message map type.
-        const MessagesMap_t *entry = message_map_entry(NORMAL_MSG, msgId, IN_MSG);
-        if (!entry) {
-            (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");
-            return;
+_Static_assert(sizeof(msg_tiny) >= sizeof(Cancel), "msg_tiny too tiny");
+_Static_assert(sizeof(msg_tiny) >= sizeof(Initialize), "msg_tiny too tiny");
+_Static_assert(sizeof(msg_tiny) >= sizeof(PassphraseAck), "msg_tiny too tiny");
+_Static_assert(sizeof(msg_tiny) >= sizeof(ButtonAck), "msg_tiny too tiny");
+_Static_assert(sizeof(msg_tiny) >= sizeof(PinMatrixAck), "msg_tiny too tiny");
+#if DEBUG_LINK
+_Static_assert(sizeof(msg_tiny) >= sizeof(DebugLinkDecision),
+               "msg_tiny too tiny");
+_Static_assert(sizeof(msg_tiny) >= sizeof(DebugLinkGetState),
+               "msg_tiny too tiny");
+#endif
+
+static void msg_read_tiny(const uint8_t *msg, size_t len) {
+    if (len != 64)
+        return;
+
+    uint8_t buf[64];
+    memcpy(buf, msg, sizeof(buf));
+
+    if (buf[0] != '?' || buf[1] != '#' || buf[2] != '#') {
+        (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");
+        return;
+    }
+
+    uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;
+    uint32_t msgSize = buf[8]        |
+            ((uint32_t)buf[7]) <<  8 |
+            ((uint32_t)buf[6]) << 16 |
+            ((uint32_t)buf[5]) << 24;
+
+    if (msgSize > 64 - 9) {
+        (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");
+        return;
+    }
+
+    const pb_field_t *fields = NULL;
+    pb_istream_t stream = pb_istream_from_buffer(buf + 9, msgSize);
+
+    switch (msgId) {
+    case MessageType_MessageType_PinMatrixAck:
+        fields = PinMatrixAck_fields;
+        break;
+    case MessageType_MessageType_ButtonAck:
+        fields = ButtonAck_fields;
+        break;
+    case MessageType_MessageType_PassphraseAck:
+        fields = PassphraseAck_fields;
+        break;
+    case MessageType_MessageType_Cancel:
+        fields = Cancel_fields;
+        break;
+    case MessageType_MessageType_Initialize:
+        fields = Initialize_fields;
+        break;
+#if DEBUG_LINK
+    case MessageType_MessageType_DebugLinkDecision:
+        fields = DebugLinkDecision_fields;
+        break;
+    case MessageType_MessageType_DebugLinkGetState:
+        fields = DebugLinkGetState_fields;
+        break;
+#endif
+    }
+
+    if (fields) {
+        bool status = pb_decode(&stream, fields, msg_tiny);
+        if (status) {
+            msg_tiny_id = msgId;
+        } else {
+            (*msg_failure)(FailureType_Failure_SyntaxError, stream.errmsg);
+            msg_tiny_id = 0xffff;
         }
+    } else {
+        (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");
+        msg_tiny_id = 0xffff;
+    }
+}
 
-        tiny_dispatch(entry, buf + 9, msgSize);
+void handle_usb_rx(const void *msg, size_t len)
+{
+    if (msg_tiny_flag) {
+        msg_read_tiny(msg, len);
     } else {
         usb_rx_helper(msg, len, NORMAL_MSG);
     }
@@ -371,28 +406,7 @@ void handle_usb_rx(const void *msg, size_t len)
 void handle_debug_usb_rx(const void *msg, size_t len)
 {
     if (msg_tiny_flag) {
-        uint8_t buf[64];
-        memcpy(buf, msg, sizeof(buf));
-
-        uint16_t msgId = buf[4] | ((uint16_t)buf[3]) << 8;
-        uint32_t msgSize = buf[8]        |
-                ((uint32_t)buf[7]) <<  8 |
-                ((uint32_t)buf[6]) << 16 |
-                ((uint32_t)buf[5]) << 24;
-
-        if (msgSize > 64 - 9) {
-            (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Malformed tiny packet");
-            return;
-        }
-
-        // Determine callback handler and message map type.
-        const MessagesMap_t *entry = message_map_entry(DEBUG_MSG, msgId, IN_MSG);
-        if (!entry) {
-            (*msg_failure)(FailureType_Failure_UnexpectedMessage, "Unknown message");
-            return;
-        }
-
-        tiny_dispatch(entry, buf + 9, msgSize);
+        msg_read_tiny(msg, len);
     } else {
         usb_rx_helper(msg, len, DEBUG_MSG);
     }
path: lib/board/messages.c