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

[Jacob Trimble]

On Mon, Jun 4, 2018 at 2:24 PM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>
> 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.

This method doesn't need the size at all, the number of elements is
actually encoded in the side-data.  These methods use the
side_data_size to double-check that the number of bytes is large
enough to hold the number of elements that the side-data says there
are.

>
> > 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.)

Just because I can't check whether my food has salmonella doesn't mean
I shouldn't check the temperature when I cook it.  Adding a NULL check
is trivial and will catch the most common error case.  We also can't
check whether malloc() allocates enough memory, so should we then not
check for NULL?  NULL is used as an error signal, so if the caller
didn't include a NULL check, they will pass it here.  Rather than
crashing the program (hopefully it will crash, it is undefined
behavior, so anything could happen), we should be nice and validate
the input and error out.  Just because it is impossible to check other
error cases doesn't mean we should ignore all error checks.