Re: [FFmpeg-devel] [PATCH] [RFC] avformat: Add basic same origin check

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 6212 bytes --]

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@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".