Skip to content

Fix out-of-bounds read in motion compensation (plm_video_process_macroblock)#77

Open
hardw00t wants to merge 1 commit into
phoboslab:masterfrom
hardw00t:fix-oob-read-motioncomp
Open

Fix out-of-bounds read in motion compensation (plm_video_process_macroblock)#77
hardw00t wants to merge 1 commit into
phoboslab:masterfrom
hardw00t:fix-oob-read-motioncomp

Conversation

@hardw00t

Copy link
Copy Markdown

Summary

plm_video_process_macroblock() bounds-checks only the base source/destination
indices si/di against max_address, but the half-pel prediction cases dereference
s[si + 1], s[si + dw] and s[si + dw + 1], and PLM_BLOCK_SET iterates a full
block_size × block_size region. The deepest source byte actually read is:

si + (block_size-1)*dw + (block_size-1) + (odd_v ? dw : 0) + (odd_h ? 1 : 0)

For si near max_address with an odd (half-pel) motion component this reaches up to one
row (dw) past the end of the plane. Since all three frames share a single
PLM_MALLOC(frame_data_size * 3) chunk with no padding, when the reference plane is the last
plane the read runs off the end of the allocation — an out-of-bounds read on crafted input.
Motion vectors from plm_video_decode_motion_vector() are wrapped to ±(fscale<<4) but not
clamped to the frame geometry, so a malformed stream can place the prediction source at the edge.

Destination writes (d[di]) stay in bounds, so this is a read-only overflow — impact is a
potential crash (read into an unmapped page) and, since the out-of-bounds bytes are averaged
into decoded pixels, a limited information-leak into the output frame.

Fix

Bound the actual deepest source and destination offsets against the plane size, not just the
base indices:

unsigned int plane_size = dw * (self->mb_height * block_size);
unsigned int max_di = di + (block_size - 1) * dw + (block_size - 1);
unsigned int max_si = si + (block_size - 1) * dw + (block_size - 1)
    + (odd_v ? dw : 0) + (odd_h ? 1 : 0);
if (si >= plane_size || di >= plane_size || max_si >= plane_size || max_di >= plane_size) {
    return; // corrupt video
}

Testing

  • Found via libFuzzer + AddressSanitizer fuzzing of the decode path (plm_create_with_memory
    plm_decode_video).
  • With this change, the crafted stream that triggered the ASan heap-buffer-overflow in
    plm_video_process_macroblock no longer overflows (the macroblock is treated as corrupt and
    skipped), and valid MPEG1 streams still decode identically.
  • Header compiles cleanly with -Wall -Wextra.

I have a minimal reproducer that I'm happy to share privately if useful — just let me know the
best channel; I kept it out of this public PR.

The bounds check only validated the base indices si/di, but the half-pel
prediction reads s[si+1], s[si+dw] and s[si+dw+1] and the block loop spans
a block_size x block_size region. For si near max_address with an odd
(half-pel) motion component the source read can run up to one row (dw) past
the frame plane; when the reference plane is the last plane in the shared
frames_data allocation this reads out of bounds.

Bound the actual deepest source and destination offsets against the plane
size instead of only the base indices. Found with libFuzzer + ASan.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant