How heap buffer overflow happens in C++ JPEG2000 decoding and how to fix it

Introduction

In the jp2forandroid repository, we discovered a critical heap buffer overflow vulnerability in openjpg.cpp at line 597. The opj_read_from_byte_array() function—responsible for reading bytes from a JPEG2000 image buffer during decoding—contained a dangerous flaw that could allow attackers to execute arbitrary code through specially crafted image files.

The vulnerability exists because the code performs a memcpy() operation using an offset value (p_user_data->offset) without first checking whether that offset has already exceeded the buffer's total length. When processing a malicious JP2 image with manipulated header fields, an attacker could cause the offset to grow beyond the buffer size, leading to out-of-bounds memory reads and heap corruption.

For Android developers working with image processing libraries, this vulnerability demonstrates why every buffer operation—no matter how simple it appears—requires defensive bounds checking.

The Vulnerability Explained

The vulnerable code resided in the opj_read_from_byte_array() function, which serves as a custom stream reader for the OpenJPEG library:

static OPJ_SIZE_T opj_read_from_byte_array (void * p_buffer, OPJ_SIZE_T p_nb_bytes, opj_byte_array_source * p_user_data)
{
    //LOGD("opj_read_from_byte_array started");
    size_t toRead = MIN(p_nb_bytes, p_user_data->length - p_user_data->offset);
    memcpy(p_buffer, p_user_data->data + p_user_data->offset, toRead);
    p_user_data->offset += toRead;
    return toRead;
}

At first glance, the MIN() macro seems protective—it calculates the smaller of the requested bytes and the remaining buffer space. However, there's a critical flaw: what happens when p_user_data->offset is already greater than or equal to p_user_data->length?

The Integer Underflow Problem

When offset >= length, the expression p_user_data->length - p_user_data->offset produces an integer underflow. Since these are unsigned integers, the result wraps around to a massive positive number rather than becoming negative. The MIN() then selects p_nb_bytes (the requested read size), and memcpy() proceeds to read far beyond the allocated buffer.

Attack Scenario

An attacker crafts a JPEG2000 image with manipulated SIZ marker box fields. Here's how the attack unfolds:

1. Malicious Image Creation: The attacker creates a JP2 file where the SIZ marker declares component dimensions that cause multiple sequential reads
2. Offset Manipulation: Through carefully crafted header values, the decoder is tricked into advancing p_user_data->offset beyond p_user_data->length
3. Buffer Overflow Trigger: On the next read operation, the integer underflow causes memcpy() to read arbitrary heap memory
4. Code Execution: The attacker can corrupt adjacent heap structures, potentially achieving arbitrary code execution

This is particularly dangerous on Android, where a user might simply open a shared image file, triggering the vulnerable code path without any additional interaction.

Additional Vulnerability: Integer Overflow in Allocation

A second issue existed in the imagetoargb() function at line 169:

outImage->pixels = (int *) malloc(sizeof(int) * w * h);

If an attacker provides manipulated width (w) and height (h) values, the multiplication sizeof(int) * w * h could overflow, resulting in a small allocation that's later written with a much larger amount of data.

The Fix

The fix introduces two critical safeguards:

Fix 1: Bounds Check Before Read

static OPJ_SIZE_T opj_read_from_byte_array (void * p_buffer, OPJ_SIZE_T p_nb_bytes, opj_byte_array_source * p_user_data)
{
    //LOGD("opj_read_from_byte_array started");
    if (p_user_data->offset >= p_user_data->length) {
        return (OPJ_SIZE_T)-1;
    }
    size_t toRead = MIN(p_nb_bytes, p_user_data->length - p_user_data->offset);
    memcpy(p_buffer, p_user_data->data + p_user_data->offset, toRead);
    p_user_data->offset += toRead;
    return toRead;
}

The new check if (p_user_data->offset >= p_user_data->length) ensures that:
- We never attempt to read when the offset has reached or exceeded the buffer length
- The subtraction length - offset is always safe (no underflow possible)
- The function returns an error code (-1) to signal the end of available data

Fix 2: Integer Overflow Protection in Allocation

outImage->pixels = (w > 0 && h > 0 && h <= SIZE_MAX / (sizeof(int) * w)) 
    ? (int *) malloc(sizeof(int) * w * h) 
    : NULL;

This change validates that:
- Both dimensions are positive
- The multiplication won't overflow SIZE_MAX
- If validation fails, NULL is returned instead of allocating a dangerously small buffer

Before and After Comparison

Prevention & Best Practices

Defensive Coding for Buffer Operations

1. Always validate offsets before arithmetic: Check that your offset is within bounds before using it in any calculation
2. Use safe integer arithmetic: Consider using compiler built-ins like __builtin_add_overflow() or libraries that detect overflow
3. Validate all size parameters from untrusted input: Image dimensions, chunk sizes, and length fields in file formats are attacker-controlled

Code Review Checklist

When reviewing C/C++ code that handles buffers:

• [ ] Is every memcpy(), memmove(), or strcpy() preceded by a bounds check?
• [ ] Are all arithmetic operations checked for overflow before use in allocations?
• [ ] Do offset variables get validated before use in pointer arithmetic?
• [ ] Are error conditions properly handled (not silently ignored)?

Tools for Detection

• AddressSanitizer (ASan): Compile with -fsanitize=address to detect buffer overflows at runtime
• Static analyzers: Tools like Coverity, PVS-Studio, or clang-tidy can identify missing bounds checks
• Fuzzing: Use AFL or libFuzzer with malformed image inputs to trigger edge cases

Key Takeaways

• Never trust that previous operations kept offsets valid: The opj_read_from_byte_array() function assumed the offset would always be less than length—a dangerous assumption when processing untrusted data
• Integer underflow is as dangerous as overflow: The expression length - offset with unsigned integers can produce massive values when offset exceeds length
• Image parsing is a high-risk area: JPEG2000, PNG, and other image formats have complex structures that attackers frequently exploit
• Multi-step attacks are common: This vulnerability required manipulating header fields to first advance the offset, then trigger the overflow—exactly the "2-step chain" the scanner identified
• Bounds checks must come before arithmetic, not after: Placing the validation after the MIN() calculation would still allow the underflow to occur

How Orbis AppSec Detected This

• Source: Untrusted data from JP2 image file header fields (SIZ marker box dimensions)
• Sink: memcpy(p_buffer, p_user_data->data + p_user_data->offset, toRead) in openjpg.cpp:601
• Missing control: No validation that p_user_data->offset < p_user_data->length before the subtraction operation
• CWE: CWE-120 (Buffer Copy without Checking Size of Input)
• Fix: Added bounds check if (p_user_data->offset >= p_user_data->length) before the memcpy operation and integer overflow protection in the malloc call

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 heap buffer overflow in opj_read_from_byte_array() demonstrates how a seemingly simple oversight—failing to validate an offset before subtraction—can create a critical security vulnerability. The fix required just three lines of code, but those three lines prevent attackers from exploiting malicious JPEG2000 images to execute arbitrary code on Android devices.

When working with native code that processes untrusted input, remember: every buffer operation is a potential attack surface. Validate early, validate often, and never assume that previous operations have left your state in a safe condition.

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow Prevention Cheat Sheet
• Semgrep Rules for Buffer Overflow
• fix: add buffer-length check in openjpg.cpp
• OpenJPEG Security Advisories