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

[Jacob Trimble]

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.  These functions do
accept the size and verify that the data is valid for the given size.
Since we are verifying the data and the size we are given, we should
be checking for NULL as well.

> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel