How buffer overflow in SCSI command handling happens in C and how to fix it

Introduction

In the libretro-common repository, we discovered a critical buffer overflow vulnerability in libretro-common/cdrom/cdrom.c at line 305. The cdrom_send_command_win32() function copies SCSI commands into a fixed-size buffer without validating the input length, creating a textbook example of CWE-120: Buffer Copy without Checking Size of Input.

The vulnerable code pattern is deceptively simple:

memcpy(sptd.s.Cdb, cmd, cmd_len);

This single line copies cmd_len bytes from the cmd parameter into sptd.s.Cdb, a SCSI Command Descriptor Block buffer that's typically only 16 bytes. The problem? There's no check to ensure cmd_len doesn't exceed 16 bytes before the copy happens.

This matters because CDROM handling code processes data from external sources—disc images, USB devices, and potentially network-mounted drives. If an attacker can control the SCSI command length, they can overflow the CDB buffer and corrupt adjacent memory structures in the sptd_with_sense struct, potentially leading to arbitrary code execution.

The Vulnerability Explained

Let's examine the vulnerable code in detail. The cdrom_send_command_win32() function at line 302-305 constructs a SCSI Pass-Through Direct (SPTD) structure:

sptd.s.SenseInfoLength = sizeof(sptd.sense);
sptd.s.SenseInfoOffset = offsetof(struct sptd_with_sense, sense);

memcpy(sptd.s.Cdb, cmd, cmd_len);  // Line 305 - VULNERABLE

The sptd.s.Cdb field is defined by the Windows SCSI_PASS_THROUGH_DIRECT structure, where the CDB (Command Descriptor Block) is a fixed array of 16 bytes. This is a standard size for SCSI commands in the Windows API.

The attack vector: An attacker creates a malicious CDROM image or connects a specially crafted USB CDROM device. When the application attempts to read from this device, it triggers SCSI command construction with a cmd_len value greater than 16. For example:

1. Attacker provides a CDROM image that reports an unusual disc format
2. The application attempts to query the device capabilities
3. Malicious firmware or image metadata causes cmd_len to be set to 32 bytes
4. The memcpy() at line 305 copies 32 bytes into a 16-byte buffer
5. The overflow overwrites the adjacent SenseInfoLength and SenseInfoOffset fields
6. Corrupted sense info pointers can lead to further memory corruption when DeviceIoControl processes the malformed SPTD structure

Real-world impact: This isn't just theoretical. The libretro-common library is used in RetroArch and numerous emulation projects that handle disc images from untrusted sources. Users downloading CDROM images from the internet could unknowingly trigger this vulnerability. The scanner confirmed this is in production code, not test-only code, making it a genuine threat to end users.

The PR description notes that similar patterns exist at lines 307, 459, 502, 510, and 987, suggesting this is a systemic issue in the codebase that requires broader review.

The Fix

The fix is elegantly simple—a two-line bounds check before the memcpy operation:

Before (vulnerable code at line 305):

sptd.s.SenseInfoLength = sizeof(sptd.sense);
sptd.s.SenseInfoOffset = offsetof(struct sptd_with_sense, sense);

memcpy(sptd.s.Cdb, cmd, cmd_len);

After (fixed code at lines 305-307):

sptd.s.SenseInfoLength = sizeof(sptd.sense);
sptd.s.SenseInfoOffset = offsetof(struct sptd_with_sense, sense);

if (cmd_len > sizeof(sptd.s.Cdb))
   return 1;
memcpy(sptd.s.Cdb, cmd, cmd_len);

How this solves the problem: The added bounds check validates that cmd_len does not exceed sizeof(sptd.s.Cdb) (which is 16 bytes) before performing the copy. If an oversized command length is detected, the function immediately returns an error code (1) without executing the memcpy.

This approach has several security advantages:

1. Fail-safe behavior: Rather than attempting to truncate or clamp the command, the function rejects invalid input entirely
2. Clear security invariant: The code explicitly states "cmd_len must not exceed CDB size"
3. No performance impact: A single comparison instruction adds negligible overhead
4. Portable solution: Works on all platforms without requiring memcpy_s() or other platform-specific APIs

The fix also includes a comprehensive regression test in tests/test_invariant_cdrom.c that validates the security invariant:

START_TEST(test_cdrom_cmd_len_bounds)
{
    unsigned char cmd[64];
    memset(cmd, 0x41, sizeof(cmd));

    size_t test_lens[] = {
        32,   /* exploit case: double the CDB buffer size */
        17,   /* boundary: one byte over max */
        12,   /* valid: standard SCSI CDB length */
    };

    for (int i = 0; i < num_cases; i++)
    {
        size_t cmd_len = test_lens[i];
        int ret = cdrom_send_command(fds, DIRECTION_NONE, buf, sizeof(buf),
                                     cmd, cmd_len, 0);

        if (cmd_len > SCSI_CDB_MAX_SIZE)
        {
            ck_assert_int_ne(ret, 0);  // Must return error
        }
    }
}

