On 26.02.2016, at 11:42, Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> On Fri, Feb 26, 2016 at 11:35 AM, Reimar Döffinger > <reimar.doeffin...@gmx.de> wrote: >> On 26.02.2016, at 02:38, Michael Niedermayer <mich...@niedermayer.cc> wrote: >> >>> On Fri, Feb 26, 2016 at 12:15:19AM +0100, Reimar Döffinger wrote: >>>> We do neither document nor check such a requirement >>>> and for application-provided get_buffer2 they could >>>> contain the result of a malloc(0) or whatever value >>>> they had previously. >>>> This fixes a use-after-free in e.g. MPlayer: >>>> https://trac.mplayerhq.hu/ticket/2262 >>>> We might want to consider changing the (documented) >>>> API in addition though. >>>> >>>> Signed-off-by: Reimar Döffinger <reimar.doeffin...@gmx.de> >>>> --- >>>> libavcodec/mjpegdec.c | 8 +++++--- >>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> the assumtation that unused plane pointers are NULL is more >>> widespread than mjpeg i think >> >> Possible, but it's the only place someone ran across so far. >> Decoders that cannot reconfigure are also much less likely to be affected by >> stale pointers. >> >>> also, is it really a good idea to leave stale pointers in the array? >> >> No, of course not. The question from my point of view is rather: should >> stale pointers or malloc(0) lead directly to an exploitable buffer overflow? >> Because that's what the current code does. >> So I think we should avoid these lazy NULL checks as a matter of code >> resilience. >> I think it would also be reasonable to extend e.g. code calling get_buffer2 >> to validate the result, though I have a slight concern that that is a bit of >> an API change. > > I mostly agree with Michael and wm4, this would appear to be a bug in > the calling code more than anything else. I am not exactly sure what their opinion is to be honest. However this attitude of "it's of course a bug in the calling code! Nobody can actually know it because it's 100% undocumented requirement but it's their bug" pisses me of quite a bit. Yes, in case of MPlayer where I ran across it, I consider it a bug. The fact that we do not document, enforce it and that for most codecs it actually works fine either way so it's in fact an issue a normal user of the library cannot discover makes this very much a bug on FFmpeg side to me. > But on the other hand there is nothing really wrong with fixing this > particular case either, but personally I wouldn't feel it worth it to > go chase through the entire code base trying to find all such cases. > > For adding checks, would at the very least have to be careful when it > comes to hwaccels, as they store random pointers in the data pointers > at varying offsets. So nobody has said a single thing about what you think _should_ be done. Just leave it and hey, if someone has the unbelievable gall of making a mistake and not setting a pointer to NULL (which like wm4 pointed out has happened to him, too) they deserve to have an exploit? I really don't feel like idle discussions that lead nowhere. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
