[FFmpeg-devel] [PATCH 2/2] avformat: Add format_whitelist
Michael Niedermayer
michaelni at gmx.at
Wed Oct 1 00:50:08 CEST 2014
On Wed, Oct 01, 2014 at 12:12:43AM +0200, wm4 wrote:
> On Tue, 30 Sep 2014 23:33:51 +0200
> Michael Niedermayer <michaelni at gmx.at> wrote:
>
> > This allows restricting demuxers to a list of needed ones for improved security
> > Note, some demuxers themselfs open other demuxers, these are only restricted if
> > AVOptions are forwarded to them. Please check that your code does that.
> >
> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > ---
> > libavformat/avformat.h | 16 ++++++++++++++++
> > libavformat/options_table.h | 2 ++
> > libavformat/utils.c | 17 +++++++++++++++++
> > 3 files changed, 35 insertions(+)
> >
> > diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > index 78054de..875a1bd 100644
> > --- a/libavformat/avformat.h
> > +++ b/libavformat/avformat.h
> > @@ -1584,6 +1584,22 @@ typedef struct AVFormatContext {
> > */
> > int format_probesize;
> >
> > + /**
> > + * ',' seperated list of allowed decoders.
> > + * If NULL then all are allowed
> > + * - encoding: unused
> > + * - decoding: set by user through AVOPtions (NO direct access)
> > + */
> > + char *codec_whitelist;
> > +
> > + /**
> > + * ',' seperated list of allowed demuxers.
> > + * If NULL then all are allowed
> > + * - encoding: unused
> > + * - decoding: set by user through AVOPtions (NO direct access)
> > + */
> > + char *format_whitelist;
> > +
> > /*****************************************************************
> > * All fields below this line are not part of the public API. They
> > * may not be used outside of libavformat and can be changed and
> > diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> > index eb4115c..b08e93a 100644
> > --- a/libavformat/options_table.h
> > +++ b/libavformat/options_table.h
> > @@ -95,6 +95,8 @@ static const AVOption avformat_options[] = {
> > {"normal", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_COMPLIANCE_NORMAL }, INT_MIN, INT_MAX, D|E, "strict"},
> > {"experimental", "allow non-standardized experimental variants", 0, AV_OPT_TYPE_CONST, {.i64 = FF_COMPLIANCE_EXPERIMENTAL }, INT_MIN, INT_MAX, D|E, "strict"},
> > {"max_ts_probe", "maximum number of packets to read while waiting for the first timestamp", OFFSET(max_ts_probe), AV_OPT_TYPE_INT, { .i64 = 50 }, 0, INT_MAX, D },
> > +{"codec_whitelist", "List of decoders that are allowed to be used", OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D },
> > +{"format_whitelist", "List of demuxers that are allowed to be used", OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL }, CHAR_MIN, CHAR_MAX, D },
> > {NULL},
> > };
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 0fd1568..368e050 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -291,6 +291,11 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> > int av_demuxer_open(AVFormatContext *ic) {
> > int err;
> >
> > + if (ic->format_whitelist && av_match_list(ic->iformat->name, ic->format_whitelist, ',') <= 0) {
> > + av_log(ic, AV_LOG_ERROR, "Format not on whitelist\n");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > if (ic->iformat->read_header) {
> > err = ic->iformat->read_header(ic);
> > if (err < 0)
> > @@ -402,6 +407,13 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
> > if ((ret = init_input(s, filename, &tmp)) < 0)
> > goto fail;
> > s->probe_score = ret;
> > +
> > + if (s->format_whitelist && av_match_list(s->iformat->name, s->format_whitelist, ',') <= 0) {
> > + av_log(s, AV_LOG_ERROR, "Format not on whitelist\n");
> > + ret = AVERROR(EINVAL);
> > + goto fail;
> > + }
> > +
> > avio_skip(s->pb, s->skip_initial_bytes);
> >
> > /* Check filename in case an image number is expected. */
> > @@ -2576,6 +2588,8 @@ static int try_decode_frame(AVFormatContext *s, AVStream *st, AVPacket *avpkt,
> > /* Force thread count to 1 since the H.264 decoder will not extract
> > * SPS and PPS to extradata during multi-threaded decoding. */
> > av_dict_set(options ? options : &thread_opt, "threads", "1", 0);
> > + if (s->codec_whitelist)
> > + av_dict_set(options ? options : &thread_opt, "codec_whitelist", s->codec_whitelist, 0);
> > ret = avcodec_open2(st->codec, codec, options ? options : &thread_opt);
> > if (!options)
> > av_dict_free(&thread_opt);
> > @@ -3016,6 +3030,9 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
> > * SPS and PPS to extradata during multi-threaded decoding. */
> > av_dict_set(options ? &options[i] : &thread_opt, "threads", "1", 0);
> >
> > + if (ic->codec_whitelist)
> > + av_dict_set(options ? &options[i] : &thread_opt, "codec_whitelist", ic->codec_whitelist, 0);
> > +
> > /* Ensure that subtitle_header is properly set. */
> > if (st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE
> > && codec && !st->codec->codec) {
>
> This doesn't suffer from global mutable state anymore, which is good.
yes but it serves a different purpose
this can be set through AVOptions and thus the command line, its
not at the same level of security as not registering entities is.
both patchsets make sense and i would suggest to apply both
>
> Does this also put bounds on parsers etc.? I'm not too familiar with
> these parts, but it probably makes sense thinking about it to make it
> completely watertight.
no, parsers would need a seperate set of checks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141001/4319b871/attachment.asc>
More information about the ffmpeg-devel
mailing list