CVE-2019-18671 (C, published 2019-12-06T18:15Z)
Description: 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.
Severity: HIGH (CVSS v3 base score: 9.8)
Weakness: CWE-787 \u2014 Out-of-bounds Write
The product writes data past the end, or before the beginning, of the intended buffer.
Repository: https://github.com/keepkey/keepkey-firmware
Files changed:
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.


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);