diff mbox series

[FFmpeg-devel] avcodec/flac_parser: Do not loose header count in find_headers_search()

Message ID 20200203225720.14324-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/flac_parser: Do not loose header count in find_headers_search()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Feb. 3, 2020, 10:57 p.m. UTC
Fixes: Timeout
Fixes: out of array access
Fixes: 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
Fixes: 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/flac_parser.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Paul B Mahol Feb. 4, 2020, 8:10 a.m. UTC | #1
probably fine

On 2/3/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: Timeout
> Fixes: out of array access
> Fixes:
> 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
> Fixes:
> 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/flac_parser.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 9ffa288548..3424583c49 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -208,16 +208,20 @@ static int find_headers_search(FLACParseContext *fpc,
> uint8_t *buf,
>      uint32_t x;
>
>      for (i = 0; i < mod_offset; i++) {
> -        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
> -            size = find_headers_search_validate(fpc, search_start + i);
> +        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
> +            int ret = find_headers_search_validate(fpc, search_start + i);
> +            size = FFMAX(size, ret);
> +        }
>      }
>
>      for (; i < buf_size - 1; i += 4) {
>          x = AV_RN32(buf + i);
>          if (((x & ~(x + 0x01010101)) & 0x80808080)) {
>              for (j = 0; j < 4; j++) {
> -                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
> -                    size = find_headers_search_validate(fpc, search_start +
> i + j);
> +                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
> +                    int ret = find_headers_search_validate(fpc,
> search_start + i + j);
> +                    size = FFMAX(size, ret);
> +                }
>              }
>          }
>      }
> --
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt Feb. 4, 2020, 11:10 a.m. UTC | #2
Michael Niedermayer:
> Fixes: Timeout
> Fixes: out of array access
> Fixes: 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
> Fixes: 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/flac_parser.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 9ffa288548..3424583c49 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -208,16 +208,20 @@ static int find_headers_search(FLACParseContext *fpc, uint8_t *buf,
>      uint32_t x;
>  
>      for (i = 0; i < mod_offset; i++) {
> -        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
> -            size = find_headers_search_validate(fpc, search_start + i);
> +        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
> +            int ret = find_headers_search_validate(fpc, search_start + i);
> +            size = FFMAX(size, ret);
> +        }
>      }
>  
>      for (; i < buf_size - 1; i += 4) {
>          x = AV_RN32(buf + i);
>          if (((x & ~(x + 0x01010101)) & 0x80808080)) {
>              for (j = 0; j < 4; j++) {
> -                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
> -                    size = find_headers_search_validate(fpc, search_start + i + j);
> +                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
> +                    int ret = find_headers_search_validate(fpc, search_start + i + j);
> +                    size = FFMAX(size, ret);
> +                }
>              }
>          }
>      }
> 
1. Actually, find_headers_search_validate() can return an error and
these FFMAX (as well as the FFMAX already present in
find_new_headers()) will make sure that they are overwritten.
2. The linked list of buffered headers is currently traversed pretty
often: E.g. if it seems that no new headers have been found in a call
to find_new_headers(), the linked list will be traversed again in
order to count the number of buffered headers. Maybe one should
instead directly increment fpc->nb_headers_buffered after a new header
has been added to the list in find_headers_search_validate() and only
use the return value of find_headers_search_validate(),
find_headers_search(), find_new_headers() to forward the allocation
failure errors? (If one stores a pointer to the end of the linked
list, one can also remove the while-loop in
find_headers_search_validate(); don't know if this helps with the
timeout.)
There would be two scenarios in which this might lead to different
outcomes than the current code: In the case that you are fixing right
now where an prima-facie invalid header might mask a header that seems
to be valid. And in the case where an allocation failure happens and
the failure is currently forwarded. That's because nb_headers_buffered
is currently only updated when no allocation error is reported
(obviously, because the return value is used for both allocation
failure and new number of buffered headers). I am actually unsure what
to do in this scenario. Given that a parser is not allowed to return
errors, wouldn't it make sense to score the headers that we do have
(if there are at least FLAC_MIN_HEADERS)?
3. What array does this out-of-array-access affect? I presume it is
the link_penalty-array, but I just don't see how this is possible.

