| Message ID | 20170602191914.32001-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jun 2, 2017 at 9:19 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > This reduces the attack surface of local file-system and local network > information leaking. > > It prevents the existing exploit leading to an information leak. As > well as similar hypothetical attacks. > > Leaks of information from files and symlinks ending in common multimedia extensions > are still possible. But files with sensitive information like private keys and passwords > generally do not use common multimedia filename extensions. > > The existing exploit depends on a specific decoder as well. > It does appear though that the exploit should be possible with any decoder. > The problem is that as long as sensitive information gets into the decoder, > the output of the decoder becomes sensitive as well. > The only obvious solution is to prevent access to sensitive information. Or to > disable hls or possibly some of its feature. More complex solutions like > checking the path to limit access to only subdirectories of the hls path may > work as an alternative. But such solutions are fragile and tricky to implement > portably and would not stop every possible attack nor would they work with all > valid hls files. > > Developers have expressed their dislike / objected to disabling hls by default as well > as disabling hls with local files. This here is a less robust but also lower > inconvenience solution. > It can be applied stand alone or together with other solutions. > One of the most important things is that at the very least HLS keeps working out of the box without magic options for actual HTTP HLS streams, ie. their primary domain. One aspect of this is that HLS streams hosted by CDNs often don't have an actual extension, since they get generated by various dynamic URLs, and this would break that, so its not a good idea. No matter what security fixes get applied, if HLS basically stops working its basic functionality, then its going too far. - Hendrik
On Fri, Jun 02, 2017 at 09:27:16PM +0200, Hendrik Leppkes wrote: > On Fri, Jun 2, 2017 at 9:19 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > This reduces the attack surface of local file-system and local network > > information leaking. > > > > It prevents the existing exploit leading to an information leak. As > > well as similar hypothetical attacks. > > > > Leaks of information from files and symlinks ending in common multimedia extensions > > are still possible. But files with sensitive information like private keys and passwords > > generally do not use common multimedia filename extensions. > > > > The existing exploit depends on a specific decoder as well. > > It does appear though that the exploit should be possible with any decoder. > > The problem is that as long as sensitive information gets into the decoder, > > the output of the decoder becomes sensitive as well. > > The only obvious solution is to prevent access to sensitive information. Or to > > disable hls or possibly some of its feature. More complex solutions like > > checking the path to limit access to only subdirectories of the hls path may > > work as an alternative. But such solutions are fragile and tricky to implement > > portably and would not stop every possible attack nor would they work with all > > valid hls files. > > > > Developers have expressed their dislike / objected to disabling hls by default as well > > as disabling hls with local files. This here is a less robust but also lower > > inconvenience solution. > > It can be applied stand alone or together with other solutions. > > > > One of the most important things is that at the very least HLS keeps > working out of the box without magic options for actual HTTP HLS > streams, ie. their primary domain. > One aspect of this is that HLS streams hosted by CDNs often don't have > an actual extension, since they get generated by various dynamic URLs, > and this would break that, so its not a good idea. please provide an example that fails with the patch > > No matter what security fixes get applied, if HLS basically stops > working its basic functionality, then its going too far. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Sat, Jun 3, 2017 at 2:31 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Jun 02, 2017 at 09:27:16PM +0200, Hendrik Leppkes wrote: >> On Fri, Jun 2, 2017 at 9:19 PM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> > This reduces the attack surface of local file-system and local network >> > information leaking. >> > >> > It prevents the existing exploit leading to an information leak. As >> > well as similar hypothetical attacks. >> > >> > Leaks of information from files and symlinks ending in common multimedia extensions >> > are still possible. But files with sensitive information like private keys and passwords >> > generally do not use common multimedia filename extensions. >> > >> > The existing exploit depends on a specific decoder as well. >> > It does appear though that the exploit should be possible with any decoder. >> > The problem is that as long as sensitive information gets into the decoder, >> > the output of the decoder becomes sensitive as well. >> > The only obvious solution is to prevent access to sensitive information. Or to >> > disable hls or possibly some of its feature. More complex solutions like >> > checking the path to limit access to only subdirectories of the hls path may >> > work as an alternative. But such solutions are fragile and tricky to implement >> > portably and would not stop every possible attack nor would they work with all >> > valid hls files. >> > >> > Developers have expressed their dislike / objected to disabling hls by default as well >> > as disabling hls with local files. This here is a less robust but also lower >> > inconvenience solution. >> > It can be applied stand alone or together with other solutions. >> > >> >> One of the most important things is that at the very least HLS keeps >> working out of the box without magic options for actual HTTP HLS >> streams, ie. their primary domain. >> One aspect of this is that HLS streams hosted by CDNs often don't have >> an actual extension, since they get generated by various dynamic URLs, >> and this would break that, so its not a good idea. > > please provide an example that fails with the patch > > I couldn't find a public HLS stream right away that uses no extensions, but I do know that they exist and its easy to craft one just by renaming the segments and editing the playlist - I had issues with this in an Android app I was working on last year. Here is another one that fails with this patch: Query Parameters after the filename: http://daserste_live-lh.akamaihd.net/i/daserste_de@91204/master.m3u8 Another side-effect, it seems to get stuck and never finish opening this file with the blacklist in place (well, at least not for a couple minutes, "never" is more time then I have). I also found some other cases of different extensions being used, ie. some streaming servers for example seem to use "m4s" for fragmented MP4 in HLS. The key point here is, extensions are rather meaningless, we don't use them for probing files if we can avoid it, but we do use them now to exclude files? - Hendrik
On Sat, Jun 03, 2017 at 11:18:46AM +0200, Hendrik Leppkes wrote: > On Sat, Jun 3, 2017 at 2:31 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Fri, Jun 02, 2017 at 09:27:16PM +0200, Hendrik Leppkes wrote: > >> On Fri, Jun 2, 2017 at 9:19 PM, Michael Niedermayer > >> <michael@niedermayer.cc> wrote: > >> > This reduces the attack surface of local file-system and local network > >> > information leaking. > >> > > >> > It prevents the existing exploit leading to an information leak. As > >> > well as similar hypothetical attacks. > >> > > >> > Leaks of information from files and symlinks ending in common multimedia extensions > >> > are still possible. But files with sensitive information like private keys and passwords > >> > generally do not use common multimedia filename extensions. > >> > > >> > The existing exploit depends on a specific decoder as well. > >> > It does appear though that the exploit should be possible with any decoder. > >> > The problem is that as long as sensitive information gets into the decoder, > >> > the output of the decoder becomes sensitive as well. > >> > The only obvious solution is to prevent access to sensitive information. Or to > >> > disable hls or possibly some of its feature. More complex solutions like > >> > checking the path to limit access to only subdirectories of the hls path may > >> > work as an alternative. But such solutions are fragile and tricky to implement > >> > portably and would not stop every possible attack nor would they work with all > >> > valid hls files. > >> > > >> > Developers have expressed their dislike / objected to disabling hls by default as well > >> > as disabling hls with local files. This here is a less robust but also lower > >> > inconvenience solution. > >> > It can be applied stand alone or together with other solutions. > >> > > >> > >> One of the most important things is that at the very least HLS keeps > >> working out of the box without magic options for actual HTTP HLS > >> streams, ie. their primary domain. > >> One aspect of this is that HLS streams hosted by CDNs often don't have > >> an actual extension, since they get generated by various dynamic URLs, > >> and this would break that, so its not a good idea. > > > > please provide an example that fails with the patch > > > > > > I couldn't find a public HLS stream right away that uses no > extensions, but I do know that they exist and its easy to craft one > just by renaming the segments and editing the playlist - I had issues > with this in an Android app I was working on last year. Do you object to fixing this security issue that has a working exploit? Can you provide a testcase the fix breaks ? So i can look into what can be done about it ? (multiple testcases would be even better so theres a lower chance of breaking things) > > Here is another one that fails with this patch: > > Query Parameters after the filename: > http://daserste_live-lh.akamaihd.net/i/daserste_de@91204/master.m3u8 ill post a patch that fixes this > > Another side-effect, it seems to get stuck and never finish opening > this file with the blacklist in place (well, at least not for a couple > minutes, "never" is more time then I have). Tried it, here it immedeatly stops and exits with [hls,applehttp @ 0x7f7388000980] Format not on whitelist '-hls,ALL' http://daserste_live-lh.akamaihd.net/i/daserste_de@91204/master.m3u8: Invalid argument > > I also found some other cases of different extensions being used, ie. > some streaming servers for example seem to use "m4s" for fragmented > MP4 in HLS. extension will be in the next revission of the patch > The key point here is, extensions are rather meaningless, we don't use > them for probing files if we can avoid it, but we do use them now to > exclude files? We would use them to limit what kind of information can be accessed and leaked by default. Every fix for this vulnerability will cause some (at least hypothetical) case to break. one can put videos in a subdirectory called .ssh with the names of the standard files in there. And any fix that protects your private ssh key will break that case. Some fixes disable more by default some disable less. Ive written 3 fixes for this, and in each thread i asked what people prefer and asked for people to submit an alternative fix (iam doing so here too Do you prefer to disable hls with local files as in the first patch? Do you prefer to disable hls via whitelist as in the 2nd patch? Do you prefer to check filenames to be multimedia files as in this patch ? Do you want to submit an alternative fix ? Thanks [...]
On Sat, 3 Jun 2017 14:58:19 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Do you object to fixing this security issue that has a working exploit? > Can you provide a testcase the fix breaks ? So i can look into what > can be done about it ? (multiple testcases would be even better so > theres a lower chance of breaking things) I haven't seen any proof that this is a real security issue yet. It all seems to be based on a lot of assumptions, and always seems to involve social engineering - basically making stupid people to stupid things.
On Sat, Jun 3, 2017 at 2:58 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Jun 03, 2017 at 11:18:46AM +0200, Hendrik Leppkes wrote: >> On Sat, Jun 3, 2017 at 2:31 AM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> > On Fri, Jun 02, 2017 at 09:27:16PM +0200, Hendrik Leppkes wrote: >> >> On Fri, Jun 2, 2017 at 9:19 PM, Michael Niedermayer >> >> <michael@niedermayer.cc> wrote: >> >> > This reduces the attack surface of local file-system and local network >> >> > information leaking. >> >> > >> >> > It prevents the existing exploit leading to an information leak. As >> >> > well as similar hypothetical attacks. >> >> > >> >> > Leaks of information from files and symlinks ending in common multimedia extensions >> >> > are still possible. But files with sensitive information like private keys and passwords >> >> > generally do not use common multimedia filename extensions. >> >> > >> >> > The existing exploit depends on a specific decoder as well. >> >> > It does appear though that the exploit should be possible with any decoder. >> >> > The problem is that as long as sensitive information gets into the decoder, >> >> > the output of the decoder becomes sensitive as well. >> >> > The only obvious solution is to prevent access to sensitive information. Or to >> >> > disable hls or possibly some of its feature. More complex solutions like >> >> > checking the path to limit access to only subdirectories of the hls path may >> >> > work as an alternative. But such solutions are fragile and tricky to implement >> >> > portably and would not stop every possible attack nor would they work with all >> >> > valid hls files. >> >> > >> >> > Developers have expressed their dislike / objected to disabling hls by default as well >> >> > as disabling hls with local files. This here is a less robust but also lower >> >> > inconvenience solution. >> >> > It can be applied stand alone or together with other solutions. >> >> > >> >> >> >> One of the most important things is that at the very least HLS keeps >> >> working out of the box without magic options for actual HTTP HLS >> >> streams, ie. their primary domain. >> >> One aspect of this is that HLS streams hosted by CDNs often don't have >> >> an actual extension, since they get generated by various dynamic URLs, >> >> and this would break that, so its not a good idea. >> > >> > please provide an example that fails with the patch >> > >> > >> >> I couldn't find a public HLS stream right away that uses no >> extensions, but I do know that they exist and its easy to craft one >> just by renaming the segments and editing the playlist - I had issues >> with this in an Android app I was working on last year. > > Do you object to fixing this security issue that has a working exploit? > Can you provide a testcase the fix breaks ? So i can look into what > can be done about it ? (multiple testcases would be even better so > theres a lower chance of breaking things) > > I object to breaking a functioning protocol in the name of some obscure social-engineering attack. - Hendrik
Le quintidi 15 prairial, an CCXXV, Hendrik Leppkes a écrit : > I object to breaking a functioning protocol in the name of some > obscure social-engineering attack. I agree, this issue is negligible. As was the issue about the concat protocol. But we obviously have many similar issues all over the place, and some of them are probably worth worrying. We need to start thinking NOW about a global solution to track the origin of data and prevent leakage. Maybe something similar to Perl's taint check, or to Windows's security zones (I know nothing about them except something like that exist), or toweb browsers anti-cross-site scripting mechanisms. And that was WE, not I. I am not competent to do it alone. Regards,
diff --git a/libavformat/hls.c b/libavformat/hls.c index 4b8fb19a52..74bd87aebc 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -204,6 +204,7 @@ typedef struct HLSContext { char *http_proxy; ///< holds the address of the HTTP proxy server AVDictionary *avio_opts; int strict_std_compliance; + char *allowed_extensions; } HLSContext; static int read_chomp_line(AVIOContext *s, char *buf, int maxlen) @@ -602,6 +603,8 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, AVDictionary *tmp = NULL; const char *proto_name = NULL; int ret; + char filename_buffer[1024]; + const char *filename; av_dict_copy(&tmp, opts, 0); av_dict_copy(&tmp, opts2, 0); @@ -618,8 +621,26 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, return AVERROR_INVALIDDATA; // only http(s) & file are allowed - if (!av_strstart(proto_name, "http", NULL) && !av_strstart(proto_name, "file", NULL)) + if (av_strstart(proto_name, "file", NULL)) { + filename = url; + } else if (av_strstart(proto_name, "http", NULL)) { + filename = filename_buffer; + av_url_split(NULL, 0, NULL, 0, NULL, 0,NULL, + filename_buffer, sizeof(filename_buffer), + url); + if (strlen(filename_buffer) + 1 >= sizeof(filename_buffer)) + return AVERROR_INVALIDDATA; + } else + return AVERROR_INVALIDDATA; + + if (strcmp(c->allowed_extensions, "ALL") && !av_match_ext(filename, c->allowed_extensions)) { + av_log(s, AV_LOG_ERROR, + "Filename extension of \'%s\' is not a common multimedia extension, blocked for security reasons.\n" + "If you wish to override this adjust allowed_extensions, you can set it to \'ALL\' to allow all\n", + filename); return AVERROR_INVALIDDATA; + } + if (!strncmp(proto_name, url, strlen(proto_name)) && url[strlen(proto_name)] == ':') ; else if (av_strstart(url, "crypto", NULL) && !strncmp(proto_name, url + 7, strlen(proto_name)) && url[7 + strlen(proto_name)] == ':') @@ -2134,6 +2155,10 @@ static int hls_probe(AVProbeData *p) static const AVOption hls_options[] = { {"live_start_index", "segment index to start live streams at (negative values are from the end)", OFFSET(live_start_index), AV_OPT_TYPE_INT, {.i64 = -3}, INT_MIN, INT_MAX, FLAGS}, + {"allowed_extensions", "List of file extensions that hls is allowed to access", + OFFSET(allowed_extensions), AV_OPT_TYPE_STRING, + {.str = "3gp,aac,avi,flac,mkv,m3u8,m4a,m4v,mpg,mov,mp2,mp3,mp4,mpeg,mpegts,ogg,ogv,oga,ts,vob,wav"}, + INT_MIN, INT_MAX, FLAGS}, {NULL} };
This reduces the attack surface of local file-system and local network information leaking. It prevents the existing exploit leading to an information leak. As well as similar hypothetical attacks. Leaks of information from files and symlinks ending in common multimedia extensions are still possible. But files with sensitive information like private keys and passwords generally do not use common multimedia filename extensions. The existing exploit depends on a specific decoder as well. It does appear though that the exploit should be possible with any decoder. The problem is that as long as sensitive information gets into the decoder, the output of the decoder becomes sensitive as well. The only obvious solution is to prevent access to sensitive information. Or to disable hls or possibly some of its feature. More complex solutions like checking the path to limit access to only subdirectories of the hls path may work as an alternative. But such solutions are fragile and tricky to implement portably and would not stop every possible attack nor would they work with all valid hls files. Developers have expressed their dislike / objected to disabling hls by default as well as disabling hls with local files. This here is a less robust but also lower inconvenience solution. It can be applied stand alone or together with other solutions. Found-by: Emil Lerner and Pavel Cheremushkin Reported-by: Thierry Foucu <tfoucu@google.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/hls.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)