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. > > 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) Ah, guess i should have mentioned that ossfuzz issue in ef13fafe22 (and e6ab99f324by extension) seeing i didn't push this one alongside it... > > thx > > [...] > > > _______________________________________________ > 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". > _______________________________________________ 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".
