| Message ID | 20180531163336.61888-1-modmaker@google.com |
|---|---|
| State | New |
| Headers | show |
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@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 thx [...]
On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer <michael@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@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. > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In a rich man's house there is no place to spit but his face. > -- Diogenes of Sinope > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2018-06-04 18:59 GMT+02:00, Jacob Trimble <modmaker-at-google.com@ffmpeg.org>: > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer > <michael@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@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? Carl Eugen
On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2018-06-04 18:59 GMT+02:00, Jacob Trimble <modmaker-at-google.com@ffmpeg.org>: > > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer > > <michael@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
2018-06-04 23:07 GMT+02:00, Jacob Trimble <modmaker-at-google.com@ffmpeg.org>: > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble >> <modmaker-at-google.com@ffmpeg.org>: >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer >> > <michael@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@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
On Mon, Jun 4, 2018 at 2:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > 2018-06-04 23:07 GMT+02:00, Jacob Trimble <modmaker-at-google.com@ffmpeg.org>: > > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> > >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble > >> <modmaker-at-google.com@ffmpeg.org>: > >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer > >> > <michael@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@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.
On 05/06/18 17:30, Jacob Trimble wrote: > 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. (My opinion, others may disagree.) Please consider what is actually useful to an API user here. The check you are suggesting will cause the function to, when passed entirely invalid arguments, silently return having done nothing. Is this better than the almost-guaranteed segfault you will get instead? Well, no. There is much more scope for the error to go unnoticed and cause other hard-to-debug issues later, where it could have been caught immediately. If there is a concern that a function like this could be misused then (since this is certainly undefined behaviour in any case) turning it into an abort() is the best case so that the program will definitely fail and any errors can be diagnosed immediately. As such, I think argument checks for nonsensical invalid input like this should be done either with av_assert or not at all. - Mark
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@gmail.com> wrote: > > > > 2018-06-04 23:07 GMT+02:00, Jacob Trimble <modmaker-at-google.com@ffmpeg.org>: > > > On Mon, Jun 4, 2018 at 10:46 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > >> > > >> 2018-06-04 18:59 GMT+02:00, Jacob Trimble > > >> <modmaker-at-google.com@ffmpeg.org>: > > >> > On Fri, Jun 1, 2018 at 5:03 PM Michael Niedermayer > > >> > <michael@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@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 [...]
On Tue, Jun 5, 2018 at 1:06 PM Mark Thompson <sw@jkqxz.net> wrote: > > On 05/06/18 17:30, Jacob Trimble wrote: > > 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. > > (My opinion, others may disagree.) > > Please consider what is actually useful to an API user here. > > The check you are suggesting will cause the function to, when passed entirely invalid arguments, silently return having done nothing. Is this better than the almost-guaranteed segfault you will get instead? Well, no. There is much more scope for the error to go unnoticed and cause other hard-to-debug issues later, where it could have been caught immediately. It returns an error, which should be checked by the caller. We can't do anything to change the caller's code, but we can make our code not crash their program. > > If there is a concern that a function like this could be misused then (since this is certainly undefined behaviour in any case) turning it into an abort() is the best case so that the program will definitely fail and any errors can be diagnosed immediately. As such, I think argument checks for nonsensical invalid input like this should be done either with av_assert or not at all. What about invalid MP4 files? Should we convert those to abort() too? When parsing MP4 files, we check that it is valid and return an error code if it is not. There is no difference between a method to parse an MP4 file and these, so these should validate the inputs as best it can and return errors if we find them. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel When I design an API, I try to make it hard to misuse, being as helpful to the caller as possible. Having the function intentionally crash on certain input when a simple if could solve it is not a good idea. I think the memcpy API is an example of a bad api, it assumes valid input. Are we going to stop checking that MP4 files are valid too? If we only accept valid MP4 files it seems pointless to check it and return errors. These functions return NULL on errors, it is an error to pass NULL, therefore we should check and return NULL. It is impossible to check some things, but if it were possible, I would suggest checking that too. One other problem is that NULL is sometimes a valid value to pass to a function and sometimes not. NULL is used as a signal value whereas an invalid pointer is never valid. From the compiler's point of view, there is no difference between a function that accepts NULL as valid and one that does not. So the only thing we can do is write a comment saying "Does not accept NULL", but this is fragile as the compiler can't check this (without static analysis). By having us return an error for invalid input, it makes the API more stable and easier to use. We can't stop the caller from ignoring the return value, but we can at least do our best to validate the input and avoid crashing the program. But if you aren't going to accept a patch to increase code heath, there is nothing I can do. I'll remove the NULL checks from my other patch so it can be submitted.
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; }
Found by Chrome's ClusterFuzz: http://crbug.com/846662. Signed-off-by: Jacob Trimble <modmaker@google.com> --- libavutil/encryption_info.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)