[FFmpeg-devel] [PATCH 2/3] avformat: reject FFmpeg-style merged side data in raw packets

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 01:40:11PM +0100, wm4 wrote:
> It looks like this could lead to security issues, as side data readers

anything could, but side data wasnt excluded from fuzzing, in fact
ive seen fuzzers find and trigger the split code
also a demuxer and user app could always set side data to something
bad.
All uses of side data must assume the data to be untrusted, because
it is even without all the split code.


> will for example rely on side data allocation sizes to be as large as
> needed for correct operation.

Is this really so ?
we allocate at least AV_INPUT_BUFFER_PADDING_SIZE, accesses beyond
that require a check on the size.

also size checks are needed for the case where a lib gets replaced by
one with newer version that has a bigger struct.


> If such files exist at all, they also
> should be brought out of circulation, so fully reject them. Under normal
> circumstances, nothing creates such files.
> 
> To avoid problems for now, we let the concat and hls demuxers do this
> (they merely return previously-demuxed packets, whose side data might
> have been merged by libavformat itself after demuxing). The
> special-cases will be removed in the next commit.
> ---
>  libavformat/utils.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 37d7024465..68f3c977d6 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -840,6 +840,15 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>              *pkt = tmp;
>          }
>  
> +        if (strcmp(s->iformat->name, "concat") && strcmp(s->iformat->name, "hls,applehttp")

can strcmp() be avoided here ?
it would be run per packet which is rather ugly


> +            && av_packet_split_side_data(pkt) == 1) {

this could fail with ENOMEM which complicates things

if we do block such packets, its prbobably better to add a new
static inline function to a header that checks if a packet has
splitable side data and use that in av_packet_split_side_data() and
here

Iam suggesting "static inline" here to make backporting easier so no
new symbol is added to libavcodec that is needed by libavformat

also it may be interresting to disable this check for fuzzing so
side data can be fuzzed in a wider range of cases and any past
testcases that happen to use this can still be used for regression
testing

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170308/17a6fd6d/attachment.sig>