[FFmpeg-devel] [PATCH] libavutil/encryption_info: Add NULL checks.

[Carl Eugen Hoyos]

2018-06-04 23:07 GMT+02:00, Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
> On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>
>> 2018-06-04 18:59 GMT+02:00, Jacob Trimble
>> <modmaker-at-google.com at ffmpeg.org>:
>> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer
>> > <michael at niedermayer.cc> wrote:
>> >>
>> >> On Thu, May 31, 2018 at 09:33:36AM -0700, Jacob Trimble wrote:
>> >> > Found by Chrome's ClusterFuzz: http://crbug.com/846662.
>> >> >
>> >> > Signed-off-by: Jacob Trimble <modmaker at google.com>
>> >> > ---
>> >> >  libavutil/encryption_info.c | 7 +++++--
>> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/libavutil/encryption_info.c
>> >> > b/libavutil/encryption_info.c
>> >> > index 20a752d6b4..a48ded922c 100644
>> >> > --- a/libavutil/encryption_info.c
>> >> > +++ b/libavutil/encryption_info.c
>> >> > @@ -64,6 +64,8 @@ AVEncryptionInfo *av_encryption_info_clone(const
>> >> > AVEncryptionInfo *info)
>> >> >  {
>> >> >      AVEncryptionInfo *ret;
>> >> >
>> >> > +    if (!info)
>> >> > +        return NULL;
>> >> >      ret = av_encryption_info_alloc(info->subsample_count,
>> >> > info->key_id_size, info->iv_size);
>> >> >      if (!ret)
>> >> >          return NULL;
>> >>
>> >> > @@ -127,7 +129,7 @@ uint8_t *av_encryption_info_add_side_data(const
>> >> > AVEncryptionInfo *info, size_t *
>> >> >      uint8_t *buffer, *cur_buffer;
>> >> >      uint32_t i;
>> >> >
>> >> > -    if (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA < info->key_id_size ||
>> >> > +    if (!info || !size || UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA <
>> >> > info->key_id_size ||
>> >> >          UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size <
>> >> > info->iv_size ||
>> >> >          (UINT32_MAX - FF_ENCRYPTION_INFO_EXTRA - info->key_id_size -
>> >> > info->iv_size) / 8 < info->subsample_count) {
>> >> >          return NULL;
>> >> > @@ -260,7 +262,8 @@ uint8_t
>> >> > *av_encryption_init_info_add_side_data(const
>> >> > AVEncryptionInitInfo *info,
>> >> >      uint8_t *buffer, *cur_buffer;
>> >> >      uint32_t i, max_size;
>> >> >
>> >> > -    if (UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> >> > info->system_id_size ||
>> >> > +    if (!info || !side_data_size ||
>> >> > +        UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA <
>> >> > info->system_id_size ||
>> >> >          UINT32_MAX - FF_ENCRYPTION_INIT_INFO_EXTRA -
>> >> > info->system_id_size < info->data_size) {
>> >> >          return NULL;
>> >> >      }
>> >>
>> >> in which valid case would these be called with NULL input ?
>> >> iam asking as this feels as if it might be a bug in teh caller
>> >>
>> >
>> > This was found by Chrome's ClusterFuzz, which I am not that familiar
>> > with.  I think it was just running fuzz tests directly on FFmpeg code,
>> > so it wasn't in production code.  But since this is a public method,
>> > we should validate the input in any case.
>>
>> How do you validate the size of C buffers in general?
>
> I'm not sure I understand your comment.  You can't verify the
> length of buffers unless the size is given to the method.

If we can't verify the size of the buffer (where both overread
and overwrite at least can have catastrophic impact) why
is it a good idea to check if the user passed an actual pointer
(as is required) or NULL as argument (where NULL typically
has limited impact)?

> These functions do accept the size and verify that the data
> is valid for the given size.

I may misunderstand the code but it looks to me as if the
given size is only checked because the needed space is
not necessarily known in advance / most functions do not
check.

> Since we are verifying the data and the size we are
> given, we should be checking for NULL as well.

Why?
(As we cannot check for the worse case anyway.)

Carl Eugen