[FFmpeg-devel] [libav-devel] [PATCH] AMV: Fix possibly exploitable crash.

[Aℓex Converse]

On Thu, Apr 28, 2011 at 12:09 PM, Reinhard Tartler <siretart at tauware.de> wrote:
> On Thu, Apr 28, 2011 at 02:36:45PM -0400, Justin Ruggles wrote:
>> On 04/28/2011 01:20 PM, Aℓex Converse wrote:
>>
>> > On Thu, Apr 28, 2011 at 4:32 AM, Reinhard Tartler <siretart at tauware.de> wrote:
>> >> On Wed, Apr 27, 2011 at 03:37:27PM -0400, Justin Ruggles wrote:
>> >>> On 04/27/2011 03:25 PM, Reinhard Tartler wrote:
>> >>>
>> >>>> From: Michael Niedermayer <michaelni at gmx.at>
>> >>>>
>> >>>> Reported-at: Thu, 21 Apr 2011 14:38:25 +0000
>> >>>> Reported-by: Dominic Chell <Dominic.Chell at ngssecure.com>
>> >>>> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
>> >>>> (cherry picked from commit 89f903b3d5ec38c9c5d90fba7e626fa0eda61a32)
>> >>>> (cherry picked from commit 9b919571e506fbb72b81a35ca1e7c1bd6efc4209)
>> >>>> ---
>> >>>>  libavcodec/sp5xdec.c |    3 +--
>> >>>>  1 files changed, 1 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/libavcodec/sp5xdec.c b/libavcodec/sp5xdec.c
>> >>>> index e2c371a..3d01020 100644
>> >>>> --- a/libavcodec/sp5xdec.c
>> >>>> +++ b/libavcodec/sp5xdec.c
>> >>>> @@ -86,7 +86,6 @@ static int sp5x_decode_frame(AVCodecContext *avctx,
>> >>>>      recoded[j++] = 0xFF;
>> >>>>      recoded[j++] = 0xD9;
>> >>>>
>> >>>> -    avctx->flags &= ~CODEC_FLAG_EMU_EDGE;
>> >>>>      av_init_packet(&avpkt_recoded);
>> >>>>      avpkt_recoded.data = recoded;
>> >>>>      avpkt_recoded.size = j;
>> >>>> @@ -121,6 +120,6 @@ AVCodec ff_amv_decoder = {
>> >>>>      NULL,
>> >>>>      ff_mjpeg_decode_end,
>> >>>>      sp5x_decode_frame,
>> >>>> -    CODEC_CAP_DR1,
>> >>>> +    0,
>> >>>>      .long_name = NULL_IF_CONFIG_SMALL("AMV Video"),
>> >>>>  };
>> >>>
>> >>>
>> >>> The log message explains nothing.  What was the issue?  How is it
>> >>> related to CODEC_CAP_DR1 and CODEC_FLAG_EMU_EDGE?  Why change
>> >>> ff_amv_decoder and not ff_sp5x_decoder?
>> >>
>> >> No idea, and I'm not able to fill in the missing information. What shall
>> >> we do about this patch now? It seems that it has been picked up by Bugtraq:
>> >> http://seclists.org/bugtraq/2011/Apr/257
>> >>
>> >> And I have a request from the Debian security team to include this
>> >> patch: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=624339
>> >>
>> >
>> >> NGS Secure is going to withhold details of this flaw for three months.
>> >> This three month window will allow users the time needed to apply the
>> >> patch before the details are released to the general public. This
>> >> reflects the NGS Secure approach to responsible disclosure.
>> >
>> > I would complain to NGS about their "responsible disclosure" policy.
>
> I've already asked them nicely for additional information a few hours
> ago. Let's hope they react.
>
>> The EMU_EDGE part looks correct.  We should go ahead and commit that.  I
>> can't find any reason why CODEC_CAP_DR1 is being turned off for AMV
>> though.  I suggest we either ask Michael why this was done or for more
>> information, or we go ahead and comment-out that line (and backport to
>> the 0.6 branch) until NGS Secure releases details of the flaw.
>
> 20:52 <siretart> michaelni: could you please elaborate a bit what the
>                 dropping of CODEC_CAP_DR1 from the AMV codec actually
>                 fixes?
> 20:02 <michaelni> siretart, no. A patch that fixes the security bug is
>                  available, details will not be released before people
>                  had a chance to apply the patch, because these details
>                  would possibly help people exlpoit it. That is normal
>                  & common practice and dominiks bugtraq post also
>                  explains when details will be released.
>
I've said this before and I'll say it again, what an asshole!