CVE-2019-18671: Out-of-bounds Write (CWE-787)
value: 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.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2019-12-06T18:15Z
Repository: https://github.com/keepkey/keepkey-firmware
Language: C
Files changed: lib/board/messages.c
Commit date: 2019-09-18 08:49:44 -0600
Commit message: board: factor out tiny_dispatch

And add stronger checks on what tiny_msg's are allowed to be decoded.

Diff: diff --git a/lib/board/messages.c b/lib/board/messages.c
index 726407d0..a5d9fcff 100644
--- a/lib/board/messages.c
+++ b/lib/board/messages.c
@@ -35,15 +35,10 @@ static msg_failure_t msg_failure;
 
 #if DEBUG_LINK
 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
  */
 bool reset_msg_stack = false;
@@ -145,32 +140,10 @@ 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
  *
  * INPUT
@@ -335,66 +308,107 @@ reset:
     cursor = 0;
     firstFrame = true;
     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);
     }
 }
 
 #if DEBUG_LINK
 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);
     }
 }
 #endif

Vulnerable code: /* 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 */

/*
 * 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;
}

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);
        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);
Fixed code: /* 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);
        msg_read_tiny(msg, len);