[FFmpeg-devel] [PATCH] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing

[Michael Niedermayer]

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 <jamrial at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200701/d71d9631/attachment.sig>