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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- 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/20200607/e92ce7b6/attachment.sig>