[FFmpeg-devel] [PATCH] mjpegdec: Do not assume unused plane pointer are NULL.

[wm4]

On Fri, 26 Feb 2016 12:19:18 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On 26.02.2016, at 11:42, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> 
> > On Fri, Feb 26, 2016 at 11:35 AM, Reimar Döffinger
> > <Reimar.Doeffinger at gmx.de> wrote:  
> >> On 26.02.2016, at 02:38, Michael Niedermayer <michael at 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.Doeffinger at 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.

Well, that often happens to me too in general. You're free top audit
ALL the decoders and filters to use the number of planes instead of
checking plane pointers. As you see, there's a bit of a practical
problem with this. It doesn't help either making half of the API handle
this "correctly" (as you or any other sane person might consider it),
while the other half still exposes the other behavior.

Anyway, I'm not against this patch either. But you'll just bump into
this again after some time.

By the way, this inconsistency at hand has actually helped me a little
in the past. I can use the "free" plane pointers in hwaccel frames for
additional hwaccel-specific information.

> 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.

Maybe clarify it in the AVFrame doxygen. And again, I'm not against
this specific patch; it just feels a bit pointless and won't fix the
deeper problems.

You could also ask why an API user is setting unused plane pointers to
random values, though.