- Andreas
Michael Niedermayer Feb. 4, 2020, 5:34 p.m. UTC | #3
On Tue, Feb 04, 2020 at 11:10:00AM +0000, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: Timeout
> > Fixes: out of array access
> > Fixes: 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
> > Fixes: 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/flac_parser.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> > index 9ffa288548..3424583c49 100644
> > --- a/libavcodec/flac_parser.c
> > +++ b/libavcodec/flac_parser.c
> > @@ -208,16 +208,20 @@ static int find_headers_search(FLACParseContext *fpc, uint8_t *buf,
> >      uint32_t x;
> >  
> >      for (i = 0; i < mod_offset; i++) {
> > -        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
> > -            size = find_headers_search_validate(fpc, search_start + i);
> > +        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
> > +            int ret = find_headers_search_validate(fpc, search_start + i);
> > +            size = FFMAX(size, ret);
> > +        }
> >      }
> >  
> >      for (; i < buf_size - 1; i += 4) {
> >          x = AV_RN32(buf + i);
> >          if (((x & ~(x + 0x01010101)) & 0x80808080)) {
> >              for (j = 0; j < 4; j++) {
> > -                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
> > -                    size = find_headers_search_validate(fpc, search_start + i + j);
> > +                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
> > +                    int ret = find_headers_search_validate(fpc, search_start + i + j);
> > +                    size = FFMAX(size, ret);
> > +                }
> >              }
> >          }
> >      }
> > 
> 1. Actually, find_headers_search_validate() can return an error and
> these FFMAX (as well as the FFMAX already present in
> find_new_headers()) will make sure that they are overwritten.

yes


> 2. The linked list of buffered headers is currently traversed pretty
> often: E.g. if it seems that no new headers have been found in a call
> to find_new_headers(), the linked list will be traversed again in
> order to count the number of buffered headers. Maybe one should
> instead directly increment fpc->nb_headers_buffered after a new header
> has been added to the list in find_headers_search_validate() and only
> use the return value of find_headers_search_validate(),
> find_headers_search(), find_new_headers() to forward the allocation
> failure errors? (If one stores a pointer to the end of the linked
> list, one can also remove the while-loop in
> find_headers_search_validate(); don't know if this helps with the
> timeout.)

These are good ideas. The problem iam facing a bit here is that i
need to (or rather i want to) fix this in the releases too, so one
goal is to keep the change simple as that makes it more likely to
backport without too much issues.
We can clean up the code afterwards and in fact there is probably
more that can be cleaned up then what you mention (just by gut feeling
from this code)


> There would be two scenarios in which this might lead to different
> outcomes than the current code: In the case that you are fixing right
> now where an prima-facie invalid header might mask a header that seems
> to be valid. And in the case where an allocation failure happens and
> the failure is currently forwarded. That's because nb_headers_buffered
> is currently only updated when no allocation error is reported
> (obviously, because the return value is used for both allocation
> failure and new number of buffered headers). I am actually unsure what
> to do in this scenario. Given that a parser is not allowed to return
> errors, wouldn't it make sense to score the headers that we do have
> (if there are at least FLAC_MIN_HEADERS)?

first thought (which may be missing something) is dont allocate at all
use a fixed size array to keep track of items.
Having an unlimited number or items does not work very well and when 
we need an upper limit then we can allocate that during init and have
one issue less to deal with
if you want to work on something like this, you sure can as i ATM have
too much todo already ...


> 3. What array does this out-of-array-access affect? I presume it is
> the link_penalty-array, but I just don't see how this is possible.

stack through recursive calls after things become inconsistent

thanks

