| Message ID | 20230502193631.10844-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RFC] avformat: Add basic same origin check | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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? And do we want check_path to be default? It's a change in behavior. > {NULL}, > }; >
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"}, > And do we want check_path to be default? It's a change > in behavior. is it usefull if its not enabled by default ? [...]
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".
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 [...]
Quoting Michael Niedermayer (2023-05-02 21:36:31) > 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 Shouldn't these be flags instead?
Quoting Michael Niedermayer (2023-05-02 23:15:46) > 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. This changes behavior in an incompatible way, so IMO this should happen on a major bump. There should also be a note in the changelog. Perhaps there could be a special 'auto' value that would initially default to no effect, but would print a warning if a URL would stop working after the bump.
On Tue, May 2, 2023 at 10:57 PM James Almer <jamrial@gmail.com> wrote: > > > > 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. > Is there any estimation how many legitimate streams would be broken by these options? If any major streams don't work with this, then its not a good option, and eg. library users will likely just turn it off or to a lower setting, as proper streams just have to work - and log output is pretty much useless for API usage cases. A quick check for example shows that even something as simple as the HLS BBC Radio streams will fail _all_ checks, since the playlists are hosted on another host entirely as the media, thanks to akamai live streaming. Playlist here, as an example: http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 - Hendrik
On Wed, May 03, 2023 at 12:05:54PM +0200, Hendrik Leppkes wrote: > On Tue, May 2, 2023 at 10:57 PM James Almer <jamrial@gmail.com> wrote: > > > > > > 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. > > > > Is there any estimation how many legitimate streams would be broken by > these options? > If any major streams don't work with this, then its not a good option, > and eg. library users will likely just turn it off or to a lower > setting, as proper streams just have to work - and log output is > pretty much useless for API usage cases. > > A quick check for example shows that even something as simple as the > HLS BBC Radio streams will fail _all_ checks, since the playlists are > hosted on another host entirely as the media, thanks to akamai live > streaming. > Playlist here, as an example: > http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 yes, thats why it says RFC in the subject, i had expected that a bit already still OTOH, blocking these by default is the safer option, i mean if a user does a ./ffplay http://trustedfoobar.org/cutevideo.avi would she expect that video to access http://127.0.0.1/ and later http://evilhost/localwebscan-success I think this should not be possible by default settings, its unexpected maybe a whitelist of hosts or urls. Something the user could add *.akamaized.net to may be an option Thx [...]
Nit: different But is there an actual threat model whence it is necessary or even useful for a media framework to implement origin policies? On top of my head, this can be used by content providers to prevent third parties from referencing their media files... but that seems user-hostile; it does not provide any security for the user of FFmpeg. I could be wrong, but IMU, origin policy is meant to prevent harmful embedding of images and frames, and to prevent cross-site scripting, but FFmpeg doesn't support either if these anyway, so it's not concerned.
On Wed, May 3, 2023 at 12:49 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, May 03, 2023 at 12:05:54PM +0200, Hendrik Leppkes wrote: > > On Tue, May 2, 2023 at 10:57 PM James Almer <jamrial@gmail.com> wrote: > > > > > > > > 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. > > > > > > > Is there any estimation how many legitimate streams would be broken by > > these options? > > If any major streams don't work with this, then its not a good option, > > and eg. library users will likely just turn it off or to a lower > > setting, as proper streams just have to work - and log output is > > pretty much useless for API usage cases. > > > > A quick check for example shows that even something as simple as the > > HLS BBC Radio streams will fail _all_ checks, since the playlists are > > hosted on another host entirely as the media, thanks to akamai live > > streaming. > > Playlist here, as an example: > > http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 > > yes, thats why it says RFC in the subject, i had expected that a bit already > > still OTOH, blocking these by default is the safer option, i mean if a user > does a > ./ffplay http://trustedfoobar.org/cutevideo.avi > > would she expect that video to access http://127.0.0.1/ and later http://evilhost/localwebscan-success > I think this should not be possible by default settings, its unexpected > Coming from the other side -- If the user needs to set the flag for nearly all streams, then they are not going to check in the future and just set it, defeating the purpose of them. At which point we might as well not burden them. - Hendrik
Hi On Wed, May 03, 2023 at 02:16:03PM +0300, Rémi Denis-Courmont wrote: > Nit: different fixed > > But is there an actual threat model whence it is necessary or even useful for a media framework to implement origin policies? On top of my head, this can be used by content providers to prevent third parties from referencing their media files... but that seems user-hostile; it does not provide any security for the user of FFmpeg. > > I could be wrong, but IMU, origin policy is meant to prevent harmful embedding of images and frames, and to prevent cross-site scripting, but FFmpeg doesn't support either if these anyway, so it's not concerned. This patch was inspired by a report on ffmpeg-security about SSRF (for which custom io_open() callback or soem sort of sandboxing/VM can be used to avoid it) The patch here was intended to explore if we can provide something thats better tahn currently by default But the same issue with roles flipped occurs for the end user and the user cannot be expected to setup a custom io_open() callback for his player The current code can be also used to poke around the local network of the user. Which is unexpected by the user for example a avi file could be probed as a m3u8 playlist and then poke around on the local net while mixing that with remote urls from the timing of the remote accesses the remote party should be able to infer what happened with the local poking. Did it timeout? was the access rejected ? was there a file that was read and probed/played ? thx [...]
Le keskiviikkona 3. toukokuuta 2023, 16.33.59 EEST Michael Niedermayer a écrit : > This patch was inspired by a report on ffmpeg-security about SSRF > (for which custom io_open() callback or soem sort of sandboxing/VM can be > used to avoid it) > The patch here was intended to explore if we can provide something thats > better tahn currently by default I am not sure how a dodgy HLS manifest would be any different from the user clicking an hyperlink from a dodgy website - or opening a dodgy playlist file in their FFmpeg-based media player application for that matter. Either way, it can open any URL. It is obviously not an ideal situation, but any restriction here will most definitely break existing use cases (and likely be abused by server operators to lock FFmpeg out). Even the "obvious" blocking of secure (HTTPS) to nonsecure (HTTP) references is likely to break stuff. If the end result is that everybody just turns origin checking off, it's pretty pointless. > But the same issue with roles flipped occurs for the end user and the user > cannot be expected to setup a custom io_open() callback for his player > The current code can be also used to poke > around the local network of the user. Which is unexpected by the user > for example a avi file could be probed as a m3u8 playlist and then > poke around on the local net while mixing that with remote urls > from the timing of the remote accesses the remote party should be able > to infer what happened with the local poking. I agree, but it is unrealistic to change anything here. People make playlists mixed with local files and network file systems or cloud storage services. Yes, there is a slight information leakage. For instance, you can probe if a local file exists by interleaving local and remote URLs in a playlist. In practice, this is a well-known issue and has been for two at least decades, and the "solution" is to limit what the open file can do. To state the obvious extreme, one wouldn't want to execute a shell script or an executable from a playlist.
On Wed, May 03, 2023 at 07:07:09PM +0300, Rémi Denis-Courmont wrote: > Le keskiviikkona 3. toukokuuta 2023, 16.33.59 EEST Michael Niedermayer a écrit > : > > This patch was inspired by a report on ffmpeg-security about SSRF > > (for which custom io_open() callback or soem sort of sandboxing/VM can be > > used to avoid it) > > The patch here was intended to explore if we can provide something thats > > better tahn currently by default > > I am not sure how a dodgy HLS manifest would be any different from the user > clicking an hyperlink from a dodgy website - or opening a dodgy playlist file > in their FFmpeg-based media player application for that matter. Either way, it > can open any URL. The difference is with a dodgy link its the web browser that has to protect the user. With a dodgy HLS file its ffmpeg that has to protect the user. > > It is obviously not an ideal situation, but any restriction here will most > definitely break existing use cases (and likely be abused by server operators > to lock FFmpeg out). My goal is to make it more secure by default and to keep it reasonable convenient to the user So to me at least, if i open an hls file and i get a popup asking me "foobar.hls wants to access http://localhost/someexploit," "[Allow] [Deny] [Allow Always] [Deny Always]" thats a win and i wouldnt call that "Breaking" an existing usecase. Nor is that allowing any server operator to lock FFmpeg out For a non GUI app thats a matter of adjusting the command line or defaults by more classical means. If we can make this more convenient to the user while keeping it secure we should. But we should not make it more convenient than what can be done securely. > > Even the "obvious" blocking of secure (HTTPS) to nonsecure (HTTP) references > is likely to break stuff. If the end result is that everybody just turns origin > checking off, it's pretty pointless. > > > But the same issue with roles flipped occurs for the end user and the user > > cannot be expected to setup a custom io_open() callback for his player > > The current code can be also used to poke > > around the local network of the user. Which is unexpected by the user > > for example a avi file could be probed as a m3u8 playlist and then > > poke around on the local net while mixing that with remote urls > > from the timing of the remote accesses the remote party should be able > > to infer what happened with the local poking. > > I agree, but it is unrealistic to change anything here. People make playlists > mixed with local files and network file systems or cloud storage services. Yes, > there is a slight information leakage. For instance, you can probe if a local > file exists by interleaving local and remote URLs in a playlist. I dont know what software you are using but FFmpeg will prevent this attack with default protocol whitelists If you have a hls file that mixes local files and remote http(s) then you need to override the default protocol whitelist. If iam the user i would do that for that one file which in fact in that case really has to be my file i wrote. Of course nothing stops the user to set that by default for all urls, thats the users choice, but i think its much wiser not to do that thx [...]
On Wed, May 03, 2023 at 02:24:34PM +0200, Hendrik Leppkes wrote: > On Wed, May 3, 2023 at 12:49 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Wed, May 03, 2023 at 12:05:54PM +0200, Hendrik Leppkes wrote: > > > On Tue, May 2, 2023 at 10:57 PM James Almer <jamrial@gmail.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > Is there any estimation how many legitimate streams would be broken by > > > these options? > > > If any major streams don't work with this, then its not a good option, > > > and eg. library users will likely just turn it off or to a lower > > > setting, as proper streams just have to work - and log output is > > > pretty much useless for API usage cases. > > > > > > A quick check for example shows that even something as simple as the > > > HLS BBC Radio streams will fail _all_ checks, since the playlists are > > > hosted on another host entirely as the media, thanks to akamai live > > > streaming. > > > Playlist here, as an example: > > > http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 > > > > yes, thats why it says RFC in the subject, i had expected that a bit already > > > > still OTOH, blocking these by default is the safer option, i mean if a user > > does a > > ./ffplay http://trustedfoobar.org/cutevideo.avi > > > > would she expect that video to access http://127.0.0.1/ and later http://evilhost/localwebscan-success > > I think this should not be possible by default settings, its unexpected > > > > Coming from the other side -- If the user needs to set the flag for > nearly all streams, then they are not going to check in the future and > just set it, defeating the purpose of them. At which point we might as > well not burden them. Yes, we need a system that is secure and works in most cases. [...]
Le keskiviikkona 3. toukokuuta 2023, 22.05.26 EEST Michael Niedermayer a écrit : > On Wed, May 03, 2023 at 07:07:09PM +0300, Rémi Denis-Courmont wrote: > The difference is with a dodgy link its the web browser that has to protect > the user. With a dodgy HLS file its ffmpeg that has to protect the user. Well, the browser does not protect the user in that case. At best, an antimalware plugin or proxy might catch it but that's pretty much it. In fact, origin checks don't protect against this, nor are they intended to (AFAIK). > > It is obviously not an ideal situation, but any restriction here will most > > definitely break existing use cases (and likely be abused by server > > operators to lock FFmpeg out). > > My goal is to make it more secure by default and to keep it reasonable > convenient to the user > > So to me at least, if i open an hls file and i get a popup asking me > "foobar.hls wants to access http://localhost/someexploit," > "[Allow] [Deny] [Allow Always] [Deny Always]" > thats a win and i wouldnt call that "Breaking" an existing usecase. > Nor is that allowing any server operator to lock FFmpeg out User dialogues are notoriously bad for security purposes, as users don't really have the necessary info to make the proper decision, and just learn to click Allow Always. All they achieve is absolve the developers from protecting the user, really. But even if, what will the criterion be? You'll match localhost. Okay. What about localhost.localdomain and 127.0.0.1? What about all the noncanonical writings of 127.0.0.1 and other addresses in 127.0.0.0/8, and their noncanonical writing? IPv6? What about the assigned external IP address of the host And so on, and that's just to prevent connections to localhost. Some browsers prevent connections to private IP addresses too, to "protect" the Intranet, but the flip side is to break corporate content and home file servers. > If we can make this more convenient to the user while keeping it secure we > should. But we should not make it more convenient than what can be done > securely. I appreciate the thought but without a clearly defined threat model, you can't define the security issue, and thus you can't hope to fix it. Meanwhile, you will break things, and if you're not careful, you might as well make it easier for publishers to deliberately block FFmpeg. > If you have a hls file that mixes local files and remote http(s) then you > need to override the default protocol whitelist. It makes sense for HLS manifests because HLS is by definition tied to HTTP, but that won't work for real playlists (M3U or other).
On 03.05.2023 21:08, Michael Niedermayer wrote: >>>> A quick check for example shows that even something as simple as the >>>> HLS BBC Radio streams will fail _all_ checks, since the playlists are >>>> hosted on another host entirely as the media, thanks to akamai live >>>> streaming. >>>> Playlist here, as an example: >>>> http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 >>> >>> yes, thats why it says RFC in the subject, i had expected that a bit already >>> >>> still OTOH, blocking these by default is the safer option, i mean if a user >>> does a >>> ./ffplay http://trustedfoobar.org/cutevideo.avi >>> >>> would she expect that video to access http://127.0.0.1/ and later http://evilhost/localwebscan-success >>> I think this should not be possible by default settings, its unexpected >>> >> >> Coming from the other side -- If the user needs to set the flag for >> nearly all streams, then they are not going to check in the future and >> just set it, defeating the purpose of them. At which point we might as >> well not burden them. > > Yes, we need a system that is secure and works in most cases. What about doing what actual browsers do, and reading the Access-Control-Allow-Origin HTTP header, and checking if the current origin is allowed? This does not really work for local files. Best you could do is check for "*" or not. But would at least fix the BBC+Akamai case.
On Wed, May 03, 2023 at 11:01:43PM +0200, Timo Rothenpieler wrote: > On 03.05.2023 21:08, Michael Niedermayer wrote: > > > > > A quick check for example shows that even something as simple as the > > > > > HLS BBC Radio streams will fail _all_ checks, since the playlists are > > > > > hosted on another host entirely as the media, thanks to akamai live > > > > > streaming. > > > > > Playlist here, as an example: > > > > > http://a.files.bbci.co.uk/media/live/manifesto/audio/simulcast/hls/nonuk/sbr_low/ak/bbc_radio_one.m3u8 > > > > > > > > yes, thats why it says RFC in the subject, i had expected that a bit already > > > > > > > > still OTOH, blocking these by default is the safer option, i mean if a user > > > > does a > > > > ./ffplay http://trustedfoobar.org/cutevideo.avi > > > > > > > > would she expect that video to access http://127.0.0.1/ and later http://evilhost/localwebscan-success > > > > I think this should not be possible by default settings, its unexpected > > > > > > > > > > Coming from the other side -- If the user needs to set the flag for > > > nearly all streams, then they are not going to check in the future and > > > just set it, defeating the purpose of them. At which point we might as > > > well not burden them. > > > > Yes, we need a system that is secure and works in most cases. > > What about doing what actual browsers do, and reading the > Access-Control-Allow-Origin HTTP header, and checking if the current origin > is allowed? > > This does not really work for local files. Best you could do is check for > "*" or not. > But would at least fix the BBC+Akamai case. I like the idea, do you want to implement it ? thx [...]
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"}, {NULL}, };
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(+)