PR #25571 opened by luoye-cpu URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25571 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25571.patch
# Summary of changes `av_dynamic_hdr_plus_to_t35()` takes five counts from `AVDynamicHDRPlus` and uses them as loop bounds over fixed-size arrays: | field | array | documented range | |---|---|---| | `num_windows` | `params[3]` | 1..3 | | `params[i].num_distribution_maxrgb_percentiles` | `distribution_maxrgb[15]` | 0..15 | | `params[i].num_bezier_curve_anchors` | `bezier_curve_anchors[15]` | 0..15 | | `targeted_system_display_maximum_luminance` rows/cols | `[25][25]` | 2..25 | | `mastering_display_actual_peak_luminance` rows/cols | `[25][25]` | 2..25 | All of them are `uint8_t` fields of a public structure that the caller fills in, and the writer trusted them. A `num_windows` of 255 walks `params` 252 entries past the end; a row/column pair of 255 makes the loop read 65025 entries from a 25x25 array. The size formula and the write loop use the same bound, so the output buffer always matches the number of bits written and `put_bits()` cannot overflow it. The out-of-bounds access is on the read side, which is why the fix is a range check and not a bigger buffer. `av_dynamic_hdr_plus_from_t35()` already rejects `num_windows` outside 1..3 and both peak-luminance pairs outside 2..25; the two per-window counts are bounded only by the bits left. This patch rejects the same upper bounds in the writer. The peak-luminance pairs are only read when their flag is set, so those two checks are gated on the flag as well and cannot reject a structure the writer never looks at. These are upper bounds against the arrays the writer indexes, not an attempt to make the writer accept exactly the range the parser does. The lower bounds the parser also enforces are deliberately not mirrored: a `num_windows` of 0 or a peak-luminance pair of 0 or 1 cannot make the writer read past an array, it only makes a payload the parser will refuse, and rejecting it would change the result for callers that pass a zeroed structure. ## Testing `libavutil/tests/hdr_dynamic_metadata.c` gets six cases: the five oversized counts must now return `AVERROR_INVALIDDATA`, and one case sets a row/column pair out of range with the flag clear and checks that it still serializes. Built against master (`0ea6606418c4602a9de5d43657b7b2f56b0f9681`) and run: the program's stdout matches `tests/ref/fate/hdr_dynamic_metadata` line for line, 152 lines. `tools/patcheck` reports nothing beyond its usual false positives on `libavutil` tests (the `printf()` in the test file, the output lines of the fate ref file being read as symbol definitions, and the `ret` heuristic). ## API No public API change: only the return value of an existing function for inputs that were previously undefined behaviour. No new FATE samples are required. >From 7f1d7287c45dc3c8edee851f0c797caf9f5cc7e4 Mon Sep 17 00:00:00 2001 From: luoye-cpu <[email protected]> Date: Thu, 29 Oct 2026 09:08:21 +0800 Subject: [PATCH] avutil/hdr_dynamic_metadata: reject oversized HDR10+ counts av_dynamic_hdr_plus_to_t35() uses five counts from AVDynamicHDRPlus as loop bounds over fixed-size arrays: num_windows over params[3], the per-window distribution_maxrgb and bezier_curve_anchors counts over 15-element arrays, and the two peak-luminance row/column pairs over 25x25 arrays. The fields are uint8_t and may be filled in by the caller, while the header documents much narrower ranges (1..3 and 2..25), and the writer trusted them. The size formula and the write loop use the same bound, so the buffer always matches the number of bits written and put_bits() cannot overflow it; the out-of-bounds access is on the read side. A row/column pair of 255 makes the loop read 65025 entries from a 25x25 array. av_dynamic_hdr_plus_from_t35() already rejects num_windows outside 1..3 and the two peak-luminance pairs outside 2..25; the two per-window counts are bounded only by the bits left. Reject the upper bounds here as well: they are the ones that decide whether the writer's loop stays inside the array. The peak-luminance pairs are read only when their flag is set, so their check is gated the same way and cannot reject a structure the writer never looks at. These are upper bounds against the arrays the writer indexes, not an attempt to make the writer accept exactly the range the parser does. The lower bounds the parser also enforces are deliberately not mirrored: a num_windows of 0 or a peak-luminance pair of 0 or 1 cannot make the writer read past an array, it only makes a payload the parser will refuse, and rejecting it would change the result for callers that pass a zeroed structure. Keeping the check to the upper bounds keeps this a pure hardening change. FATE cases cover the rejected counts, plus one for an out-of-range count whose flag is clear, which must still serialize. Signed-off-by: luoye-cpu <[email protected]> --- libavutil/hdr_dynamic_metadata.c | 39 +++++++++++++ libavutil/tests/hdr_dynamic_metadata.c | 76 ++++++++++++++++++++++++++ tests/ref/fate/hdr_dynamic_metadata | 6 ++ 3 files changed, 121 insertions(+) diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c index 9b464993433..a0061dd4c18 100644 --- a/libavutil/hdr_dynamic_metadata.c +++ b/libavutil/hdr_dynamic_metadata.c @@ -244,6 +244,45 @@ int av_dynamic_hdr_plus_to_t35(const AVDynamicHDRPlus *s, uint8_t **data, size_t return AVERROR(EINVAL); if ((!data || *data) && !size) return AVERROR(EINVAL); + /* These counts are uint8_t fields of a public structure that callers fill + * in, and each of them is a loop bound over a fixed-size array: refuse the + * ones that would take the write loop past the end of that array. + * av_dynamic_hdr_plus_from_t35() rejects the same upper bounds when reading + * back. + * + * A num_windows of 0 stays accepted: no per-window loop runs for it, so + * nothing is read out of bounds, and rejecting it would change the result + * for callers that pass a zeroed structure. */ + if (s->num_windows > FF_ARRAY_ELEMS(s->params)) + return AVERROR_INVALIDDATA; + + for (int w = 0; w < s->num_windows; w++) { + const AVHDRPlusColorTransformParams *p = &s->params[w]; + + if (p->num_distribution_maxrgb_percentiles > + FF_ARRAY_ELEMS(p->distribution_maxrgb) || + p->num_bezier_curve_anchors > + FF_ARRAY_ELEMS(p->bezier_curve_anchors)) + return AVERROR_INVALIDDATA; + } + + /* The peak-luminance pairs use a row count and a column count, and are + * only read when their flag is set, so check them the same way. */ + if (s->targeted_system_display_actual_peak_luminance_flag) { + if (s->num_rows_targeted_system_display_actual_peak_luminance > + FF_ARRAY_ELEMS(s->targeted_system_display_actual_peak_luminance) || + s->num_cols_targeted_system_display_actual_peak_luminance > + FF_ARRAY_ELEMS(s->targeted_system_display_actual_peak_luminance[0])) + return AVERROR_INVALIDDATA; + } + + if (s->mastering_display_actual_peak_luminance_flag) { + if (s->num_rows_mastering_display_actual_peak_luminance > + FF_ARRAY_ELEMS(s->mastering_display_actual_peak_luminance) || + s->num_cols_mastering_display_actual_peak_luminance > + FF_ARRAY_ELEMS(s->mastering_display_actual_peak_luminance[0])) + return AVERROR_INVALIDDATA; + } /** * Buffer size per CTA-861-H p.253-254: diff --git a/libavutil/tests/hdr_dynamic_metadata.c b/libavutil/tests/hdr_dynamic_metadata.c index e85604e4f07..fea5870264e 100644 --- a/libavutil/tests/hdr_dynamic_metadata.c +++ b/libavutil/tests/hdr_dynamic_metadata.c @@ -486,6 +486,82 @@ static void test_hdr_plus(void) printf("to_t35 no data no size: ret==AVERROR(EINVAL)=%s\n", ret == AVERROR(EINVAL) ? "yes" : "no"); + /* num_windows indexes the fixed-size params[] array, so a count above its + * length must be refused instead of serializing windows that are not + * there. Queried in size-only mode to get past the data/size check above + * and actually reach the range check. */ + hdr->num_windows = FF_ARRAY_ELEMS(hdr->params) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 num_windows>3: ret==AVERROR_INVALIDDATA=%s\n", + ret == AVERROR_INVALIDDATA ? "yes" : "no"); + hdr->num_windows = 2; + + /* The same holds for every other count the write loop uses as a bound: + * the two per-window counts below index 15-element arrays, and the two + * peak-luminance row/column pairs index 25x25 arrays while only being + * serialized into 5-bit fields, so an out-of-range value would both be + * truncated on the wire and read past the array. */ + uint8_t pct = hdr->params[0].num_distribution_maxrgb_percentiles; + uint8_t anch = hdr->params[0].num_bezier_curve_anchors; + uint8_t trow = hdr->num_rows_targeted_system_display_actual_peak_luminance; + uint8_t mcol = hdr->num_cols_mastering_display_actual_peak_luminance; + uint8_t tflag = hdr->targeted_system_display_actual_peak_luminance_flag; + uint8_t mflag = hdr->mastering_display_actual_peak_luminance_flag; + + /* The two pairs below are only read when their flag is set, which is + * also how the check is gated: a struct the writer never reads must + * not start being rejected. */ + hdr->targeted_system_display_actual_peak_luminance_flag = 1; + hdr->mastering_display_actual_peak_luminance_flag = 1; + + hdr->params[0].num_distribution_maxrgb_percentiles = + FF_ARRAY_ELEMS(hdr->params[0].distribution_maxrgb) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 percentiles>15: ret==AVERROR_INVALIDDATA=%s\n", + ret == AVERROR_INVALIDDATA ? "yes" : "no"); + hdr->params[0].num_distribution_maxrgb_percentiles = pct; + + hdr->params[0].num_bezier_curve_anchors = + FF_ARRAY_ELEMS(hdr->params[0].bezier_curve_anchors) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 anchors>15: ret==AVERROR_INVALIDDATA=%s\n", + ret == AVERROR_INVALIDDATA ? "yes" : "no"); + hdr->params[0].num_bezier_curve_anchors = anch; + + hdr->num_rows_targeted_system_display_actual_peak_luminance = + FF_ARRAY_ELEMS(hdr->targeted_system_display_actual_peak_luminance) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 targeted rows>25: ret==AVERROR_INVALIDDATA=%s\n", + ret == AVERROR_INVALIDDATA ? "yes" : "no"); + hdr->num_rows_targeted_system_display_actual_peak_luminance = trow; + + hdr->num_cols_mastering_display_actual_peak_luminance = + FF_ARRAY_ELEMS(hdr->mastering_display_actual_peak_luminance[0]) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 mastering cols>25: ret==AVERROR_INVALIDDATA=%s\n", + ret == AVERROR_INVALIDDATA ? "yes" : "no"); + hdr->num_cols_mastering_display_actual_peak_luminance = mcol; + + /* With the flags clear the counts are never read, so an out-of-range + * value alone must not turn into an error. */ + hdr->targeted_system_display_actual_peak_luminance_flag = 0; + hdr->mastering_display_actual_peak_luminance_flag = 0; + hdr->num_rows_targeted_system_display_actual_peak_luminance = + FF_ARRAY_ELEMS(hdr->targeted_system_display_actual_peak_luminance) + 1; + size = 0; + ret = av_dynamic_hdr_plus_to_t35(hdr, NULL, &size); + printf("to_t35 rows>25 with flag clear: ret==0=%s\n", + ret == 0 ? "yes" : "no"); + hdr->num_rows_targeted_system_display_actual_peak_luminance = trow; + + hdr->targeted_system_display_actual_peak_luminance_flag = tflag; + hdr->mastering_display_actual_peak_luminance_flag = mflag; + ret = av_dynamic_hdr_plus_from_t35(NULL, (const uint8_t *)"", 0); printf("from_t35 NULL s: ret==AVERROR(EINVAL)=%s\n", ret == AVERROR(EINVAL) ? "yes" : "no"); diff --git a/tests/ref/fate/hdr_dynamic_metadata b/tests/ref/fate/hdr_dynamic_metadata index 118f69d7e51..3e57f667684 100644 --- a/tests/ref/fate/hdr_dynamic_metadata +++ b/tests/ref/fate/hdr_dynamic_metadata @@ -60,6 +60,12 @@ decoded: Testing error paths to_t35 NULL s: ret==AVERROR(EINVAL)=yes to_t35 no data no size: ret==AVERROR(EINVAL)=yes +to_t35 num_windows>3: ret==AVERROR_INVALIDDATA=yes +to_t35 percentiles>15: ret==AVERROR_INVALIDDATA=yes +to_t35 anchors>15: ret==AVERROR_INVALIDDATA=yes +to_t35 targeted rows>25: ret==AVERROR_INVALIDDATA=yes +to_t35 mastering cols>25: ret==AVERROR_INVALIDDATA=yes +to_t35 rows>25 with flag clear: ret==0=yes from_t35 NULL s: ret==AVERROR(EINVAL)=yes from_t35 oversized: ret==AVERROR(EINVAL)=yes -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