[...]
Michael Niedermayer Feb. 9, 2020, 7:38 p.m. UTC | #4
On Tue, Feb 04, 2020 at 06:34:47PM +0100, Michael Niedermayer wrote:
> On Tue, Feb 04, 2020 at 11:10:00AM +0000, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > Fixes: Timeout
> > > Fixes: out of array access
> > > Fixes: 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
> > > Fixes: 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/flac_parser.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> > > index 9ffa288548..3424583c49 100644
> > > --- a/libavcodec/flac_parser.c
> > > +++ b/libavcodec/flac_parser.c
> > > @@ -208,16 +208,20 @@ static int find_headers_search(FLACParseContext *fpc, uint8_t *buf,
> > >      uint32_t x;
> > >  
> > >      for (i = 0; i < mod_offset; i++) {
> > > -        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
> > > -            size = find_headers_search_validate(fpc, search_start + i);
> > > +        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
> > > +            int ret = find_headers_search_validate(fpc, search_start + i);
> > > +            size = FFMAX(size, ret);
> > > +        }
> > >      }
> > >  
> > >      for (; i < buf_size - 1; i += 4) {
> > >          x = AV_RN32(buf + i);
> > >          if (((x & ~(x + 0x01010101)) & 0x80808080)) {
> > >              for (j = 0; j < 4; j++) {
> > > -                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
> > > -                    size = find_headers_search_validate(fpc, search_start + i + j);
> > > +                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
> > > +                    int ret = find_headers_search_validate(fpc, search_start + i + j);
> > > +                    size = FFMAX(size, ret);
> > > +                }
> > >              }
> > >          }
> > >      }
> > > 
> > 1. Actually, find_headers_search_validate() can return an error and
> > these FFMAX (as well as the FFMAX already present in
> > find_new_headers()) will make sure that they are overwritten.
> 
> yes
> 
> 
> > 2. The linked list of buffered headers is currently traversed pretty
> > often: E.g. if it seems that no new headers have been found in a call
> > to find_new_headers(), the linked list will be traversed again in
> > order to count the number of buffered headers. Maybe one should
> > instead directly increment fpc->nb_headers_buffered after a new header
> > has been added to the list in find_headers_search_validate() and only
> > use the return value of find_headers_search_validate(),
> > find_headers_search(), find_new_headers() to forward the allocation
> > failure errors? (If one stores a pointer to the end of the linked
> > list, one can also remove the while-loop in
> > find_headers_search_validate(); don't know if this helps with the
> > timeout.)
> 
> These are good ideas. The problem iam facing a bit here is that i
> need to (or rather i want to) fix this in the releases too, so one
> goal is to keep the change simple as that makes it more likely to
> backport without too much issues.
> We can clean up the code afterwards and in fact there is probably
> more that can be cleaned up then what you mention (just by gut feeling
> from this code)

any objections ?
if not i will push this in a few days

thx


[...]
Nicolas George Feb. 9, 2020, 7:40 p.m. UTC | #5
Michael Niedermayer (12020-02-09):
> any objections ?
> if not i will push this in a few days

Please fix the spelling mistake in the commit message:

To loose = to set free.
To lose = to not have it anymore accidentally.

Regards,
Michael Niedermayer Feb. 10, 2020, 10:14 p.m. UTC | #6
On Sun, Feb 09, 2020 at 08:40:08PM +0100, Nicolas George wrote:
> Michael Niedermayer (12020-02-09):
> > any objections ?
> > if not i will push this in a few days
> 
> Please fix the spelling mistake in the commit message:
> 
> To loose = to set free.
> To lose = to not have it anymore accidentally.

fixed, others have also pointed me to this already

thnaks

[...]
diff mbox series

Patch

diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
index 9ffa288548..3424583c49 100644
--- a/libavcodec/flac_parser.c
+++ b/libavcodec/flac_parser.c
@@ -208,16 +208,20 @@  static int find_headers_search(FLACParseContext *fpc, uint8_t *buf,
     uint32_t x;
 
     for (i = 0; i < mod_offset; i++) {
-        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
-            size = find_headers_search_validate(fpc, search_start + i);
+        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
+            int ret = find_headers_search_validate(fpc, search_start + i);
+            size = FFMAX(size, ret);
+        }
     }
 
     for (; i < buf_size - 1; i += 4) {
         x = AV_RN32(buf + i);
         if (((x & ~(x + 0x01010101)) & 0x80808080)) {
             for (j = 0; j < 4; j++) {
-                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
-                    size = find_headers_search_validate(fpc, search_start + i + j);
+                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
+                    int ret = find_headers_search_validate(fpc, search_start + i + j);
+                    size = FFMAX(size, ret);
+                }
             }
         }
     }