[FFmpeg-devel] [PATCH] [RFC] avformat: Add basic same origin check
Michael Niedermayer
michael at niedermayer.cc
Wed May 3 00:15:46 EEST 2023
On Tue, May 02, 2023 at 05:57:09PM -0300, James Almer wrote:
> On 5/2/2023 5:16 PM, Michael Niedermayer wrote:
> > On Tue, May 02, 2023 at 05:00:29PM -0300, James Almer wrote:
> > > On 5/2/2023 4:36 PM, Michael Niedermayer wrote:
> > > > TODO: bump minor version, add docs
> > > >
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > > libavformat/avformat.h | 10 ++++++++++
> > > > libavformat/options.c | 29 +++++++++++++++++++++++++++++
> > > > libavformat/options_table.h | 3 +++
> > > > 3 files changed, 42 insertions(+)
> > > >
> > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > > > index 1916aa2dc5..5ff77323ba 100644
> > > > --- a/libavformat/avformat.h
> > > > +++ b/libavformat/avformat.h
> > > > @@ -1713,6 +1713,16 @@ typedef struct AVFormatContext {
> > > > * @return 0 on success, a negative AVERROR code on failure
> > > > */
> > > > int (*io_close2)(struct AVFormatContext *s, AVIOContext *pb);
> > > > +
> > > > + /**
> > > > + * Perform basic same origin checks in default io_open()
> > > > + * - encoding: set by user
> > > > + * - decoding: set by user
> > > > + */
> > > > + int same_origin_check;
> > > > +#define AVFMT_SAME_ORIGIN_CHECK_NONE 0 //no check
> > > > +#define AVFMT_SAME_ORIGIN_CHECK_HOST 1 //protocol, host, auth, port
> > > > +#define AVFMT_SAME_ORIGIN_CHECK_PATH 2 //protocol, host, auth, port, parent path
> > > > } AVFormatContext;
> > > > /**
> > > > diff --git a/libavformat/options.c b/libavformat/options.c
> > > > index e4a3aceed0..7db4bc9b38 100644
> > > > --- a/libavformat/options.c
> > > > +++ b/libavformat/options.c
> > > > @@ -26,6 +26,7 @@
> > > > #include "libavcodec/codec_par.h"
> > > > #include "libavutil/avassert.h"
> > > > +#include "libavutil/avstring.h"
> > > > #include "libavutil/internal.h"
> > > > #include "libavutil/intmath.h"
> > > > #include "libavutil/opt.h"
> > > > @@ -148,6 +149,34 @@ static int io_open_default(AVFormatContext *s, AVIOContext **pb,
> > > > av_log(s, loglevel, "Opening \'%s\' for %s\n", url, flags & AVIO_FLAG_WRITE ? "writing" : "reading");
> > > > + if (s->same_origin_check) {
> > > > + URLComponents uc;
> > > > + int err;
> > > > + size_t len;
> > > > + const char *end;
> > > > + err = ff_url_decompose(&uc, s->url, NULL);
> > > > + if (err < 0)
> > > > + return err;
> > > > +
> > > > + if (s->same_origin_check == AVFMT_SAME_ORIGIN_CHECK_PATH) {
> > > > + end = uc.query;
> > > > + while (end > uc.path && *end != '/')
> > > > + end--;
> > > > + } else
> > > > + end = uc.path;
> > > > +
> > > > + len = end - s->url;
> > > > + if (strncmp(url, s->url, len)) {
> > > > + av_log(s, AV_LOG_ERROR, "Blocking url with differnt origin\n");
> > > > + return AVERROR(EIO);
> > > > + }
> > > > + if (s->same_origin_check == AVFMT_SAME_ORIGIN_CHECK_PATH &&
> > > > + av_strnstr(url + len, "/../", uc.query - end)) {
> > > > + av_log(s, AV_LOG_ERROR, "Blocking url tricks\n");
> > > > + return AVERROR(EIO);
> > > > + }
> > > > + }
> > > > +
> > > > return ffio_open_whitelist(pb, url, flags, &s->interrupt_callback, options, s->protocol_whitelist, s->protocol_blacklist);
> > > > }
> > > > diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> > > > index 86d836cfeb..da788164f1 100644
> > > > --- a/libavformat/options_table.h
> > > > +++ b/libavformat/options_table.h
> > > > @@ -106,6 +106,9 @@ static const AVOption avformat_options[] = {
> > > > {"max_streams", "maximum number of streams", OFFSET(max_streams), AV_OPT_TYPE_INT, { .i64 = 1000 }, 0, INT_MAX, D },
> > > > {"skip_estimate_duration_from_pts", "skip duration calculation in estimate_timings_from_pts", OFFSET(skip_estimate_duration_from_pts), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, D},
> > > > {"max_probe_packets", "Maximum number of packets to probe a codec", OFFSET(max_probe_packets), AV_OPT_TYPE_INT, { .i64 = 2500 }, 0, INT_MAX, D },
> > > > +{"same_origin", "same origin check", OFFSET(same_origin_check) , AV_OPT_TYPE_INT , { .i64 = AVFMT_SAME_ORIGIN_CHECK_PATH }, 0, INT_MAX, D|E, "same_origin"},
> > > > +{"same_host" , "same protocol, host, port, auth", 0 , AV_OPT_TYPE_CONST, { .i64 = AVFMT_SAME_ORIGIN_CHECK_HOST }, 0, INT_MAX, D|E, "same_origin"},
> > > > +{"same_path" , "same protocol, host, port, auth, parent path", 0 , AV_OPT_TYPE_CONST, { .i64 = AVFMT_SAME_ORIGIN_CHECK_PATH }, 0, INT_MAX, D|E, "same_origin"},
> > >
> > > Missing NONE const?
> >
> > added
> > +{"same_none" , "same origin check off" , 0 , AV_OPT_TYPE_CONST, { .i64 = AVFMT_SAME_ORIGIN_CHECK_NONE }, 0, INT_MAX, D|E, "same_origin"},
>
> "none" sounds more natural.
ill change it
>
> >
> >
> > > And do we want check_path to be default? It's a change
> > > in behavior.
> >
> > is it usefull if its not enabled by default ?
>
> It is, since it can be enabled, like the whitelists and blacklists, but the
> question is if it's preferable to have it enabled. If you consider it so,
> then it's good and i wont oppose it.
the problem with default-disabled is that the user needs to know
1. that the option exist
2. what the option does
3. what an attacker can do with such urls
4. that its not enabled by default
OTOH if its enabled by default, the worst it can do is fail with a error
the user can lookup the error and disable the option
but i may be missing something here, also comments both from people
who regularly work with hls and anything else contaning urls in files
and also people who dealt with any related attacks is welcome.
The goal is that this actually does something useful in reality.
thx
[...]
--
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: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230502/eee938a2/attachment.sig>
More information about the ffmpeg-devel
mailing list