How buffer overflow in rcdevice.c request parser happens in C and how to fix it

How Buffer Overflow in rcdevice.c Request Parser Happens in C and How to Fix It

Summary

A critical buffer overflow vulnerability was discovered in src/main/io/rcdevice.c at line 489, inside the rcdeviceReceive() function's request parser state machine. The parser wrote incoming bytes directly into requestParserContext.request.data[] using only a single runtime bound—requestParserContext.expectedDataLength—with no check against the compile-time array capacity RCDEVICE_PROTOCOL_MAX_DATA_SIZE. An attacker controlling the device's I/O data stream could overflow the buffer, corrupt adjacent memory, and potentially achieve arbitrary code execution. The fix adds a hard upper bound so writes are always capped at the actual allocated buffer size.

Introduction

The src/main/io/rcdevice.c file is the heart of RC (remote control) device communication in this firmware. It implements a byte-by-byte state machine in rcdeviceReceive() that parses an incoming data stream from the device's I/O interface—reading packet headers, lengths, and payloads character by character as they arrive. This kind of incremental parser is common in embedded C firmware, and it's also one of the most historically dangerous patterns in systems programming.

The flaw lives in the RCDEVICE_STATE_WAITING_DATA branch of that state machine, at line 489. Here's exactly what the vulnerable code looked like before the fix:

case RCDEVICE_STATE_WAITING_DATA:
    if (requestParserContext.request.dataLength < requestParserContext.expectedDataLength) {
        requestParserContext.request.data[requestParserContext.request.dataLength] = c;
        requestParserContext.request.dataLength++;
    }

At first glance this looks safe—it checks dataLength before writing. But there's a subtle and critical problem: the guard uses expectedDataLength, a runtime value derived from the incoming data stream, not the compile-time size of the data[] array. If those two values diverge—through a malformed packet, a protocol bug, or deliberate attacker manipulation—bytes get written past the end of the buffer.

The Vulnerability Explained

What Goes Wrong

In C, arrays are fixed-size allocations. requestParserContext.request.data is declared with a maximum size defined by the constant RCDEVICE_PROTOCOL_MAX_DATA_SIZE. That constant is the true upper bound—it's the number of bytes the compiler actually reserved.

requestParserContext.expectedDataLength, on the other hand, is populated at runtime from the parsed packet header. It tells the parser how many bytes this particular packet claims to contain. In a well-formed, trusted data stream these two values align. But the parser has no way to verify that the incoming stream is trustworthy before using expectedDataLength as its sole write guard.

Here's the exploit scenario made concrete:

1. An attacker sends a crafted byte stream to the device's I/O interface.
2. The packet header encodes an expectedDataLength value larger than RCDEVICE_PROTOCOL_MAX_DATA_SIZE.
3. The state machine transitions to RCDEVICE_STATE_WAITING_DATA.
4. The condition requestParserContext.request.dataLength < requestParserContext.expectedDataLength remains true even after the buffer is full.
5. Each subsequent byte is written one position further past the end of data[], overwriting adjacent stack or heap memory.

The PR description captures this precisely: "Send data stream longer than requestParserContext.expectedDataLength to the device's I/O interface, causing buffer overflow and potential code execution."

Why This Is Especially Dangerous in Embedded Firmware

In embedded environments like this one, stack layouts are often predictable, ASLR may be absent or weak, and the memory adjacent to requestParserContext.request.data likely contains other fields of the RequestParserContext struct—including state, expectedDataLength, and function pointers or return addresses. Overwriting state could redirect the parser into an undefined state. Overwriting a return address could redirect execution entirely.

Even if full code execution is not immediately achievable, corrupting expectedDataLength or dataLength itself could create a persistent feedback loop that keeps writing indefinitely—a denial-of-service at minimum.

The Fix

The fix is surgical and correct. It adds a second, independent bounds check against RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1, the compile-time buffer capacity:

Before

case RCDEVICE_STATE_WAITING_DATA:
    if (requestParserContext.request.dataLength < requestParserContext.expectedDataLength) {
        requestParserContext.request.data[requestParserContext.request.dataLength] = c;
        requestParserContext.request.dataLength++;
    }

After

case RCDEVICE_STATE_WAITING_DATA:
    if (requestParserContext.request.dataLength < requestParserContext.expectedDataLength &&
        requestParserContext.request.dataLength < (RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1)) {
        requestParserContext.request.data[requestParserContext.request.dataLength] = c;
        requestParserContext.request.dataLength++;
    }

Why This Works

The && operator means both conditions must be true before any byte is written. The second condition—requestParserContext.request.dataLength < (RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1)—is a hard ceiling derived from the actual array declaration. It cannot be spoofed by the incoming data stream because it references a compile-time constant, not a runtime variable.

Notice the use of RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1 rather than RCDEVICE_PROTOCOL_MAX_DATA_SIZE. This is a deliberate off-by-one safety margin. If the array is declared as char data[RCDEVICE_PROTOCOL_MAX_DATA_SIZE], valid indices run from 0 to RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1. The check dataLength < (RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1) ensures that after the write and increment, dataLength never reaches the array's length—leaving room for a null terminator or preventing an exact-boundary edge case.

This is the defense-in-depth pattern for C buffer management: validate against protocol expectations and validate against physical memory limits, independently.

