On Wed, Jul 01, 2020 at 10:22:48AM -0300, James Almer wrote: > On 7/1/2020 6:40 AM, Michael Niedermayer wrote: > > On Sun, Jun 07, 2020 at 12:45:15PM -0300, James Almer wrote: > >> On 6/7/2020 12:20 PM, James Almer wrote: > >>> On 6/7/2020 11:45 AM, Michael Niedermayer wrote: > >>>> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote: > >>>>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is > >>>>> written right > >>>>> after reading it using the same CBS context (hevc_metadata), the list > >>>>> of parsed > >>>>> parameters sets used by the writer must not be the one that's the > >>>>> result of the > >>>>> reader having already parsed the current Access Unit in question. > >>>>> > >>>>> Fixes: out of array access > >>>>> Fixes: > >>>>> 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz > >>>>> > >>>>> Found-by: continuous fuzzing process > >>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>>> Signed-off-by: James Almer <jamr...@gmail.com> > >>>>> --- > >>>>> An alternative is forcing the usage of separate CBS contexts for > >>>>> reading and > >>>>> writing. > >>>>> > >>>>> libavcodec/cbs_h264.h | 18 ++++++++--- > >>>>> libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++---------- > >>>>> libavcodec/cbs_h265.h | 26 +++++++++++---- > >>>>> 3 files changed, 89 insertions(+), 27 deletions(-) > >>>> > >>>> i think the change is probably ok and it fixes the issue > >>>> what i feel uneasy about is if this is the sole way the security > >>>> issue is fixed. > >>>> > >>>> let me try to explain what i mean by a simpler example: > >>>> if you have a sprintf() that overwrites the buffer there are 3 ways > >>>> to fix that > >>>> A. You make the buffer big enough for what is written > >>>> B. You make the amount written only as large as the buffer > >>>> C. You check by using snprintf() > >>>> > >>>> Now like here A/B may represent a bugfix > >>>> the problem with A/B is that security rests on potentially complex code > >>>> So even when A/B is done, we also should do C > >>>> > >>>> This patch fixes the inconsistency on the write side be keeping more > >>>> references > >>>> to the parameter sets. > >>>> For security one would have to proof that no crafted input to the reader > >>>> fed into any available useer of the reader could result in an > >>>> inconsistency > >>>> to a writer following. > >>>> Are you sure thats the case now and with future users of the code ? > >>>> OTOH as dumb as a check in the writer may look, anyone can proof it > >>>> fixes the > >>>> specific inconsistency. > >>>> > >>>> What i suggest specifically is to also include or apply the simple check > >>>> on top of this, or a equivalent / more generic check. So that security > >>>> does not > >>>> rest on alot of spread out code > >>>> > >>>> Thanks > >>> > >>> Well, one possibility is that after this, the infer() warning could be > >>> replaced with an assert() instead. The CBS framework is not public, so > >>> crashing with an assert() would be acceptable as infer() failing in > >>> writing scenarios should be considered an internal bug (bitstream > >>> filters, or any CBS user for that matter, should ensure to not change > >>> fields in a way that would result in an invalid bitstream and thus > >>> failing infer() checks). > >>> > >>> The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag > >>> is 1 in this scenario, then we should stop to avoid out of array > >>> access", but as "We did something wrong because > >>> inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at > >>> this point". So using assert() after this patch sounds like a good > >>> solution and will help detect future bugs in the parsing code. > >> > >> It could also be a generic return AVERROR_INVALIDDATA as you suggested, > >> to be fair. All the standard writing helpers abort gracefully that way, > >> so infer() could do the same. > >> > >> Or the other helpers could be changed to assert(). > > > > whats the status of this ? > > has this issue been fixed in some other way i missed ? > > I pushed the first two patches and backported them, so the issue should > be fixed by the first. > > > will this get applied ? > > I delayed applying this one waiting for more opinions, especially > Mark's, since it's kind of ugly. > > > should i apply my not so pretty fix for 23034 ? > > what will be done about releases ? can this be backported ? > > Already answered above, but maybe confirm it's fixed just to be sure.
23034 no longer reproduces on master locally thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