This test ensures that any future modifications to the CDROM code cannot reintroduce the vulnerability without breaking the test suite.

Prevention & Best Practices

To prevent buffer overflow vulnerabilities in SCSI and low-level I/O code:

1. Always validate buffer sizes before copying: Never assume input lengths are within expected bounds. Use explicit checks like if (src_len > dest_size) return ERROR; before memcpy operations.

2. Use sizeof() for buffer dimensions: In the fix, sizeof(sptd.s.Cdb) ensures the check remains correct even if the structure definition changes. Avoid hardcoded magic numbers like if (cmd_len > 16).

3. Prefer bounds-checked functions: Where available, use memcpy_s(), strncpy_s(), or similar functions that include explicit destination buffer size parameters. However, ensure these are available in your target environment.

4. Define maximum sizes as constants: The test code defines SCSI_CDB_MAX_SIZE as 16. Using named constants makes the security requirement explicit and searchable.

5. Review similar patterns: The PR notes that lines 307, 459, 502, 510, and 987 use similar patterns. When fixing one buffer overflow, audit the entire codebase for related instances using grep or static analysis.

6. Add security regression tests: The included test guards against future regressions by codifying the security invariant: "cmd_len must never exceed SCSI_CDB_MAX_SIZE."

7. Static analysis integration: Tools like Semgrep, CodeQL, and commercial SAST solutions can detect unchecked memcpy patterns. The multi_agent_ai scanner that detected this issue demonstrates the value of automated security scanning in CI/CD pipelines.

OWASP guidance: The OWASP Secure Coding Practices recommend input validation at trust boundaries. CDROM devices and disc images are untrusted inputs that must be validated before processing.

CWE-120 mitigation: The CWE-120 entry recommends "assume all input is malicious" and "validate the size of the input before copying." This fix exemplifies both principles.

Key Takeaways

• Never trust SCSI command lengths from external sources: The cmd_len parameter in cdrom_send_command_win32() comes from CDROM devices or disc images and must be validated before copying into fixed-size buffers.

• The sptd.s.Cdb buffer is exactly 16 bytes: This is defined by the Windows SCSI_PASS_THROUGH_DIRECT API structure. Any memcpy into this buffer must check that the source length doesn't exceed 16 bytes.

• A single unchecked memcpy can compromise memory safety: Line 305's vulnerability demonstrates how one missing bounds check in low-level I/O code can create a critical security flaw exploitable through malicious hardware or disc images.

• Security invariants should be explicit in code: The fix makes the requirement "cmd_len ≤ sizeof(sptd.s.Cdb)" explicit through a visible if-statement rather than relying on implicit assumptions about input data.

• Similar patterns at lines 307, 459, 502, 510, and 987 need review: This vulnerability likely exists in multiple locations in cdrom.c, requiring a comprehensive audit of all memcpy operations involving SCSI command buffers.

How Orbis AppSec Detected This

• Source: The cmd and cmd_len parameters in cdrom_send_command_win32() originate from CDROM device queries and disc image parsing, representing untrusted external input.
• Sink: The dangerous call site is memcpy(sptd.s.Cdb, cmd, cmd_len) at libretro-common/cdrom/cdrom.c:305, which copies variable-length data into a fixed 16-byte buffer.
• Missing control: No validation exists to ensure cmd_len does not exceed sizeof(sptd.s.Cdb) before the copy operation.
• CWE: CWE-120 (Buffer Copy without Checking Size of Input) - copying data to a buffer without verifying the source size fits within the destination.
• Fix: Added a bounds check if (cmd_len > sizeof(sptd.s.Cdb)) return 1; before the memcpy to reject oversized commands.

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

This buffer overflow in libretro-common's CDROM handling code demonstrates how a single missing bounds check can create a critical security vulnerability. The fix—a simple two-line validation before memcpy—prevents memory corruption from malicious CDROM devices or disc images. By explicitly checking that cmd_len doesn't exceed the 16-byte SCSI CDB buffer size, the code now maintains its security invariant even under adversarial input.

The key lesson is that all external input, especially from hardware devices and file formats, must be validated before copying into fixed-size buffers. Static analysis tools can detect these patterns automatically, but the ultimate responsibility lies with developers to understand buffer boundaries and enforce them in code. The included regression test ensures this vulnerability cannot silently reappear in future code changes.

If you're working with low-level I/O, SCSI commands, or any code that handles untrusted input, audit your memcpy and strcpy calls today. Your users' security depends on it.

References

• CWE-120: Buffer Copy without Checking Size of Input
• CWE-787: Out-of-bounds Write
• OWASP Secure Coding Practices: Input Validation
• Microsoft SCSI_PASS_THROUGH_DIRECT structure documentation
• Semgrep rules for unsafe memcpy patterns
• fix: add bounds check before memcpy in cdrom.c