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

[James Almer <jamrial@gmail.com>]

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.

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

> 
> 
> [...]
> 
> 
> _______________________________________________
> 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".
_______________________________________________
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".