[FFmpeg-devel,2/4] h264_mp4toannexb_bsf: Improve extradata overread checks

Submitted by Andreas Rheinhardt on July 22, 2019, 3:27 a.m.

Details

Message ID 20190722032716.40032-2-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt July 22, 2019, 3:27 a.m.
1. Currently during parsing the extradata, h264_mp4toannexb checks for
overreads by adding the size of the current unit to the current position
pointer and comparing this to the end position of the extradata. But
pointer comparisons and pointer arithmetic is only defined if it does not
exceed the object it is used on (one past the last element of an array
is allowed, too). In practice, this might lead to overflows. Therefore
the check has been changed.
2. The minimal size of an AVCDecoderConfigurationRecord is actually 7:
Four bytes containing version, profile and level, one byte for length
size and one byte each for the numbers of SPS and PPS. This has been
changed. The byte for the number of PPS has been forgotten.
3. The earlier code also did not detect an error if the extradata ended
directly after the last SPS in the SPS array (if any) although the
number of PPS has to come afterwards. A check for this has been
integrated into the general overread check.
4. The earlier code also might overread when reading the size of the
next unit. Given that this overread is not dangerous (the extradata is
supposed to be padded), only a comment for it has been added; the error
itself will be detected as part of the normal check for overreads.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/h264_mp4toannexb_bsf.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer July 24, 2019, 7:43 a.m.
On Mon, Jul 22, 2019 at 05:27:13AM +0200, Andreas Rheinhardt wrote:
> 1. Currently during parsing the extradata, h264_mp4toannexb checks for
> overreads by adding the size of the current unit to the current position
> pointer and comparing this to the end position of the extradata. But
> pointer comparisons and pointer arithmetic is only defined if it does not
> exceed the object it is used on (one past the last element of an array
> is allowed, too). In practice, this might lead to overflows. Therefore
> the check has been changed.
> 2. The minimal size of an AVCDecoderConfigurationRecord is actually 7:
> Four bytes containing version, profile and level, one byte for length
> size and one byte each for the numbers of SPS and PPS. This has been
> changed. The byte for the number of PPS has been forgotten.

> 3. The earlier code also did not detect an error if the extradata ended
> directly after the last SPS in the SPS array (if any) although the
> number of PPS has to come afterwards. A check for this has been
> integrated into the general overread check.

what if there is no pps afterwards and instead in stream?
could this change break such streams ?


> 4. The earlier code also might overread when reading the size of the
> next unit. Given that this overread is not dangerous (the extradata is
> supposed to be padded), only a comment for it has been added; the error
> itself will be detected as part of the normal check for overreads.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/h264_mp4toannexb_bsf.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

changes feel ok but this should be split as they are independant issues

thanks

[...]
Andreas Rheinhardt July 24, 2019, 2:40 p.m.
Michael Niedermayer:
> On Mon, Jul 22, 2019 at 05:27:13AM +0200, Andreas Rheinhardt wrote:
>> 1. Currently during parsing the extradata, h264_mp4toannexb checks for
>> overreads by adding the size of the current unit to the current position
>> pointer and comparing this to the end position of the extradata. But
>> pointer comparisons and pointer arithmetic is only defined if it does not
>> exceed the object it is used on (one past the last element of an array
>> is allowed, too). In practice, this might lead to overflows. Therefore
>> the check has been changed.
>> 2. The minimal size of an AVCDecoderConfigurationRecord is actually 7:
>> Four bytes containing version, profile and level, one byte for length
>> size and one byte each for the numbers of SPS and PPS. This has been
>> changed. The byte for the number of PPS has been forgotten.
> 
>> 3. The earlier code also did not detect an error if the extradata ended
>> directly after the last SPS in the SPS array (if any) although the
>> number of PPS has to come afterwards. A check for this has been
>> integrated into the general overread check.
> 
> what if there is no pps afterwards and instead in stream?
> could this change break such streams ?
> 
This patch increases the strictness for exactly one scenario: If the
extradata immediately ends after the SPS array, without the
numOfPictureParameterSets byte (that has to be present). This is now
considered invalid data, earlier code read into the padding and
therefore concluded that the number of PPS is zero. If the byte is
present and zero, then nothing changes: the stream will be playable if
the PPS is available in-band.
> 
>> 4. The earlier code also might overread when reading the size of the
>> next unit. Given that this overread is not dangerous (the extradata is
>> supposed to be padded), only a comment for it has been added; the error
>> itself will be detected as part of the normal check for overreads.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/h264_mp4toannexb_bsf.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> changes feel ok but this should be split as they are independant issues
> 
Ok, will split into 1. + (2. + 3.) + 4.

- Andreas

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
index 8b2899f300..076dfa31a7 100644
--- a/libavcodec/h264_mp4toannexb_bsf.c
+++ b/libavcodec/h264_mp4toannexb_bsf.c
@@ -71,7 +71,8 @@  static int h264_extradata_to_annexb(AVBSFContext *ctx, const int padding)
     int     total_size                  = 0;
     uint8_t *out                        = NULL, unit_nb, sps_done = 0,
              sps_seen                   = 0, pps_seen = 0;
-    const uint8_t *extradata            = ctx->par_in->extradata + 4;
+    const uint8_t *extradata            = ctx->par_in->extradata + 4,
+                  *extradata_end        = ctx->par_in->extradata + ctx->par_in->extradata_size;
     static const uint8_t nalu_header[4] = { 0, 0, 0, 1 };
     int length_size = (*extradata++ & 0x3) + 1; // retrieve length coded size
 
@@ -89,10 +90,11 @@  static int h264_extradata_to_annexb(AVBSFContext *ctx, const int padding)
     while (unit_nb--) {
         int err;
 
-        unit_size   = AV_RB16(extradata);
+        unit_size   = AV_RB16(extradata); /* possible overread ok due to padding */
+        extradata  += 2;
         total_size += unit_size + 4;
-        if (extradata + 2 + unit_size > ctx->par_in->extradata + ctx->par_in->extradata_size) {
-            av_log(ctx, AV_LOG_ERROR, "Packet header is not contained in global extradata, "
+        if (extradata_end - extradata < unit_size + !sps_done) {
+            av_log(ctx, AV_LOG_ERROR, "Global extradata truncated, "
                    "corrupted stream or invalid MP4/AVCC bitstream\n");
             av_free(out);
             return AVERROR(EINVAL);
@@ -100,8 +102,8 @@  static int h264_extradata_to_annexb(AVBSFContext *ctx, const int padding)
         if ((err = av_reallocp(&out, total_size + padding)) < 0)
             return err;
         memcpy(out + total_size - unit_size - 4, nalu_header, 4);
-        memcpy(out + total_size - unit_size, extradata + 2, unit_size);
-        extradata += 2 + unit_size;
+        memcpy(out + total_size - unit_size, extradata, unit_size);
+        extradata += unit_size;
 pps:
         if (!unit_nb && !sps_done++) {
             unit_nb = *extradata++; /* number of pps unit(s) */
@@ -144,7 +146,7 @@  static int h264_mp4toannexb_init(AVBSFContext *ctx)
         (extra_size >= 4 && AV_RB32(ctx->par_in->extradata) == 1)) {
         av_log(ctx, AV_LOG_VERBOSE,
                "The input looks like it is Annex B already\n");
-    } else if (extra_size >= 6) {
+    } else if (extra_size >= 7) {
         ret = h264_extradata_to_annexb(ctx, AV_INPUT_BUFFER_PADDING_SIZE);
         if (ret < 0)
             return ret;