2018-06-04 23:07 GMT+02:00, Jacob Trimble <modmaker-at-google....@ffmpeg.org>: > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffm...@gmail.com> wrote: >> >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble >> <modmaker-at-google....@ffmpeg.org>: >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer >> > <mich...@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 <modma...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
