Skip to content

Bound col_starts indices in TrackingData motion-vector decoding (heap OOB read) - #6311

Open
evilgensec wants to merge 1 commit into
google-ai-edge:masterfrom
evilgensec:fix-trackingdata-oob
Open

Bound col_starts indices in TrackingData motion-vector decoding (heap OOB read)#6311
evilgensec wants to merge 1 commit into
google-ai-edge:masterfrom
evilgensec:fix-trackingdata-oob

Conversation

@evilgensec

Copy link
Copy Markdown

Problem

MotionVectorFrameFromTrackingData() (and the sibling
FeatureAndDescriptorFromTrackingData()) in mediapipe/util/tracking/tracking.cc decode
a compressed-sparse-column layout from a TrackingData proto:

for (int c = 0; c < motion_data.col_starts_size() - 1; ++c) {
  for (int r = motion_data.col_starts(c),
           r_end = motion_data.col_starts(c + 1);   // attacker-controlled bound
       r < r_end; ++r) {
    const float y  = motion_data.row_indices(r);    // r unchecked vs row_indices_size()
    const float dx = motion_data.vector_data(2 * r);     // unchecked vs vector_data_size()
    const float dy = motion_data.vector_data(2 * r + 1);
    ...
    if (long_tracks) motion_vector.track_id = motion_data.track_id(r);  // unchecked
  }
}

col_starts, row_indices, vector_data, track_id (and feature_descriptors) are
independent repeated fields; nothing constrains col_starts(c+1) to the sizes of the
others. A TrackingData whose col_starts values exceed the dependent arrays drives r
(and 2*r) past the end of each RepeatedField. RepeatedField::Get(int) is bounds
checked only when the bounds-check mode is kAbort; with the default mode it is an
unchecked elements()[index] (the ABSL_DCHECK is a no-op under NDEBUG), so this is a
heap out-of-bounds read in opt builds. Out-of-bounds heap contents are emitted as box
coordinates; large values crash.

This is reachable from deserialized TrackingData: BoxTracker::ReadChunkFromCache
parses chunk_* cache files (box_tracker.cc) and feeds them here, and
BoxTrackerCalculator accepts TrackingData on its TRACKING input stream. The
binary-decode path (FlowPackager::DecodeTrackingData) has a consistency check
(ABSL_CHECK_EQ(num_vectors, col_starts.back())), but these direct-proto paths do not.

Fix

Before dereferencing, reject an index r that falls outside the dependent repeated
fields. Because r increases monotonically within the inner loop, breaking on the first
out-of-range index is sufficient and changes nothing for well-formed input. The same guard
is added to FeatureAndDescriptorFromTrackingData.

MotionVectorFrameFromTrackingData and FeatureAndDescriptorFromTrackingData walk a
compressed-sparse-column structure built from independent repeated fields of
TrackingData (col_starts, row_indices, vector_data, track_id, feature_descriptors).
The col_starts values are used as loop bounds and as indices into the other arrays
with no check that they are within those arrays' sizes. A TrackingData whose
col_starts exceed the dependent arrays drives the index past the end of the protobuf
RepeatedField, a heap out-of-bounds read (RepeatedField::Get is unchecked under
NDEBUG with the default bounds-check mode). This is reachable from deserialized
TrackingData (e.g. BoxTracker cache chunks and the BoxTrackerCalculator TRACKING
input).

Reject indices that fall outside the dependent repeated fields before dereferencing.
@evilgensec

Copy link
Copy Markdown
Author

Checking in on this. Anything I can do to help get it reviewed?

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