| Message ID | 20200531135020.5675-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 4b2248594c7faf1530c43e662c4b80655240b269 |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/jpeg2000dec: clear pointer which become stale in get_ppt() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sun, May 31, 2020 at 7:21 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: use after free > Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/jpeg2000dec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > index 65555424ed..b7766459c4 100644 > --- a/libavcodec/jpeg2000dec.c > +++ b/libavcodec/jpeg2000dec.c > @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > tile->packed_headers = new; > } else > return AVERROR(ENOMEM); > + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); > memcpy(tile->packed_headers + tile->packed_headers_size, > s->g.buffer, n - 3); > tile->packed_headers_size += n - 3; > -- > 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". I do not understand this. Wouldn't the memcpy overwrite the address anyway?
On Sun, May 31, 2020 at 09:22:46PM +0530, Gautam Ramakrishnan wrote: > On Sun, May 31, 2020 at 7:21 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > Fixes: use after free > > Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/jpeg2000dec.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > index 65555424ed..b7766459c4 100644 > > --- a/libavcodec/jpeg2000dec.c > > +++ b/libavcodec/jpeg2000dec.c > > @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > tile->packed_headers = new; > > } else > > return AVERROR(ENOMEM); > > + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); > > memcpy(tile->packed_headers + tile->packed_headers_size, > > s->g.buffer, n - 3); > > tile->packed_headers_size += n - 3; > > -- > > 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". > > I do not understand this. Wouldn't the memcpy overwrite the address anyway? The "GetByteContext packed_headers_stream" can point to the stream prior to av_realloc(), and after realloc its no longer valid that needs to be cleared because if not, its possible this will be used and crash later. Note, this is not a valid jpeg2000 file, but some trash from the fuzzer. Its certainly possible to add a check elsewhere to avoid this. the memset was just the obvious solution that came to my mind. Also on this subject its quite possible that the jpeg2000 code is missing more such saftey checks Iam mentioning this as you are working on jpeg2000. its always a good idea to keep the question "can this be made to do something nasty from crafted input" in mind when writing code. thx [...]
On Sun, May 31, 2020 at 10:12 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, May 31, 2020 at 09:22:46PM +0530, Gautam Ramakrishnan wrote: > > On Sun, May 31, 2020 at 7:21 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > Fixes: use after free > > > Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/jpeg2000dec.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > > index 65555424ed..b7766459c4 100644 > > > --- a/libavcodec/jpeg2000dec.c > > > +++ b/libavcodec/jpeg2000dec.c > > > @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > > tile->packed_headers = new; > > > } else > > > return AVERROR(ENOMEM); > > > + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); > > > memcpy(tile->packed_headers + tile->packed_headers_size, > > > s->g.buffer, n - 3); > > > tile->packed_headers_size += n - 3; > > > -- > > > 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". > > > > I do not understand this. Wouldn't the memcpy overwrite the address anyway? > > The "GetByteContext packed_headers_stream" can point to the stream prior > to av_realloc(), and after realloc its no longer valid > that needs to be cleared because if not, its possible this will be used and > crash later. Yep, got it. > > Note, this is not a valid jpeg2000 file, but some trash from the fuzzer. > Its certainly possible to add a check elsewhere to avoid this. > the memset was just the obvious solution that came to my mind. > > Also on this subject its quite possible that the jpeg2000 code is missing > more such saftey checks > Iam mentioning this as you are working on jpeg2000. its always a good > idea to keep the question "can this be made to do something nasty from crafted input" > in mind when writing code. I shall keep this in mind. > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The educated differ from the uneducated as much as the living from the > dead. -- Aristotle > _______________________________________________ > 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".
On Sun, May 31, 2020 at 7:21 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: use after free > Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/jpeg2000dec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > index 65555424ed..b7766459c4 100644 > --- a/libavcodec/jpeg2000dec.c > +++ b/libavcodec/jpeg2000dec.c > @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > tile->packed_headers = new; > } else > return AVERROR(ENOMEM); > + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); > memcpy(tile->packed_headers + tile->packed_headers_size, > s->g.buffer, n - 3); > tile->packed_headers_size += n - 3; > -- > 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". Looks good to me
On Mon, Jun 01, 2020 at 12:10:54AM +0530, Gautam Ramakrishnan wrote: > On Sun, May 31, 2020 at 7:21 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > Fixes: use after free > > Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/jpeg2000dec.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c > > index 65555424ed..b7766459c4 100644 > > --- a/libavcodec/jpeg2000dec.c > > +++ b/libavcodec/jpeg2000dec.c > > @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) > > tile->packed_headers = new; > > } else > > return AVERROR(ENOMEM); > > + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); > > memcpy(tile->packed_headers + tile->packed_headers_size, > > s->g.buffer, n - 3); > > tile->packed_headers_size += n - 3; > > -- > > 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". > > Looks good to me will apply thx [...]
diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c index 65555424ed..b7766459c4 100644 --- a/libavcodec/jpeg2000dec.c +++ b/libavcodec/jpeg2000dec.c @@ -928,6 +928,7 @@ static int get_ppt(Jpeg2000DecoderContext *s, int n) tile->packed_headers = new; } else return AVERROR(ENOMEM); + memset(&tile->packed_headers_stream, 0, sizeof(tile->packed_headers_stream)); memcpy(tile->packed_headers + tile->packed_headers_size, s->g.buffer, n - 3); tile->packed_headers_size += n - 3;
Fixes: use after free Fixes: 22484/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5671488765296640 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/jpeg2000dec.c | 1 + 1 file changed, 1 insertion(+)