Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: Check whitelist

Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: Check whitelist

[Michael Niedermayer]

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

On Thu, Jan 16, 2025 at 04:39:49AM +0100, Michael Niedermayer wrote:
> Fixes: CVE-2023-6602, V. DASH Playlist SSRF
> 
> Found-by: Harvey Phillips of Amazon Element55 (element55)
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/dashdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

will apply patch 1, ill wait a bit more for the others in case someone
wants to test

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable

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

Re: [FFmpeg-devel] [PATCH 3/3] avformat/hls: Be more picky on extensions

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 3727 bytes --]

On 1/16/2025 12:39 AM, Michael Niedermayer wrote:
> This blocks disallowed extensions from probing
> It also requires segments to have matching extensions to the format
> 
> It is recommended to set the whitelists correctly
> instead of depending on extensions, but this should help a bit,
> and this is easier to backport
> 
> Fixes: CVE-2023-6602 II. HLS Force TTY Demuxer
> Fixes: CVE-2023-6602 IV. HLS XBIN Demuxer DoS Amplification
> 
> The other parts of CVE-2023-6602 have been fixed by prior commits
> 
> Found-by: Harvey Phillips of Amazon Element55 (element55)
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/hls.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 045741c3b4e..a802eafc3fe 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -223,6 +223,7 @@ typedef struct HLSContext {
>       AVDictionary *avio_opts;
>       AVDictionary *seg_format_opts;
>       char *allowed_extensions;
> +    int extension_picky;
>       int max_reload;
>       int http_persistent;
>       int http_multiple;
> @@ -2114,6 +2115,24 @@ static int hls_read_header(AVFormatContext *s)
>               pls->ctx->interrupt_callback = s->interrupt_callback;
>               url = av_strdup(pls->segments[0]->url);
>               ret = av_probe_input_buffer(&pls->pb.pub, &in_fmt, url, NULL, 0, 0);
> +            if (c->extension_picky && ret >= 0) {

This should be a check for s->strict_std_compliance instead of a new 
demuxer specific option, IMO.
Since you want the strict behavior enabled by default, make this line be:

s->strict_std_compliance >= FF_COMPLIANCE_NORMAL && ret >= 0

> +                for (int n = 0; n < pls->n_segments; n++) {
> +                    struct segment *seg = pls->segments[n];
> +                    if ( strcmp(c->allowed_extensions, "ALL") &&
> +                        !av_match_ext    (seg->url, c->allowed_extensions) &&
> +                        !ff_match_url_ext(seg->url, c->allowed_extensions)) {
> +                        av_log(s, AV_LOG_ERROR, "URL %s is not in allowed_extensions\n", seg->url);
> +                        ret = AVERROR_INVALIDDATA;
> +                    }
> +
> +                    if (!in_fmt->extensions ||
> +                        !av_match_ext(    seg->url, in_fmt->extensions) &&
> +                        !ff_match_url_ext(seg->url, in_fmt->extensions)) {
> +                        av_log(s, AV_LOG_ERROR, "detected format extension %s mismatches url %s\n", in_fmt->extensions ? in_fmt->extensions : "none", seg->url);
> +                        ret = AVERROR_INVALIDDATA;
> +                    }
> +                }
> +            }
>               if (ret < 0) {
>                   /* Free the ctx - it isn't initialized properly at this point,
>                   * so avformat_close_input shouldn't be called. If
> @@ -2576,6 +2595,8 @@ static const AVOption hls_options[] = {
>           OFFSET(allowed_extensions), AV_OPT_TYPE_STRING,
>           {.str = "3gp,aac,avi,ac3,eac3,flac,mkv,m3u8,m4a,m4s,m4v,mpg,mov,mp2,mp3,mp4,mpeg,mpegts,ogg,ogv,oga,ts,vob,wav"},
>           INT_MIN, INT_MAX, FLAGS},
> +    {"extension_picky", "Be picky with all extensions matching",
> +        OFFSET(extension_picky), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, FLAGS},
>       {"max_reload", "Maximum number of times a insufficient list is attempted to be reloaded",
>           OFFSET(max_reload), AV_OPT_TYPE_INT, {.i64 = 3}, 0, INT_MAX, FLAGS},
>       {"m3u8_hold_counters", "The maximum number of times to load m3u8 when it refreshes without new segments",


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: Check whitelist

[Steven Liu]

Michael Niedermayer <michael@niedermayer.cc> 于2025年1月18日周六 06:31写道：
>
> On Thu, Jan 16, 2025 at 04:39:49AM +0100, Michael Niedermayer wrote:
> > Fixes: CVE-2023-6602, V. DASH Playlist SSRF
> >
> > Found-by: Harvey Phillips of Amazon Element55 (element55)
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/dashdec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> will apply patch 1, ill wait a bit more for the others in case someone
> wants to test
Hi michael,

No problem, lgtm




Thanks
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/3] avformat/hls: Be more picky on extensions

[Michael Niedermayer]

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

Hi James

On Fri, Jan 17, 2025 at 07:37:16PM -0300, James Almer wrote:
> On 1/16/2025 12:39 AM, Michael Niedermayer wrote:
> > This blocks disallowed extensions from probing
> > It also requires segments to have matching extensions to the format
> > 
> > It is recommended to set the whitelists correctly
> > instead of depending on extensions, but this should help a bit,
> > and this is easier to backport
> > 
> > Fixes: CVE-2023-6602 II. HLS Force TTY Demuxer
> > Fixes: CVE-2023-6602 IV. HLS XBIN Demuxer DoS Amplification
> > 
> > The other parts of CVE-2023-6602 have been fixed by prior commits
> > 
> > Found-by: Harvey Phillips of Amazon Element55 (element55)
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/hls.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> > 
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index 045741c3b4e..a802eafc3fe 100644
> > --- a/libavformat/hls.c
> > +++ b/libavformat/hls.c
> > @@ -223,6 +223,7 @@ typedef struct HLSContext {
> >       AVDictionary *avio_opts;
> >       AVDictionary *seg_format_opts;
> >       char *allowed_extensions;
> > +    int extension_picky;
> >       int max_reload;
> >       int http_persistent;
> >       int http_multiple;
> > @@ -2114,6 +2115,24 @@ static int hls_read_header(AVFormatContext *s)
> >               pls->ctx->interrupt_callback = s->interrupt_callback;
> >               url = av_strdup(pls->segments[0]->url);
> >               ret = av_probe_input_buffer(&pls->pb.pub, &in_fmt, url, NULL, 0, 0);
> > +            if (c->extension_picky && ret >= 0) {
> 
> This should be a check for s->strict_std_compliance instead of a new demuxer
> specific option, IMO.
> Since you want the strict behavior enabled by default, make this line be:
> 
> s->strict_std_compliance >= FF_COMPLIANCE_NORMAL && ret >= 0

I have not attempted to implement any standard compliance

This is an implementation of security checks to avoid the quoted issues

and even if, by chance, my implementation happens to be what a standard
demands.
overloading strict_std_compliance with a security feature is not correct

The question "do i want "bitstream level" standard compliance"?
and
The question "am i sure the input is fully trusted so i can turn of
a security feature" / do i understand what i do and have i taken precautions
like setting up whitelists

target different users and different use cases

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

Re: [FFmpeg-devel] [PATCH 2/3] avformat/mpegts: Add standard extension so hls can check in extension_picky mode

[Michael Niedermayer]

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

On Thu, Jan 16, 2025 at 04:39:50AM +0100, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mpegts.c | 1 +
>  1 file changed, 1 insertion(+)

will apply

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"

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