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 ? will this get applied ? should i apply my not so pretty fix for 23034 ? what will be done about releases ? can this be backported ? PS: please just make sure 23034/ is mentioned in the commit message so whatever fixes it can be kept track of (i know its already mentioned this is more intended as a remainder for other alternative fixes) thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you fake or manipulate statistics in a paper in physics you will never get a job again. If you fake or manipulate statistics in a paper in medicin you will get a job for life at the pharma industry.
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".
