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 <siret...@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 <michae...@gmx.at>
> >>>>
> >>>> Reported-at: Thu, 21 Apr 2011 14:38:25 +0000
> >>>> Reported-by: Dominic Chell <dominic.ch...@ngssecure.com>
> >>>> Signed-off-by: Michael Niedermayer <michae...@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.
> Unless someone else wants to dig deeper to try to figure out why it was
> done.
What does this CODEC_CAP_DR1 flag do anyway? From its documentation,
removing it doesn't change its behavior at all, but is rather an
indication to the application about whether the codec does its own
buffer management or if a custom get_buffer() handler can be used.
From a related thread:
,----[http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/2414/focus=2418
| > The reason I ask is that I can not understand why a decoder would
| > need to declare CODEC_CAP_DR1 in its list of capabilities. Is this really
| > necessary?
| hmm, the flag is really a bit useless since the get_buffer() stuff
| except that some codecs (like binary only ones) might not be able to draw
into
| user defined buffers, but that seems to be the only reason i can think of
| currently, all codecs support DR1 (except mjpeg but its propably easy to fix)
| the additional edge/boarder area used for MC by some codecs isnt an issue
| either as all codecs which need it can optionally copy the small affected
| area to a temp buffer
`----
With this in mind, I think Ruggles suggestion seems correct to me.
regards,
Reinhard
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
