| Message ID | 20170511003833.15087-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | cb243972b121b1ae6b60a78ff55a0506c69f3879 |
| Headers | show |
On 5/11/17, Michael Niedermayer <michael@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > This is suboptimal solution.
On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > On 5/11/17, Michael Niedermayer <michael@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@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@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 [...]
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@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@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@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 [...]
On 5/12/17, Michael Niedermayer <michael@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@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@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@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;
Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit :
> I told you that its unacceptable.
And I told you, before you pushed, that your cowboy-style parser was
unacceptable. You chose to disregard it.
Regards,
On 5/12/17, Nicolas George <george@nsup.org> wrote: > Le tridi 23 floreal, an CCXXV, Paul B Mahol a ecrit : >> I told you that its unacceptable. > > And I told you, before you pushed, that your cowboy-style parser was > unacceptable. You chose to disregard it. Because I assumed packets are padded with zeroes.
On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > On 5/12/17, Michael Niedermayer <michael@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@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@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@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. [...]
On 5/12/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: >> On 5/12/17, Michael Niedermayer <michael@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@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@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@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. In theory and/or practice?
On 5/12/17, Paul B Mahol <onemda@gmail.com> wrote: > On 5/12/17, Michael Niedermayer <michael@niedermayer.cc> wrote: >> On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: >>> On 5/12/17, Michael Niedermayer <michael@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@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@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@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. > > In theory and/or practice? > Feel free to commit this until I came with better solution.
Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit :
> Because I assumed packets are padded with zeroes.
Because you chose to write quick-and-dirty instead of robust.
Regards,
On 5/12/17, Nicolas George <george@nsup.org> wrote: > Le tridi 23 floreal, an CCXXV, Paul B Mahol a ecrit : >> Because I assumed packets are padded with zeroes. > > Because you chose to write quick-and-dirty instead of robust. Thank you.
On Fri, May 12, 2017 at 08:58:52PM +0200, Paul B Mahol wrote: > On 5/12/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > >> On 5/12/17, Michael Niedermayer <michael@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@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@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@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. > > In theory and/or practice? I do not know if any application does this in practice. [...]
On Fri, May 12, 2017 at 09:13:51PM +0200, Paul B Mahol wrote: > On 5/12/17, Paul B Mahol <onemda@gmail.com> wrote: > > On 5/12/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > >>> On 5/12/17, Michael Niedermayer <michael@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@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@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@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. > > > > In theory and/or practice? > > > > Feel free to commit this until I came with better solution. ok thanks [...]
On Fri, 12 May 2017 20:55:13 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > > On 5/12/17, Michael Niedermayer <michael@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@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@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@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.
On Fri, 12 May 2017 18:21:07 +0200 Nicolas George <george@nsup.org> wrote: > Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit : > > I told you that its unacceptable. > > And I told you, before you pushed, that your cowboy-style parser was > unacceptable. You chose to disregard it. AFAIK all of our subtitle parsers make the same assumption.
On Sat, May 13, 2017 at 08:38:52AM +0200, wm4 wrote: > On Fri, 12 May 2017 20:55:13 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > > > On 5/12/17, Michael Niedermayer <michael@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@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@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@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) [...]
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; }
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@niedermayer.cc> --- libavcodec/xpmdec.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)