A fifth length byte: integer overflow and a signed syscall compare in Zephyr RTOS

MQTT caps its packet-length field at four bytes. Zephyr's decoder would read five. The reason is one character in subsys/net/lib/mqtt/mqtt_decoder.c:

do {
        if (bytes > MQTT_MAX_LENGTH_BYTES) {
                return -EINVAL;
        }
        if (buf->cur >= buf->end) {
                return -EAGAIN;
        }
        *length += ((u32_t)*(buf->cur) & MQTT_LENGTH_VALUE_MASK) << shift;
        shift += MQTT_LENGTH_SHIFT;
        bytes++;
} while ((*(buf->cur++) & MQTT_LENGTH_CONTINUATION_BIT) != 0U);

MQTT_MAX_LENGTH_BYTES is 4. Count how many times the body can run. bytes starts at zero, and the check rejects only when bytes is strictly greater than 4. So the loop is happy to process a fifth byte before it gives up. NCC Group's report says the logic "allows the code to parse up to 5 length bytes, rather than 4". The cause is "the use of the > operator instead of >=."

Everything interesting is downstream of that character.

Ilya Zhuravlev and Jeremy Boone did this work at NCC Group, over a self-directed research project into Zephyr and MCUboot that they published in May 2020. Zephyr is a real-time OS for microcontrollers, sponsored by the Linux Foundation. It aims at the class of device where Linux won't fit. The report runs to eighty pages and covers 25 issues in Zephyr and one in MCUboot. Almost nobody read it. Two of the findings are worth walking through here, because they are different flavours of the same mistake: a comparison that's correct in isolation and wrong for the value being compared.

The MQTT length field#

Every MQTT packet begins with a fixed header: one control byte, then a variable-length integer giving the number of bytes remaining in the packet. The integer is encoded seven bits at a time. The low seven bits of each byte carry data, and the top bit says whether another byte follows. So the value 127 fits in one byte and 128 takes two. The spec caps the field at four bytes, which gives you a maximum packet size of 256 MB. That cap is the only reason a length field like this can't run away.

Zephyr's decoder shifts each new byte in by shift, which grows by seven each iteration. With four bytes the last shift is 21 and the value tops out at 28 bits, comfortably inside a 32-bit result. With five, the last byte is shifted by 28, so seven bits starting at bit 28 reach bit 34. The report works out the maximum as 0x7_ffff_ffff. That does not fit in the u32_t it's being accumulated into, and the top bits fall off the end. What's left is that "any length value in the range 0x0000_0000-0xffff_ffff is possible". The attacker now picks the number, rather than being confined to the range the wire format is supposed to allow.

Nothing between there and the consumers re-checks it. The value comes back out of packet_length_decode, through fixed_header_decode, through mqtt_read_and_parse_fixed_header, and lands in mqtt_handle_rx as var_length. From there it goes two places.

The first is mqtt_read_message_chunk, which wants to top up the receive buffer:

remaining = length - (buf->end - buf->cur);

if (remaining <= 0) {
        return 0;
}

/* Check if read does not exceed the buffer. */
if (buf->end + remaining > client->rx_buf + client->rx_buf_size) {
        /* ... */
        return -ENOMEM;
}

len = mqtt_transport_read(client, buf->end, remaining, false);

remaining is an int, length is a u32_t, and buf->end is a pointer. If remaining is huge, the expression buf->end + remaining wraps around to a small value. The bounds check passes, and the code hands that same huge remaining to a thin wrapper around recv(). NCC Group point out a detail that helps the attacker here. recv returning fewer bytes than asked for means you don't have to write two gigabytes past the buffer. You write however much you want to.

The second path is the mirror image. var_length goes to publish_decode, which does:

param->message.payload.len = var_length - var_header_length;

Send a tiny length and that subtraction underflows. The resulting enormous payload.len is copied into client->internal.remaining_payload, where the public mqtt_read_publish_payload API will happily use it. One direction overflows, the other underflows. Both are reachable from a single MQTT packet from the network. That's why the finding is rated Critical.

The fix, in PR 23821, is what you'd guess. The operator becomes >=. The function also grows an actual upper bound check on the decoded value, rather than relying on the byte count to imply one. It's CVE-2020-10062, tracked by Zephyr as ZEPSEC-54, and per the report's own patch table it was fixed in v2.2.0. NVD's entry for the same CVE says affected versions are "Zephyr version 2.2.0 and later", naming as vulnerable the release that carries the fix. Go with the report and the patch table.

The syscall number check#

Zephyr has an optional user-space mode. Turn on CONFIG_USERSPACE and threads run unprivileged, reaching the kernel through a syscall interface, same idea as any other OS. On ARM, a syscall is an SVC instruction with the call number in r6. The exception handler validates it:

ldr ip, =K_SYSCALL_LIMIT
cmp r6, ip
blt valid_syscall_id

BLT is the signed branch. Call number 0xF0000000 is a large unsigned number and a very negative signed one. It sails through the check as comfortably below the limit. What happens next is that z_arm_do_syscall uses the number as an index into _k_syscall_table. It shifts the number left by two, adds the table base, loads whatever is at that address, and calls it.

The proof of concept is a few lines of C:

static void user(void *p1, void *p2, void *p3) {
    print_control("user");
    __asm__ volatile (
        "mov r6, %0\n"
        "svc 3\n" :: "r"(-0x10000000) : "r6"
    );
}

That produces a bus fault at 0xc0006140 on the test board. It is the address the out-of-bounds index lands on: 0xf0000000 shifted left by two, added to the table base. The kernel faults reading the entry. Nothing exciting on its own. But an unprivileged thread that knows the memory layout gets to choose the address, and the kernel calls it in supervisor mode. That's the whole of the user/kernel boundary, gone, on any Zephyr build with user space enabled.

The same BLT shows up on the ARC architecture too, in arch/arc/core/fault_s.S rather than an SVC handler. NCC Group's recommendation is one instruction: use BCC on ARM and BLO on ARC, both of which are the unsigned form. The two architectures got separate CVE numbers. CVE-2020-10024 covers ARM and CVE-2020-10027 covers ARC, fixed in v1.14.2, v2.1.0 and v2.2.0.

Two of the 26 findings, then: the wrong relational operator in one, the wrong signedness in the other.

The disclosure timeline is my favourite part of the report. It opens on 18 February 2020, with NCC Group joining the Zephyr Slack to ask how to report a vulnerability. The wiki documentation was out of date and didn't link to the Jira instance. Two days later they still can't file through Jira and are told to email the PSIRT address instead. The report is sent on 24 February. Six days from wanting to report bugs to being able to, before anyone has looked at a single finding.

After that it moves quickly. Zephyr confirms receipt in two days, starts patching within a week, and ships v2.2.0 on 10 March with the first batch of fixes in it.