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

[Michael Niedermayer]

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 <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 ?
> 
> 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
-------------- 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/20200702/ae465123/attachment.sig>