Prevention & Best Practices

1. Always Use Two Independent Bounds Checks for Protocol Parsers

Any time you write into a fixed buffer based on a length field from a data stream, you need two guards:
- One against the protocol-level expected length (what the packet says)
- One against the compile-time buffer capacity (what the compiler allocated)

// Pattern: dual-bound write guard
if (ctx.dataLength < ctx.expectedLength &&
    ctx.dataLength < (BUFFER_MAX_SIZE - 1)) {
    buffer[ctx.dataLength++] = incoming_byte;
}

2. Treat All Length Fields from External Sources as Untrusted

expectedDataLength is populated from the incoming byte stream. That makes it attacker-controlled input. Apply the same skepticism you would to any user-supplied value: validate it against known-good ranges before using it as a loop or write bound.

// Validate expectedDataLength before entering WAITING_DATA state
if (parsed_length > RCDEVICE_PROTOCOL_MAX_DATA_SIZE) {
    // Reject malformed packet
    requestParserContext.state = RCDEVICE_STATE_ERROR;
    break;
}
requestParserContext.expectedDataLength = parsed_length;

3. Use static_assert to Document Buffer Size Invariants

In C11 and later, you can use static_assert to make buffer size relationships explicit and compiler-checked:

static_assert(RCDEVICE_PROTOCOL_MAX_DATA_SIZE > 0,
    "Buffer size must be positive");
static_assert(sizeof(requestParserContext.request.data) == RCDEVICE_PROTOCOL_MAX_DATA_SIZE,
    "data array must match protocol max size constant");

4. Enable Compiler and Sanitizer Protections

• -fstack-protector-strong: Adds canaries around stack buffers to detect overwrites at runtime.
• AddressSanitizer (-fsanitize=address): Catches out-of-bounds writes during development and testing.
• -D_FORTIFY_SOURCE=2: Enables glibc's compile-time and runtime buffer overflow checks for standard library calls.

5. Write Regression Tests That Probe Boundary Conditions

The PR includes an excellent regression test that sends three payloads: one short, one exactly at expectedDataLength, and one byte over. This is the right approach—test the boundary, the exact boundary, and one past it. The test verifies the security property directly:

ck_assert_msg(ctx.request.dataLength <= ctx.expectedDataLength,
             "Buffer overflow: dataLength=%d exceeds expectedDataLength=%d",
             ctx.request.dataLength, ctx.expectedDataLength);

Relevant Standards

• CWE-121: Stack-based Buffer Overflow
• CWE-120: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow')
• OWASP: Buffer Overflow
• CERT C: Rule ARR38-C — Guarantee that library functions do not form invalid pointers

Key Takeaways

• expectedDataLength is attacker-controlled data: It comes from the incoming byte stream in rcdeviceReceive(). Never use a protocol-supplied length as the sole guard on a buffer write.
• RCDEVICE_PROTOCOL_MAX_DATA_SIZE is the true safety ceiling: The compile-time constant reflects what the compiler actually allocated. It must always be part of the write guard.
• The fix is additive, not structural: Adding && requestParserContext.request.dataLength < (RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1) required changing exactly two lines while preserving all existing parser logic.
• Off-by-one margins matter: Using RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1 rather than the raw size prevents exact-boundary edge cases and leaves room for null termination.
• State machine parsers in embedded C need dual-bound guards as a pattern: This isn't a one-off fix—apply the same pattern to every buffer write in every state of rcdeviceReceive() and similar parsers.

How Orbis AppSec Detected This

• Source: Incoming byte c read from the RC device's I/O interface in rcdeviceReceive(), with expectedDataLength populated from the packet header—both are attacker-controllable.
• Sink: requestParserContext.request.data[requestParserContext.request.dataLength] = c at src/main/io/rcdevice.c:489—a direct array write with no compile-time bound check.
• Missing control: No validation of requestParserContext.request.dataLength against RCDEVICE_PROTOCOL_MAX_DATA_SIZE before the write. The only guard used a runtime value derived from untrusted input.
• CWE: CWE-121 — Stack-based Buffer Overflow
• Fix: Added && requestParserContext.request.dataLength < (RCDEVICE_PROTOCOL_MAX_DATA_SIZE - 1) as a second condition in the RCDEVICE_STATE_WAITING_DATA write guard, capping all writes at the compile-time buffer capacity.

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The buffer overflow in rcdeviceReceive() is a textbook example of why protocol-supplied length values can never be trusted as the sole guard on a buffer write in C. The vulnerable code in RCDEVICE_STATE_WAITING_DATA looked reasonable in isolation—it did check dataLength before writing—but it checked against the wrong bound. By anchoring the write guard to RCDEVICE_PROTOCOL_MAX_DATA_SIZE, the fix makes the safety property hold unconditionally, regardless of what any incoming data stream claims its length to be.

For developers writing state machine parsers in C—especially in embedded firmware where memory protections may be limited—the lesson is clear: every buffer write needs two guards. One for protocol semantics, one for physical memory limits. Neither alone is sufficient.

References

• CWE-121: Stack-based Buffer Overflow
• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow Vulnerability
• OWASP Input Validation Cheat Sheet
• CERT C Rule ARR38-C
• Semgrep rules for buffer overflow in C
• fix: the request parser writes data into a fixed-siz... in rcdevice.c