[FFmpeg-devel] [PATCH 2/2] avformat/mpc8: fix hang with fuzzed file

[Michael Niedermayer]

On Tue, Feb 03, 2015 at 10:06:28PM +0100, wm4 wrote:
> On Tue, 3 Feb 2015 22:00:11 +0100
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > On Tue, Feb 03, 2015 at 09:54:51PM +0100, wm4 wrote:
> > > On Tue, 3 Feb 2015 21:47:57 +0100
> > > Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> > > 
> > > > On Tue, Feb 03, 2015 at 07:04:12PM +0100, wm4 wrote:
> > > > > This can lead to an endless loop by seeking back a few bytes after each
> > > > > attempted chunk read. Assuming negative sizes are always invalid, this
> > > > > is easy to fix. Other code in this demuxer treats negative sizes as
> > > > > invalid as well.
> > > > > 
> > > > > Fixes ticket #4262.
> > > > > ---
> > > > >  libavformat/mpc8.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/libavformat/mpc8.c b/libavformat/mpc8.c
> > > > > index d6ca338..6524c7e 100644
> > > > > --- a/libavformat/mpc8.c
> > > > > +++ b/libavformat/mpc8.c
> > > > > @@ -223,6 +223,10 @@ static int mpc8_read_header(AVFormatContext *s)
> > > > >      while(!avio_feof(pb)){
> > > > >          pos = avio_tell(pb);
> > > > >          mpc8_get_chunk_header(pb, &tag, &size);
> > > > > +        if (size < 0) {
> > > > 
> > > > Isn't the only way for this to become negative for a too
> > > > large uint64_t to be assigned to a int64_t?
> > > > I.e. undefined behaviour.
> > > > In that case this isn't quite the right way in the strictest sense,
> > > > though it is likely to work "normally".
> > > 
> > > This is what mpc8_get_chunk_header() does:
> > > 
> > >     pos = avio_tell(pb);
> > >     *tag = avio_rl16(pb);
> > >     *size = ffio_read_varlen(pb);
> > >     *size -= avio_tell(pb) - pos;
> > > 
> > > ffio_read_varlen() returns uint64_t, so I don't think it's supposed to
> > > read negative values. But the last line could make it negative anyway
> > > if the chunk size field read here is inconsistent.
> > 
> > The problem is in this line:
> > *size = ffio_read_varlen(pb);
> > *size is int64_t.
> > If ffio_read_varlen returns a value > 0x7f..ffull then this
> > assignment is undefined, which a fairly risky thing to allow
> > for a security-relevant check.
> 
> AFAIK it's implementation-defined, rather than undefined. Which means
> it works on all CPUs with complement-of-2 integers anyway.

applied

thanks

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150204/fd8b09f2/attachment.asc>