XShmGetImage Heap Corruption: Unvalidated Image Height in streamproxy

The Vulnerability Explained

The XShmGetImage function retrieves pixel data from an X11 drawable (window or pixmap) using shared memory for efficiency. During image transfer, the function allocates a destination buffer and copies rows of pixel data using memcpy in a loop.

The vulnerable code iterates based on the source image height without checking whether the source is larger than the destination:

for (unsigned int r = 0; r < (unsigned int)im->height; r++)
    memcpy(image->data + (size_t)r * image->bytes_per_line,
           im->data + (size_t)r * im->bytes_per_line, copy);

Here, im is the source image returned by the X server, and image is the pre-allocated destination buffer. If im->height is larger than image->height, the loop writes past the end of the destination buffer on each row iteration, corrupting heap memory.

Attack Scenario

Consider a video conferencing application that fetches window thumbnails via XShmGetImage:

1. The application allocates a destination buffer for a 480×360 image.
2. A compromised or attacker-controlled X server returns an XImage struct with height = 1200.
3. The loop iterates 1200 times, writing 1200 rows of pixel data into a buffer sized for 360 rows.
4. Each of the extra 840 writes corrupts adjacent heap objects.

An attacker with X server control (or network interception on an unencrypted X display connection) can trigger this without privilege escalation.

Affected Versions

The vulnerability exists in the XShmGetImage implementation in the zcall-bridge streaming proxy. No version number is assigned; this is a first-party fix applied via pull request review.

The Fix

The fix introduces a critical validation step: compare both heights before entering the loop, and iterate only up to the minimum.

Before:

size_t copy = im->bytes_per_line < image->bytes_per_line
                  ? im->bytes_per_line
                  : image->bytes_per_line;
for (unsigned int r = 0; r < (unsigned int)im->height; r++)
    memcpy(image->data + (size_t)r * image->bytes_per_line,
           im->data + (size_t)r * im->bytes_per_line, copy);

After:

size_t copy = im->bytes_per_line < image->bytes_per_line
                  ? (size_t)im->bytes_per_line
                  : (size_t)image->bytes_per_line;
unsigned int rows = (unsigned int)im->height < (unsigned int)image->height
                        ? (unsigned int)im->height
                        : (unsigned int)image->height;
for (unsigned int r = 0; r < rows; r++)
    memcpy(image->data + (size_t)r * image->bytes_per_line,
           im->data + (size_t)r * im->bytes_per_line, copy);

Why This Works

1. Height comparison: A new variable rows holds the minimum of source and destination heights. The loop will never exceed the destination buffer's actual row count.
2. Early type casting: Both im->height and image->height are cast to unsigned int at assignment time, preventing any type confusion or sign-extension bugs during the comparison.
3. Byte-width safety: The copy variable (bytes per row) was already capped to the smaller of the two; now the iteration count is also capped, creating a complete bounds check.

The fix is minimal and focused: it does not alter the API, does not allocate additional memory, and does not change behavior for validly-sized images.

Key Takeaways

• Validate both dimensions when copying 2D buffers: When memcpy is called in a loop (row-by-row, chunk-by-chunk), validate both the loop count AND the byte count against the destination size. A single dimension check is insufficient.
• Never trust height or width from untrusted sources: X server responses, network image headers, or API responses can be crafted. Always compare against the destination allocation before use.
• Type mismatches enable overflow: The original code mixed signed and unsigned comparisons without consistent casting. Always cast to the same type before boundary checks to avoid wraparound or sign-extension issues.
• Shared memory APIs are high-risk: XShm and similar zero-copy mechanisms skip normal marshalling checks. Code using them must add extra validation where normal copy APIs would fail safely.

How Orbis AppSec Detected This

Source: The X server response, specifically the im->height field in the XImage struct returned by XGetImage.

Sink: The memcpy call inside the row-copy loop, invoked with a loop bound derived from im->height.

Missing control: No validation that im->height does not exceed image->height before the loop begins. The code validated bytes-per-line but not row count.

CWE: CWE-119 — Improper Restriction of Operations within the Bounds of a Memory Buffer.

Fix: Add a height comparison that computes the minimum of source and destination heights, and use that minimum as the loop bound instead of the untrusted source height.

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

Heap buffer overflows in image-processing and streaming code often stem from assumptions that all dimensions come from the same trusted source. The XShmGetImage fix demonstrates that when source and destination are decoupled (especially across network or IPC boundaries), every dimension must be validated independently.

The minimal height comparison added here—comparing two integer fields before a loop—prevents an attacker or compromised server from overwriting heap memory. For streaming and real-time imaging applications, this kind of bounds checking is as critical as input validation in web frameworks.