How integer overflow in bounds checking happens in C and how to fix it

Introduction

In the DOOM source port's WAD file handling code, we discovered a critical integer overflow vulnerability in DOOM/w_file.c at line 93. The W_Read function is responsible for safely reading data from memory-mapped WAD files, but a subtle flaw in its bounds checking logic allowed attackers to bypass validation entirely using carefully crafted offset values.

The vulnerable code attempted to prevent out-of-bounds reads with what appeared to be a straightforward check: if (offset + buffer_len > wad->length). However, this arithmetic operation is susceptible to integer overflow when offset approaches UINT_MAX, causing the sum to wrap around to a small value and bypass the protection.

This vulnerability matters for any developer working with binary file formats, network protocols, or memory-mapped I/O where untrusted input controls offset and length values.

The Vulnerability Explained

The W_Read function reads data from a memory-mapped WAD file into a buffer. Before performing the read, it needs to ensure the requested range [offset, offset + buffer_len) falls within the valid file bounds. Here's the vulnerable code:

size_t W_Read(wad_file_t *wad, unsigned int offset,
              void *buffer, size_t buffer_len)
{
    if (offset + buffer_len > wad->length)
        buffer_len = wad->length - offset;

    memcpy(buffer, wad->mapped + offset, buffer_len);
    return buffer_len;
}

The problem lies in the expression offset + buffer_len. When offset is a value like 0xFFFFFFFF (UINT_MAX on 32-bit) and buffer_len is 2, the addition wraps around:

0xFFFFFFFF + 2 = 0x00000001 (due to overflow)

If wad->length is, say, 100, the check becomes 1 > 100, which is false. The bounds check passes, and the code proceeds to execute memcpy(buffer, wad->mapped + 0xFFFFFFFF, 2), attempting to read from an address far beyond the mapped file.

Attack Scenario

An attacker crafts a malicious WAD file with manipulated lump directory entries. WAD files contain a directory that specifies offsets and sizes for each data "lump." By setting a lump's offset to a value near UINT_MAX, the attacker can:

1. Bypass bounds validation through integer overflow
2. Read arbitrary memory beyond the WAD file's mapped region
3. Cause a crash if the address is unmapped, or leak sensitive data if it's accessible

This is particularly dangerous because WAD files are commonly shared and loaded from untrusted sources (custom game mods, downloaded content, etc.).

Secondary Issue: Integer Underflow

There's also a subtler problem. If offset > wad->length and the check somehow passes, the "correction" buffer_len = wad->length - offset would underflow, producing a massive buffer_len value and causing memcpy to read far more data than intended.

The Fix

The fix restructures the bounds checking to avoid arithmetic that can overflow. Instead of computing offset + buffer_len, it uses two separate checks:

size_t W_Read(wad_file_t *wad, unsigned int offset,
              void *buffer, size_t buffer_len)
{
    if (offset >= wad->length)
        return 0;

    if (buffer_len > wad->length - offset)
        buffer_len = wad->length - offset;

    memcpy(buffer, wad->mapped + offset, buffer_len);
    return buffer_len;
}

Before and After Comparison

Why This Works

1. First check: if (offset >= wad->length) ensures that offset is strictly within bounds. If the offset is at or beyond the file length, we return 0 immediately—there's nothing to read.

2. Second check: Only after confirming offset < wad->length do we compute wad->length - offset. This subtraction is now guaranteed not to underflow because we know wad->length > offset.

3. Safe comparison: buffer_len > wad->length - offset compares without addition, eliminating the overflow risk entirely.

The fix also includes a comprehensive regression test (tests/test_invariant_w_file.c) that validates the security invariant: "W_Read must never read beyond wad->length regardless of offset/buffer_len values." The test covers:

• Integer overflow bypass attempts (UINT32_MAX offset)
• Boundary conditions (offset equals length)
• Underflow scenarios (offset exceeds length)
• Near-overflow boundary cases

Prevention & Best Practices

Safe Integer Arithmetic Patterns

When performing bounds checks in C, follow these principles:

1. Validate inputs independently before combining them
   ```c
   // Bad: can overflow
   if (a + b > limit)

// Good: check separately
if (a > limit || b > limit - a)
```

1. Use subtraction instead of addition when possible
   c // Instead of: offset + len > max // Use: len > max - offset (after validating offset < max)

2. Consider using safe integer libraries
   - GCC's __builtin_add_overflow()
   - Microsoft's safe_int library
   - Custom checked arithmetic macros

Compiler Protections

Enable compiler flags that help detect integer issues:
- -ftrapv: Trap on signed overflow (GCC/Clang)
- -fsanitize=integer: Runtime integer overflow detection
- -Wconversion: Warn on implicit conversions that may lose data

Code Review Checklist

When reviewing bounds-checking code, ask:
- Can any arithmetic operation overflow?
- Are all operands validated before use?
- What happens at boundary values (0, MAX, MAX-1)?
- Is the order of operations safe?

Key Takeaways

• Never trust arithmetic in bounds checks: The expression offset + buffer_len > limit is a classic integer overflow pattern that appears safe but fails at boundary values.
• Validate offset before computing remaining space: Always check offset < length before computing length - offset to prevent underflow.
• WAD file parsers must treat directory entries as untrusted: Offset and size values from file headers can be attacker-controlled.
• Regression tests should include overflow boundary cases: Test with UINT_MAX, UINT_MAX - small_value, and values that would cause wraparound.
• The W_Read function now implements defense-in-depth: Two independent checks ensure neither overflow nor underflow can bypass bounds validation.

How Orbis AppSec Detected This

• Source: WAD file lump directory entries containing offset and size values parsed from untrusted input files
• Sink: memcpy(buffer, wad->mapped + offset, buffer_len) in DOOM/w_file.c:97
• Missing control: No validation that offset + buffer_len arithmetic wouldn't overflow before comparison
• CWE: CWE-190 (Integer Overflow or Wraparound)
• Fix: Split the bounds check into two separate validations—first ensuring offset is within bounds, then safely computing the maximum readable length

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

Integer overflow vulnerabilities in bounds checking are insidious because the code often looks correct at first glance. The pattern if (a + b > limit) is intuitive but dangerous when a and b can be controlled by an attacker. The fix demonstrated here—separating validation into independent checks and using subtraction after validation—is a robust pattern applicable to any code handling untrusted offsets and lengths.

When working with binary file formats, memory-mapped I/O, or any code where arithmetic values come from external sources, always consider what happens at the boundaries of your data types. A few extra lines of defensive code can prevent critical memory safety vulnerabilities.

References

• CWE-190: Integer Overflow or Wraparound
• OWASP Integer Overflow Guidelines
• SEI CERT C Coding Standard - INT32-C
• Semgrep Integer Overflow Rules
• fix: add integer overflow check in w_file.c