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. _______________________________________________ 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".
