[FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
Michael Niedermayer
michael at niedermayer.cc
Sat May 13 16:14:54 EEST 2017
On Sat, May 13, 2017 at 08:38:52AM +0200, wm4 wrote:
> On Fri, 12 May 2017 20:55:13 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
>
> > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote:
> > > On 5/12/17, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote:
> > > >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote:
> > > >> > On 5/11/17, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >> > > Most of these were found through code review in response to
> > > >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896
> > > >> > > There is thus no testcase for most of this.
> > > >> > > The initial issue was Found-by: continuous fuzzing process
> > > >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > > >> > >
> > > >> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > >> > > ---
> > > >> > > libavcodec/xpmdec.c | 37 ++++++++++++++++++++++++++++++-------
> > > >> > > 1 file changed, 30 insertions(+), 7 deletions(-)
> > > >> > >
> > > >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c
> > > >> > > index 9112d4cb5e..03172e4aad 100644
> > > >> > > --- a/libavcodec/xpmdec.c
> > > >> > > +++ b/libavcodec/xpmdec.c
> > > >> > > @@ -29,6 +29,8 @@
> > > >> > > typedef struct XPMContext {
> > > >> > > uint32_t *pixels;
> > > >> > > int pixels_size;
> > > >> > > + uint8_t *buf;
> > > >> > > + int buf_size;
> > > >> > > } XPMDecContext;
> > > >> > >
> > > >> > > typedef struct ColorEntry {
> > > >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char
> > > >> > > *p, int
> > > >> > > len)
> > > >> > > const ColorEntry *entry;
> > > >> > > char color_name[100];
> > > >> > >
> > > >> > > + len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1);
> > > >> > > +
> > > >> > > if (*p == '#') {
> > > >> > > p++;
> > > >> > > len--;
> > > >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext
> > > >> > > *avctx,
> > > >> > > void *data,
> > > >> > > {
> > > >> > > XPMDecContext *x = avctx->priv_data;
> > > >> > > AVFrame *p=data;
> > > >> > > - const uint8_t *end, *ptr = avpkt->data;
> > > >> > > + const uint8_t *end, *ptr;
> > > >> > > int ncolors, cpp, ret, i, j;
> > > >> > > int64_t size;
> > > >> > > uint32_t *dst;
> > > >> > >
> > > >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA;
> > > >> > >
> > > >> > > - end = avpkt->data + avpkt->size;
> > > >> > > - while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9)
> > > >> > > + av_fast_padded_malloc(&x->buf, &x->buf_size, avpkt->size);
> > > >> > > + if (!x->buf)
> > > >> > > + return AVERROR(ENOMEM);
> > > >> > > + memcpy(x->buf, avpkt->data, avpkt->size);
> > > >> > > + x->buf[avpkt->size] = 0;
> > > >> > > +
> > > >> > > + ptr = x->buf;
> > > >> > > + end = x->buf + avpkt->size;
> > > >> > > + while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9))
> > > >> > > ptr++;
> > > >> > >
> > > >> > > - if (ptr >= end) {
> > > >> > > + if (end - ptr <= 9) {
> > > >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n");
> > > >> > > return AVERROR_INVALIDDATA;
> > > >> > > }
> > > >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx,
> > > >> > > void
> > > >> > > *data,
> > > >> > >
> > > >> > > size = 1;
> > > >> > > for (i = 0; i < cpp; i++)
> > > >> > > - size *= 94;
> > > >> > > + size *= 95;
> > > >> > >
> > > >> > > if (ncolors <= 0 || ncolors > size) {
> > > >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors:
> > > >> > > %d\n",
> > > >> > > ncolors);
> > > >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext
> > > >> > > *avctx,
> > > >> > > void *data,
> > > >> > > return AVERROR(ENOMEM);
> > > >> > >
> > > >> > > ptr += mod_strcspn(ptr, ",") + 1;
> > > >> > > + if (end - ptr < 1)
> > > >> > > + return AVERROR_INVALIDDATA;
> > > >> > > +
> > > >> > > for (i = 0; i < ncolors; i++) {
> > > >> > > const uint8_t *index;
> > > >> > > int len;
> > > >> > >
> > > >> > > ptr += mod_strcspn(ptr, "\"") + 1;
> > > >> > > - if (ptr + cpp > end)
> > > >> > > + if (end - ptr < cpp)
> > > >> > > return AVERROR_INVALIDDATA;
> > > >> > > index = ptr;
> > > >> > > ptr += cpp;
> > > >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext
> > > >> > > *avctx,
> > > >> > > void *data,
> > > >> > >
> > > >> > > x->pixels[ret] = color_string_to_rgba(ptr, len);
> > > >> > > ptr += mod_strcspn(ptr, ",") + 1;
> > > >> > > + if (end - ptr < 1)
> > > >> > > + return AVERROR_INVALIDDATA;
> > > >> > > }
> > > >> > >
> > > >> > > for (i = 0; i < avctx->height; i++) {
> > > >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]);
> > > >> > > + if (end - ptr < 1)
> > > >> > > + return AVERROR_INVALIDDATA;
> > > >> > > ptr += mod_strcspn(ptr, "\"") + 1;
> > > >> > > + if (end - ptr < 1)
> > > >> > > + return AVERROR_INVALIDDATA;
> > > >> > >
> > > >> > > for (j = 0; j < avctx->width; j++) {
> > > >> > > - if (ptr + cpp > end)
> > > >> > > + if (end - ptr < cpp)
> > > >> > > return AVERROR_INVALIDDATA;
> > > >> > >
> > > >> > > if ((ret = ascii2index(ptr, cpp)) < 0)
> > > >> > > @@ -405,6 +425,9 @@ static av_cold int
> > > >> > > xpm_decode_close(AVCodecContext
> > > >> > > *avctx)
> > > >> > > XPMDecContext *x = avctx->priv_data;
> > > >> > > av_freep(&x->pixels);
> > > >> > >
> > > >> > > + av_freep(&x->buf);
> > > >> > > + x->buf_size = 0;
> > > >> > > +
> > > >> > > return 0;
> > > >> > > }
> > > >> > >
> > > >> > > --
> > > >> > > 2.11.0
> > > >> > >
> > > >> > > _______________________________________________
> > > >> > > ffmpeg-devel mailing list
> > > >> > > ffmpeg-devel at ffmpeg.org
> > > >> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >> > >
> > > >> >
> > > >> > This is suboptimal solution.
> > > >>
> > > >> That comment isnt helpfull in improving the patch. It doesnt say
> > > >> what it is that you would like changed.
> > > >>
> > > >> I could probably replace string functions and avoid copying the whole
> > > >> packet if thats what you suggest ?
> > > >> it makes this bugfix more complex though
> > > >
> > > > ping, this is a security issue and needs to be fixed
> > >
> > >
> > > I told you that its unacceptable.
> > >
> >
> > > Do this: avpkt->data[avpkt->size] = 0;
> >
> > packet input to a decoder is not writable,
> > in fact it could be part of a memory maped file and litterally be in
> > non writable memory.
>
> You don't seem to know our API. You can make it writable.
its quite possible i dont know all the APIs ...
AVBufferRef can be made writable (which if it was not writable before
means malloc + copy compared to fast_malloc (reuse of buffer) + memcpy
in the patch).
On top of that AVPackets can use AVBufferRef but do not have to
complicating this and we really write after the packet not in the
packet which might add an additional corner case where the AVBufferRef
doesnt cover the byte we want to write into.
So, no doubt this can be done differently, and maybe theres even a
better way to do it than how i did. If you see a way to do this better
dont hesitate to change it.
I did look at how other decoders turn their input writable before i
wrote my implementation and what i found was doing the same
fast malloc + copy (svq3)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170513/d5e8672b/attachment.sig>
More information about the ffmpeg-devel
mailing list