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

[Reimar Döffinger]

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