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

Michael Niedermayer michael at niedermayer.cc
Tue Jun 5 23:02:25 EEST 2018


On Tue, Jun 05, 2018 at 09:30:51AM -0700, Jacob Trimble wrote:
> 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.

If a function can do a sensable action with a null pointer then it
makes sense to support it with a null pointer.
a common case is a cleanup function that one wants to be able to
call at any time, even twice and even before allocation.
That is its not an error to be NULL

Then there are functions for which a NULL pointer represents a
meaningless or impossible situation.
For example printf() or memcpy() do not allow NULL pointers.
The act of copying an "object" or printing it makes limited sense if there
is none. These functions should not check for NULL and return error
codes, because it is not an error of the function. Its an error in the
calling code, its too late by the time memcpy is called with a NULL.
Checking for NULL and halting program execution is the best memcpy
could do if it does check for it.
Continuing execution with for example memcpy(NULL) is likely more
exploitable than a plain null pointer dereference. There might
be a memcpy(p+123 in the next line for example becoming an out of
array write if execution continued

The case here looks similar to memcpy, but please correct me if iam
wrong.
If there is no input object then the operation of converting it to
some side data doesnt make much sense. The caller should have no need
to call this if it has nothing to add/convert


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180605/ea4957e9/attachment.sig>


More information about the ffmpeg-devel mailing list