[FFmpeg-devel] [PATCH 1/3] avformat/dashdec: fail on probing non mpd file extension

[FFmpeg-devel] [PATCH 1/3] avformat/dashdec: fail on probing non mpd file extension

[Michael Niedermayer]

Its unexpected that a .avi or other "standard" file turns into a playlist.
The goal of this patch is to avoid this unexpected behavior and possible
privacy or security differences.

This is similar to the same change to hls

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/dashdec.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 29d4680c68..294e14150d 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
         av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
         av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
         av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
-        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
-        return AVPROBE_SCORE_MAX;
-    }
-    if (av_stristr(p->buf, "dash:profile")) {
+        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
+        av_stristr(p->buf, "dash:profile")) {
+        if (!av_match_ext(p->filename, "mpd")) {
+            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non standard extension\n");
+            return 0;
+        }
+
         return AVPROBE_SCORE_MAX;
     }
 
-- 
2.17.1

_______________________________________________
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] [PATCH 2/3] avformat/imfdec: fail on probing non xml file extension

[Michael Niedermayer]

Its unexpected that a .avi or other "standard" file turns into a playlist.
The goal of this patch is to avoid this unexpected behavior and possible
privacy or security differences.

This is similar to the same change to hls

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/imfdec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
index 7d04d0d853..eafe9a6a26 100644
--- a/libavformat/imfdec.c
+++ b/libavformat/imfdec.c
@@ -926,6 +926,11 @@ static int imf_probe(const AVProbeData *p)
     if (!strstr(p->buf, "ContentTitle>"))
         return 0;
 
+    if (!av_match_ext(p->filename, "xml")) {
+        av_log(NULL, AV_LOG_ERROR, "Not detecting imf with non standard extension\n");
+        return 0;
+    }
+
     return AVPROBE_SCORE_MAX;
 }
 
-- 
2.17.1

_______________________________________________
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] [PATCH 3/3] avformat/mpeg: Fix filename extension check for subtitle file

[Michael Niedermayer]

This fixes undefined behavior and other issues.
No testcase, this was found by code review

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mpeg.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/libavformat/mpeg.c b/libavformat/mpeg.c
index 781c3162d6..fdc808dc50 100644
--- a/libavformat/mpeg.c
+++ b/libavformat/mpeg.c
@@ -756,12 +756,17 @@ static int vobsub_read_header(AVFormatContext *s)
         }
 
         fname_len = strlen(vobsub->sub_name);
-        ext = vobsub->sub_name - 3 + fname_len;
-        if (fname_len < 4 || *(ext - 1) != '.') {
+        if (fname_len < 4) {
             av_log(s, AV_LOG_ERROR, "The input index filename is too short "
                    "to guess the associated .SUB file\n");
             return AVERROR_INVALIDDATA;
         }
+        ext = vobsub->sub_name + fname_len - 3;
+        if (*(ext - 1) != '.' || strchr(ext, '/')) {
+            av_log(s, AV_LOG_ERROR, "The input index filename needs to have a 3 letter extension "
+                   "to guess the associated .SUB file\n");
+            return AVERROR_INVALIDDATA;
+        }
         memcpy(ext, !strncmp(ext, "IDX", 3) ? "SUB" : "sub", 3);
         av_log(s, AV_LOG_VERBOSE, "IDX/SUB: %s -> %s\n", s->url, vobsub->sub_name);
     }
-- 
2.17.1

_______________________________________________
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/imfdec: fail on probing non xml file extension

[Pierre-Anthony Lemieux]

On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> Its unexpected that a .avi or other "standard" file turns into a playlist.
> The goal of this patch is to avoid this unexpected behavior and possible
> privacy or security differences.

Per the IMF specification, a CPL can have any extension or, in fact,
no extension. The latter is routinely used.

>
> This is similar to the same change to hls
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/imfdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> index 7d04d0d853..eafe9a6a26 100644
> --- a/libavformat/imfdec.c
> +++ b/libavformat/imfdec.c
> @@ -926,6 +926,11 @@ static int imf_probe(const AVProbeData *p)
>      if (!strstr(p->buf, "ContentTitle>"))
>          return 0;
>
> +    if (!av_match_ext(p->filename, "xml")) {
> +        av_log(NULL, AV_LOG_ERROR, "Not detecting imf with non standard extension\n");
> +        return 0;
> +    }
> +
>      return AVPROBE_SCORE_MAX;
>  }
>
> --
> 2.17.1
>
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 2/3] avformat/imfdec: fail on probing non xml file extension

[Michael Niedermayer]

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

On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > The goal of this patch is to avoid this unexpected behavior and possible
> > privacy or security differences.
> 
> Per the IMF specification, a CPL can have any extension or, in fact,
> no extension. The latter is routinely used.

is there a restriction on the URL/URIs used in it ?
that is in practice, can they be restricted to the same server, 
child directories, or some other restriction ?

thx

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway

[-- 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 1/3] avformat/dashdec: fail on probing non mpd file extension

[Anton Khirnov]

Quoting Michael Niedermayer (2023-05-06 15:25:01)
> Its unexpected that a .avi or other "standard" file turns into a playlist.
> The goal of this patch is to avoid this unexpected behavior and possible
> privacy or security differences.
> 
> This is similar to the same change to hls

I heavily dislike this.

-- 
Anton Khirnov
_______________________________________________
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/imfdec: fail on probing non xml file extension

[Pierre-Anthony Lemieux]

On Sun, May 7, 2023 at 12:18 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> > On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > > The goal of this patch is to avoid this unexpected behavior and possible
> > > privacy or security differences.
> >
> > Per the IMF specification, a CPL can have any extension or, in fact,
> > no extension. The latter is routinely used.
>
> is there a restriction on the URL/URIs used in it ?
> that is in practice, can they be restricted to the same server,
> child directories, or some other restriction ?

Below is a brief overview of the linkage between the various of
components of an IMF composition:

- the Composition Playlist (CPL) is the file that is passed to FFMPEG
as input (-i)
- the CPL is an XML document and defines a playlist
- each of the components that make up the playlist is identified by a
UUID, i.e. the CPL does not contain file paths/URLs.
- the mapping between UUIDs and URLs is done through separate XML
files called Asset Maps. Paths to Asset Maps can be provided
explicitly through the "-assetmaps" argument, otherwise FFMPEG looks
for a file called "ASSETMAP.xml" in the same directory as the CPL
file.
- according to the standard, all URLs in each Asset Map is relative to
the location of the Asset Map, and thus the CPL and the Asset Map have
the same origin
- some applications have relaxed this constraint and allowed absolute
URLs in the Asset Map

What is the threat scenario? Is the concern that a malicious actor
provides a CPL and Asset Map from origin A that makes malicious
requests to a different origin B?

>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Breaking DRM is a little like attempting to break through a door even
> though the window is wide open and the only thing in the house is a bunch
> of things you dont want and which you would get tomorrow for free anyway
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: fail on probing non mpd file extension

[James Almer]

On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> Its unexpected that a .avi or other "standard" file turns into a playlist.
> The goal of this patch is to avoid this unexpected behavior and possible
> privacy or security differences.
> 
> This is similar to the same change to hls
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/dashdec.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 29d4680c68..294e14150d 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
>           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
>           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
>           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> -        return AVPROBE_SCORE_MAX;
> -    }
> -    if (av_stristr(p->buf, "dash:profile")) {
> +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> +        av_stristr(p->buf, "dash:profile")) {
> +        if (!av_match_ext(p->filename, "mpd")) {
> +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non standard extension\n");
> +            return 0;
> +        }
> +
>           return AVPROBE_SCORE_MAX;
>       }

Failing because it didn't match an extensions sort of goes against the 
point of probing, which even has a low score return value that's 
basically "it matched extension" as a sort of last resort.

I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the 
spec does state mpd must be the extension), but i think we have no 
access to the AVFormatContext here?
_______________________________________________
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: fail on probing non mpd file extension

[Tobias Rapp]

On 08/05/2023 14:00, James Almer wrote:

> On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
>> Its unexpected that a .avi or other "standard" file turns into a 
>> playlist.
>> The goal of this patch is to avoid this unexpected behavior and possible
>> privacy or security differences.
>>
>> This is similar to the same change to hls
>>
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>   libavformat/dashdec.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index 29d4680c68..294e14150d 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
>>           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
>>           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
>>           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
>> -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
>> -        return AVPROBE_SCORE_MAX;
>> -    }
>> -    if (av_stristr(p->buf, "dash:profile")) {
>> +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
>> +        av_stristr(p->buf, "dash:profile")) {
>> +        if (!av_match_ext(p->filename, "mpd")) {
>> +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non 
>> standard extension\n");
>> +            return 0;
>> +        }
>> +
>>           return AVPROBE_SCORE_MAX;
>>       }
>
> Failing because it didn't match an extensions sort of goes against the 
> point of probing, which even has a low score return value that's 
> basically "it matched extension" as a sort of last resort.
>
> I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the 
> spec does state mpd must be the extension), but i think we have no 
> access to the AVFormatContext here?

DASH is usually transferred over HTTP where file extensions are of minor 
interest, the relevant type information is in the Mime-Type header.

I think we already have the "format_whitelist" API for applications that 
want to restrict the list of formats when loading a file from untrusted 
sources?

Regards, Tobias

_______________________________________________
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: fail on probing non mpd file extension

[Pierre-Anthony Lemieux]

On Mon, May 8, 2023 at 7:05 AM Tobias Rapp <t.rapp@noa-archive.com> wrote:
>
> On 08/05/2023 14:00, James Almer wrote:
>
> > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> >> Its unexpected that a .avi or other "standard" file turns into a
> >> playlist.
> >> The goal of this patch is to avoid this unexpected behavior and possible
> >> privacy or security differences.
> >>
> >> This is similar to the same change to hls
> >>
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>   libavformat/dashdec.c | 11 +++++++----
> >>   1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> >> index 29d4680c68..294e14150d 100644
> >> --- a/libavformat/dashdec.c
> >> +++ b/libavformat/dashdec.c
> >> @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> >>           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> >>           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> >>           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> >> -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> >> -        return AVPROBE_SCORE_MAX;
> >> -    }
> >> -    if (av_stristr(p->buf, "dash:profile")) {
> >> +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> >> +        av_stristr(p->buf, "dash:profile")) {
> >> +        if (!av_match_ext(p->filename, "mpd")) {
> >> +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> >> standard extension\n");
> >> +            return 0;
> >> +        }
> >> +
> >>           return AVPROBE_SCORE_MAX;
> >>       }
> >
> > Failing because it didn't match an extensions sort of goes against the
> > point of probing, which even has a low score return value that's
> > basically "it matched extension" as a sort of last resort.
> >
> > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > spec does state mpd must be the extension), but i think we have no
> > access to the AVFormatContext here?
>
> DASH is usually transferred over HTTP where file extensions are of minor
> interest, the relevant type information is in the Mime-Type header.
>
> I think we already have the "format_whitelist" API for applications that
> want to restrict the list of formats when loading a file from untrusted
> sources?

Yes, the IMF playlist, for example, is only allowed to reference MXF files:

https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/imfdec.c#L393

>
> Regards, Tobias
>
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: fail on probing non mpd file extension

[Michael Niedermayer]

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

On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> On 08/05/2023 14:00, James Almer wrote:
> 
> > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> > > Its unexpected that a .avi or other "standard" file turns into a
> > > playlist.
> > > The goal of this patch is to avoid this unexpected behavior and possible
> > > privacy or security differences.
> > > 
> > > This is similar to the same change to hls
> > > 
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >   libavformat/dashdec.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > > index 29d4680c68..294e14150d 100644
> > > --- a/libavformat/dashdec.c
> > > +++ b/libavformat/dashdec.c
> > > @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> > >           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> > >           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> > >           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> > > -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> > > -        return AVPROBE_SCORE_MAX;
> > > -    }
> > > -    if (av_stristr(p->buf, "dash:profile")) {
> > > +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> > > +        av_stristr(p->buf, "dash:profile")) {
> > > +        if (!av_match_ext(p->filename, "mpd")) {
> > > +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> > > standard extension\n");
> > > +            return 0;
> > > +        }
> > > +
> > >           return AVPROBE_SCORE_MAX;
> > >       }
> > 
> > Failing because it didn't match an extensions sort of goes against the
> > point of probing, which even has a low score return value that's
> > basically "it matched extension" as a sort of last resort.

True


> > 
> > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > spec does state mpd must be the extension), but i think we have no
> > access to the AVFormatContext here?

Thats not what this was intended to do.

The whole idea is more like a user clicking on a readme.txt and not
expecting that to downloade a list of URLs in it because it happens to be a
valid list or URLs

The problem here is the information available to the user suggests one thing
but the action of the user of opening this file does something different, something
unexpected

Thats not an issue if the difference is between 2 of 1000 similar formats
but If the user believes the format cannot open random local and remote 
URLs but is just a single monolithic file and then when she clicks it
does open other things without the user even ever knowing. That is not
ideal.


> 
> DASH is usually transferred over HTTP where file extensions are of minor
> interest, the relevant type information is in the Mime-Type header.

yes, true


> 
> I think we already have the "format_whitelist" API for applications that
> want to restrict the list of formats when loading a file from untrusted
> sources?



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

It is what and why we do it that matters, not just one of them.

[-- 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 1/3] avformat/dashdec: fail on probing non mpd file extension

[Pierre-Anthony Lemieux]

On Mon, May 8, 2023 at 10:11 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> > On 08/05/2023 14:00, James Almer wrote:
> >
> > > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> > > > Its unexpected that a .avi or other "standard" file turns into a
> > > > playlist.
> > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > privacy or security differences.
> > > >
> > > > This is similar to the same change to hls
> > > >
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >   libavformat/dashdec.c | 11 +++++++----
> > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > > > index 29d4680c68..294e14150d 100644
> > > > --- a/libavformat/dashdec.c
> > > > +++ b/libavformat/dashdec.c
> > > > @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> > > >           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> > > >           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> > > >           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> > > > -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> > > > -        return AVPROBE_SCORE_MAX;
> > > > -    }
> > > > -    if (av_stristr(p->buf, "dash:profile")) {
> > > > +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> > > > +        av_stristr(p->buf, "dash:profile")) {
> > > > +        if (!av_match_ext(p->filename, "mpd")) {
> > > > +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> > > > standard extension\n");
> > > > +            return 0;
> > > > +        }
> > > > +
> > > >           return AVPROBE_SCORE_MAX;
> > > >       }
> > >
> > > Failing because it didn't match an extensions sort of goes against the
> > > point of probing, which even has a low score return value that's
> > > basically "it matched extension" as a sort of last resort.
>
> True
>
>
> > >
> > > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > > spec does state mpd must be the extension), but i think we have no
> > > access to the AVFormatContext here?
>
> Thats not what this was intended to do.
>
> The whole idea is more like a user clicking on a readme.txt and not
> expecting that to downloade a list of URLs in it because it happens to be a
> valid list or URLs

That presumes that "readme.txt":

(a) is detected as a playlist
(b) is a valid XML file (in the case of IMF or dash playlists)
(c) is structured to be interpreted as a playlist by the demuxer

This is unlikely to happen with a random "readme.txt". It sounds like
we are specifically trying to protect against a maliciously-crafted
"readme.txt".

If so, could enforcing, by default, same origin mitigate risks?


>
> The problem here is the information available to the user suggests one thing
> but the action of the user of opening this file does something different, something
> unexpected
>
> Thats not an issue if the difference is between 2 of 1000 similar formats
> but If the user believes the format cannot open random local and remote
> URLs but is just a single monolithic file and then when she clicks it
> does open other things without the user even ever knowing. That is not
> ideal.
>
>
> >
> > DASH is usually transferred over HTTP where file extensions are of minor
> > interest, the relevant type information is in the Mime-Type header.
>
> yes, true
>
>
> >
> > I think we already have the "format_whitelist" API for applications that
> > want to restrict the list of formats when loading a file from untrusted
> > sources?
>
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is what and why we do it that matters, not just one of them.
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 2/3] avformat/imfdec: fail on probing non xml file extension

[Michael Niedermayer]

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

On Sun, May 07, 2023 at 10:09:58PM -0700, Pierre-Anthony Lemieux wrote:
> On Sun, May 7, 2023 at 12:18 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> > > On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > privacy or security differences.
> > >
> > > Per the IMF specification, a CPL can have any extension or, in fact,
> > > no extension. The latter is routinely used.
> >
> > is there a restriction on the URL/URIs used in it ?
> > that is in practice, can they be restricted to the same server,
> > child directories, or some other restriction ?
> 
> Below is a brief overview of the linkage between the various of
> components of an IMF composition:
> 
> - the Composition Playlist (CPL) is the file that is passed to FFMPEG
> as input (-i)
> - the CPL is an XML document and defines a playlist
> - each of the components that make up the playlist is identified by a
> UUID, i.e. the CPL does not contain file paths/URLs.
> - the mapping between UUIDs and URLs is done through separate XML
> files called Asset Maps. Paths to Asset Maps can be provided
> explicitly through the "-assetmaps" argument, otherwise FFMPEG looks
> for a file called "ASSETMAP.xml" in the same directory as the CPL
> file.
> - according to the standard, all URLs in each Asset Map is relative to
> the location of the Asset Map, and thus the CPL and the Asset Map have
> the same origin
> - some applications have relaxed this constraint and allowed absolute
> URLs in the Asset Map

Thank you for this information


> 
> What is the threat scenario? Is the concern that a malicious actor
> provides a CPL and Asset Map from origin A that makes malicious
> requests to a different origin B?

I do not have an exhaustive list of what can be done, but ill list a
few things i can think of with some random ideas.

First if i pretend to be the attacker, i want one file not 2 because
thats easier
can i just send the victim a ASSETMAP.xml that parses correctly as
CPL too ?
If yes, i think that can be checked for and trigger an error because
i dont think a valid file would use itself as assetmap
we could go a bit further here and play with things like
ASSETMAP.xml?video.avi
or something like that to make the link look more normal
i didint look at if that would work but it just makes it more harmless looking

now what can one do with this 

A Spying
1. User downloads a video file
now every time she plays the file, the file pings a URL revealing time, frequency and IP of the watched file
This is probably not expected by the user

B1 Poking
1. User downloads or plays a video file
now the file refers to various urls testing the users local network and network services
timing of remote accesses reveals this to an attacker
This is probably not expected by the user either

B2 same as B1 but a attacker uploads the file to a server where the attacker pokes around using it

B3 the URL requests to other services may or may not be able to do more than just reading

C DOS
a attacker uploads a file with many references and lets the server repeatly attempt connections
to them
This one is tricky because we liekly want to continue if one reference fails 
but also not do thousands of odd accesses to anything

This could plausibly even be used to bruteforcing some auth parameters
upload a file with all 4 digit pin codes in their URL and then depending on
what is encoding, maybe what length the resulting encoded file has one could
maybe figure out which URL access succeeded. 

Iam not an expert in this so quite likely theres more that can be done that
iam not thinking of

Thus anything that isnt part of normal use cases, i suggest to not allow by default
(like for example a ASSETMAP.xml thats also a valid CPL file)

thx

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

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."

[-- 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/imfdec: fail on probing non xml file extension

[Pierre-Anthony Lemieux]

On Mon, May 8, 2023 at 11:23 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, May 07, 2023 at 10:09:58PM -0700, Pierre-Anthony Lemieux wrote:
> > On Sun, May 7, 2023 at 12:18 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> > > > On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> > > > <michael@niedermayer.cc> wrote:
> > > > >
> > > > > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > > privacy or security differences.
> > > >
> > > > Per the IMF specification, a CPL can have any extension or, in fact,
> > > > no extension. The latter is routinely used.
> > >
> > > is there a restriction on the URL/URIs used in it ?
> > > that is in practice, can they be restricted to the same server,
> > > child directories, or some other restriction ?
> >
> > Below is a brief overview of the linkage between the various of
> > components of an IMF composition:
> >
> > - the Composition Playlist (CPL) is the file that is passed to FFMPEG
> > as input (-i)
> > - the CPL is an XML document and defines a playlist
> > - each of the components that make up the playlist is identified by a
> > UUID, i.e. the CPL does not contain file paths/URLs.
> > - the mapping between UUIDs and URLs is done through separate XML
> > files called Asset Maps. Paths to Asset Maps can be provided
> > explicitly through the "-assetmaps" argument, otherwise FFMPEG looks
> > for a file called "ASSETMAP.xml" in the same directory as the CPL
> > file.
> > - according to the standard, all URLs in each Asset Map is relative to
> > the location of the Asset Map, and thus the CPL and the Asset Map have
> > the same origin
> > - some applications have relaxed this constraint and allowed absolute
> > URLs in the Asset Map
>
> Thank you for this information
>
>
> >
> > What is the threat scenario? Is the concern that a malicious actor
> > provides a CPL and Asset Map from origin A that makes malicious
> > requests to a different origin B?
>
> I do not have an exhaustive list of what can be done, but ill list a
> few things i can think of with some random ideas.
>
> First if i pretend to be the attacker, i want one file not 2 because
> thats easier
> can i just send the victim a ASSETMAP.xml that parses correctly as
> CPL too ?

Both ASSETMAP.xml and CPL are XML files. The root element of the
former is "AssetMap" and the root element of the latter is
"CompositionPlaylist".
The IMF demuxer fails if this is not true, so an Asset Map document
cannot be mistaken for a CPL, and vice-versa.

> If yes, i think that can be checked for and trigger an error because
> i dont think a valid file would use itself as assetmap
> we could go a bit further here and play with things like
> ASSETMAP.xml?video.avi
> or something like that to make the link look more normal
> i didint look at if that would work but it just makes it more harmless looking
>
> now what can one do with this
>
> A Spying
> 1. User downloads a video file
> now every time she plays the file, the file pings a URL revealing time, frequency and IP of the watched file
> This is probably not expected by the user
>
> B1 Poking
> 1. User downloads or plays a video file
> now the file refers to various urls testing the users local network and network services
> timing of remote accesses reveals this to an attacker
> This is probably not expected by the user either
>
> B2 same as B1 but a attacker uploads the file to a server where the attacker pokes around using it
>
> B3 the URL requests to other services may or may not be able to do more than just reading
>
> C DOS
> a attacker uploads a file with many references and lets the server repeatly attempt connections
> to them
> This one is tricky because we liekly want to continue if one reference fails
> but also not do thousands of odd accesses to anything
>
> This could plausibly even be used to bruteforcing some auth parameters
> upload a file with all 4 digit pin codes in their URL and then depending on
> what is encoding, maybe what length the resulting encoded file has one could
> maybe figure out which URL access succeeded.
>
> Iam not an expert in this so quite likely theres more that can be done that
> iam not thinking of
>
> Thus anything that isnt part of normal use cases, i suggest to not allow by default
> (like for example a ASSETMAP.xml thats also a valid CPL file)

The scenarios above require FFMPEG to access URLs outside of the
origin of the CPL and ASSETMAP. Would implementing a same-origin
policy help?

>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The real ebay dictionary, page 1
> "Used only once"    - "Some unspecified defect prevented a second use"
> "In good condition" - "Can be repaird by experienced expert"
> "As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 2/3] avformat/imfdec: fail on probing non xml file extension

[Michael Niedermayer]

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

On Mon, May 08, 2023 at 11:40:49AM -0700, Pierre-Anthony Lemieux wrote:
> On Mon, May 8, 2023 at 11:23 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sun, May 07, 2023 at 10:09:58PM -0700, Pierre-Anthony Lemieux wrote:
> > > On Sun, May 7, 2023 at 12:18 PM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> > > > > On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> > > > > <michael@niedermayer.cc> wrote:
> > > > > >
> > > > > > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > > > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > > > privacy or security differences.
> > > > >
> > > > > Per the IMF specification, a CPL can have any extension or, in fact,
> > > > > no extension. The latter is routinely used.
> > > >
> > > > is there a restriction on the URL/URIs used in it ?
> > > > that is in practice, can they be restricted to the same server,
> > > > child directories, or some other restriction ?
> > >
> > > Below is a brief overview of the linkage between the various of
> > > components of an IMF composition:
> > >
> > > - the Composition Playlist (CPL) is the file that is passed to FFMPEG
> > > as input (-i)
> > > - the CPL is an XML document and defines a playlist
> > > - each of the components that make up the playlist is identified by a
> > > UUID, i.e. the CPL does not contain file paths/URLs.
> > > - the mapping between UUIDs and URLs is done through separate XML
> > > files called Asset Maps. Paths to Asset Maps can be provided
> > > explicitly through the "-assetmaps" argument, otherwise FFMPEG looks
> > > for a file called "ASSETMAP.xml" in the same directory as the CPL
> > > file.
> > > - according to the standard, all URLs in each Asset Map is relative to
> > > the location of the Asset Map, and thus the CPL and the Asset Map have
> > > the same origin
> > > - some applications have relaxed this constraint and allowed absolute
> > > URLs in the Asset Map
> >
> > Thank you for this information
> >
> >
> > >
> > > What is the threat scenario? Is the concern that a malicious actor
> > > provides a CPL and Asset Map from origin A that makes malicious
> > > requests to a different origin B?
> >
> > I do not have an exhaustive list of what can be done, but ill list a
> > few things i can think of with some random ideas.
> >
> > First if i pretend to be the attacker, i want one file not 2 because
> > thats easier
> > can i just send the victim a ASSETMAP.xml that parses correctly as
> > CPL too ?
> 
> Both ASSETMAP.xml and CPL are XML files. The root element of the
> former is "AssetMap" and the root element of the latter is
> "CompositionPlaylist".
> The IMF demuxer fails if this is not true, so an Asset Map document
> cannot be mistaken for a CPL, and vice-versa.

That is good


> 
> > If yes, i think that can be checked for and trigger an error because
> > i dont think a valid file would use itself as assetmap
> > we could go a bit further here and play with things like
> > ASSETMAP.xml?video.avi
> > or something like that to make the link look more normal
> > i didint look at if that would work but it just makes it more harmless looking
> >
> > now what can one do with this
> >
> > A Spying
> > 1. User downloads a video file
> > now every time she plays the file, the file pings a URL revealing time, frequency and IP of the watched file
> > This is probably not expected by the user
> >
> > B1 Poking
> > 1. User downloads or plays a video file
> > now the file refers to various urls testing the users local network and network services
> > timing of remote accesses reveals this to an attacker
> > This is probably not expected by the user either
> >
> > B2 same as B1 but a attacker uploads the file to a server where the attacker pokes around using it
> >
> > B3 the URL requests to other services may or may not be able to do more than just reading
> >
> > C DOS
> > a attacker uploads a file with many references and lets the server repeatly attempt connections
> > to them
> > This one is tricky because we liekly want to continue if one reference fails
> > but also not do thousands of odd accesses to anything
> >
> > This could plausibly even be used to bruteforcing some auth parameters
> > upload a file with all 4 digit pin codes in their URL and then depending on
> > what is encoding, maybe what length the resulting encoded file has one could
> > maybe figure out which URL access succeeded.
> >
> > Iam not an expert in this so quite likely theres more that can be done that
> > iam not thinking of
> >
> > Thus anything that isnt part of normal use cases, i suggest to not allow by default
> > (like for example a ASSETMAP.xml thats also a valid CPL file)
> 
> The scenarios above require FFMPEG to access URLs outside of the
> origin of the CPL and ASSETMAP. Would implementing a same-origin
> policy help?

yes, i belive this would significantly reduce what an attacker can do
with this

thx

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

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- 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/imfdec: fail on probing non xml file extension

[Pierre-Anthony Lemieux]

On Mon, May 8, 2023 at 3:01 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Mon, May 08, 2023 at 11:40:49AM -0700, Pierre-Anthony Lemieux wrote:
> > On Mon, May 8, 2023 at 11:23 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Sun, May 07, 2023 at 10:09:58PM -0700, Pierre-Anthony Lemieux wrote:
> > > > On Sun, May 7, 2023 at 12:18 PM Michael Niedermayer
> > > > <michael@niedermayer.cc> wrote:
> > > > >
> > > > > On Sat, May 06, 2023 at 11:01:20AM -0700, Pierre-Anthony Lemieux wrote:
> > > > > > On Sat, May 6, 2023 at 6:25 AM Michael Niedermayer
> > > > > > <michael@niedermayer.cc> wrote:
> > > > > > >
> > > > > > > Its unexpected that a .avi or other "standard" file turns into a playlist.
> > > > > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > > > > privacy or security differences.
> > > > > >
> > > > > > Per the IMF specification, a CPL can have any extension or, in fact,
> > > > > > no extension. The latter is routinely used.
> > > > >
> > > > > is there a restriction on the URL/URIs used in it ?
> > > > > that is in practice, can they be restricted to the same server,
> > > > > child directories, or some other restriction ?
> > > >
> > > > Below is a brief overview of the linkage between the various of
> > > > components of an IMF composition:
> > > >
> > > > - the Composition Playlist (CPL) is the file that is passed to FFMPEG
> > > > as input (-i)
> > > > - the CPL is an XML document and defines a playlist
> > > > - each of the components that make up the playlist is identified by a
> > > > UUID, i.e. the CPL does not contain file paths/URLs.
> > > > - the mapping between UUIDs and URLs is done through separate XML
> > > > files called Asset Maps. Paths to Asset Maps can be provided
> > > > explicitly through the "-assetmaps" argument, otherwise FFMPEG looks
> > > > for a file called "ASSETMAP.xml" in the same directory as the CPL
> > > > file.
> > > > - according to the standard, all URLs in each Asset Map is relative to
> > > > the location of the Asset Map, and thus the CPL and the Asset Map have
> > > > the same origin
> > > > - some applications have relaxed this constraint and allowed absolute
> > > > URLs in the Asset Map
> > >
> > > Thank you for this information
> > >
> > >
> > > >
> > > > What is the threat scenario? Is the concern that a malicious actor
> > > > provides a CPL and Asset Map from origin A that makes malicious
> > > > requests to a different origin B?
> > >
> > > I do not have an exhaustive list of what can be done, but ill list a
> > > few things i can think of with some random ideas.
> > >
> > > First if i pretend to be the attacker, i want one file not 2 because
> > > thats easier
> > > can i just send the victim a ASSETMAP.xml that parses correctly as
> > > CPL too ?
> >
> > Both ASSETMAP.xml and CPL are XML files. The root element of the
> > former is "AssetMap" and the root element of the latter is
> > "CompositionPlaylist".
> > The IMF demuxer fails if this is not true, so an Asset Map document
> > cannot be mistaken for a CPL, and vice-versa.
>
> That is good
>
>
> >
> > > If yes, i think that can be checked for and trigger an error because
> > > i dont think a valid file would use itself as assetmap
> > > we could go a bit further here and play with things like
> > > ASSETMAP.xml?video.avi
> > > or something like that to make the link look more normal
> > > i didint look at if that would work but it just makes it more harmless looking
> > >
> > > now what can one do with this
> > >
> > > A Spying
> > > 1. User downloads a video file
> > > now every time she plays the file, the file pings a URL revealing time, frequency and IP of the watched file
> > > This is probably not expected by the user
> > >
> > > B1 Poking
> > > 1. User downloads or plays a video file
> > > now the file refers to various urls testing the users local network and network services
> > > timing of remote accesses reveals this to an attacker
> > > This is probably not expected by the user either
> > >
> > > B2 same as B1 but a attacker uploads the file to a server where the attacker pokes around using it
> > >
> > > B3 the URL requests to other services may or may not be able to do more than just reading
> > >
> > > C DOS
> > > a attacker uploads a file with many references and lets the server repeatly attempt connections
> > > to them
> > > This one is tricky because we liekly want to continue if one reference fails
> > > but also not do thousands of odd accesses to anything
> > >
> > > This could plausibly even be used to bruteforcing some auth parameters
> > > upload a file with all 4 digit pin codes in their URL and then depending on
> > > what is encoding, maybe what length the resulting encoded file has one could
> > > maybe figure out which URL access succeeded.
> > >
> > > Iam not an expert in this so quite likely theres more that can be done that
> > > iam not thinking of
> > >
> > > Thus anything that isnt part of normal use cases, i suggest to not allow by default
> > > (like for example a ASSETMAP.xml thats also a valid CPL file)
> >
> > The scenarios above require FFMPEG to access URLs outside of the
> > origin of the CPL and ASSETMAP. Would implementing a same-origin
> > policy help?
>
> yes, i belive this would significantly reduce what an attacker can do
> with this

Is there a general concept of "same-origin" in FFMPEG?

>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/dashdec: fail on probing non mpd file extension

[Michael Niedermayer]

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

On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> On 08/05/2023 14:00, James Almer wrote:
> 
> > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> > > Its unexpected that a .avi or other "standard" file turns into a
> > > playlist.
> > > The goal of this patch is to avoid this unexpected behavior and possible
> > > privacy or security differences.
> > > 
> > > This is similar to the same change to hls
> > > 
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >   libavformat/dashdec.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > > index 29d4680c68..294e14150d 100644
> > > --- a/libavformat/dashdec.c
> > > +++ b/libavformat/dashdec.c
> > > @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> > >           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> > >           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> > >           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> > > -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> > > -        return AVPROBE_SCORE_MAX;
> > > -    }
> > > -    if (av_stristr(p->buf, "dash:profile")) {
> > > +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> > > +        av_stristr(p->buf, "dash:profile")) {
> > > +        if (!av_match_ext(p->filename, "mpd")) {
> > > +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> > > standard extension\n");
> > > +            return 0;
> > > +        }
> > > +
> > >           return AVPROBE_SCORE_MAX;
> > >       }
> > 
> > Failing because it didn't match an extensions sort of goes against the
> > point of probing, which even has a low score return value that's
> > basically "it matched extension" as a sort of last resort.
> > 
> > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > spec does state mpd must be the extension), but i think we have no
> > access to the AVFormatContext here?
> 
> DASH is usually transferred over HTTP where file extensions are of minor
> interest, the relevant type information is in the Mime-Type header.

would anyone be opposed to return 0 from dash_probe() when
both the mime_type and the extension are wrong ?

Here both the server says its not dash as well as the extension not being the
standard for dash

example: a crafted image.jpeg uploaded somewhere is played as dash.
or am i missing something that would stop that ?

thx

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

Never trust a computer, one day, it may think you are the virus. -- Compn

[-- 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 1/3] avformat/dashdec: fail on probing non mpd file extension

[Anton Khirnov]

Quoting Michael Niedermayer (2023-05-09 00:35:08)
> On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> > On 08/05/2023 14:00, James Almer wrote:
> > 
> > > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> > > > Its unexpected that a .avi or other "standard" file turns into a
> > > > playlist.
> > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > privacy or security differences.
> > > > 
> > > > This is similar to the same change to hls
> > > > 
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >   libavformat/dashdec.c | 11 +++++++----
> > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > > > index 29d4680c68..294e14150d 100644
> > > > --- a/libavformat/dashdec.c
> > > > +++ b/libavformat/dashdec.c
> > > > @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> > > >           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> > > >           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> > > >           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> > > > -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> > > > -        return AVPROBE_SCORE_MAX;
> > > > -    }
> > > > -    if (av_stristr(p->buf, "dash:profile")) {
> > > > +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> > > > +        av_stristr(p->buf, "dash:profile")) {
> > > > +        if (!av_match_ext(p->filename, "mpd")) {
> > > > +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> > > > standard extension\n");
> > > > +            return 0;
> > > > +        }
> > > > +
> > > >           return AVPROBE_SCORE_MAX;
> > > >       }
> > > 
> > > Failing because it didn't match an extensions sort of goes against the
> > > point of probing, which even has a low score return value that's
> > > basically "it matched extension" as a sort of last resort.
> > > 
> > > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > > spec does state mpd must be the extension), but i think we have no
> > > access to the AVFormatContext here?
> > 
> > DASH is usually transferred over HTTP where file extensions are of minor
> > interest, the relevant type information is in the Mime-Type header.
> 
> would anyone be opposed to return 0 from dash_probe() when
> both the mime_type and the extension are wrong ?

I would.

probe() is for probing, not implementing security policies. IMO trying
to fix security issues at the wrong layer will only lead to more
confusion, more complexity, and LESS security.

-- 
Anton Khirnov
_______________________________________________
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: fail on probing non mpd file extension

[Tobias Rapp]

On 09/05/2023 08:19, Anton Khirnov wrote:

> Quoting Michael Niedermayer (2023-05-09 00:35:08)
>> On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
>>> [...]
>>> DASH is usually transferred over HTTP where file extensions are of minor
>>> interest, the relevant type information is in the Mime-Type header.
>> would anyone be opposed to return 0 from dash_probe() when
>> both the mime_type and the extension are wrong ?
> I would.
>
> probe() is for probing, not implementing security policies. IMO trying
> to fix security issues at the wrong layer will only lead to more
> confusion, more complexity, and LESS security.

I agree that probing should be unrelated to the actual format selection 
policy.

>> example: a crafted image.jpeg uploaded somewhere is played as dash.
>> or am i missing something that would stop that ?
The player application could exclude the dash format (and other playlist 
formats) from the format_whitelist I guess?

The alternative for the player application if it doesn't need to depend 
on the system installation of FFmpeg libraries would be to exclude 
unwanted formats at compilation time.

Regards, Tobias

_______________________________________________
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: fail on probing non mpd file extension

[Michael Niedermayer]

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

On Tue, May 09, 2023 at 09:35:09AM +0200, Tobias Rapp wrote:
> On 09/05/2023 08:19, Anton Khirnov wrote:
> 
> > Quoting Michael Niedermayer (2023-05-09 00:35:08)
> > > On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> > > > [...]
> > > > DASH is usually transferred over HTTP where file extensions are of minor
> > > > interest, the relevant type information is in the Mime-Type header.
> > > would anyone be opposed to return 0 from dash_probe() when
> > > both the mime_type and the extension are wrong ?
> > I would.
> > 
> > probe() is for probing, not implementing security policies. IMO trying
> > to fix security issues at the wrong layer will only lead to more
> > confusion, more complexity, and LESS security.
> 
> I agree that probing should be unrelated to the actual format selection
> policy.
> 

> > > example: a crafted image.jpeg uploaded somewhere is played as dash.
> > > or am i missing something that would stop that ?
> The player application could exclude the dash format (and other playlist
> formats) from the format_whitelist I guess?

That would push the problem down to every application which is really
not a very good solution

Its even worse because every player than needs to also know which format
is a playlist format. Including all future ones and then also if the user
minds them being disabled completely

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2


[-- 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 1/3] avformat/dashdec: fail on probing non mpd file extension

[Michael Niedermayer]

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

On Tue, May 09, 2023 at 08:19:36AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-05-09 00:35:08)
> > On Mon, May 08, 2023 at 04:05:40PM +0200, Tobias Rapp wrote:
> > > On 08/05/2023 14:00, James Almer wrote:
> > > 
> > > > On 5/6/2023 10:25 AM, Michael Niedermayer wrote:
> > > > > Its unexpected that a .avi or other "standard" file turns into a
> > > > > playlist.
> > > > > The goal of this patch is to avoid this unexpected behavior and possible
> > > > > privacy or security differences.
> > > > > 
> > > > > This is similar to the same change to hls
> > > > > 
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >   libavformat/dashdec.c | 11 +++++++----
> > > > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > > > > index 29d4680c68..294e14150d 100644
> > > > > --- a/libavformat/dashdec.c
> > > > > +++ b/libavformat/dashdec.c
> > > > > @@ -2336,10 +2336,13 @@ static int dash_probe(const AVProbeData *p)
> > > > >           av_stristr(p->buf, "dash:profile:isoff-live:2011") ||
> > > > >           av_stristr(p->buf, "dash:profile:isoff-live:2012") ||
> > > > >           av_stristr(p->buf, "dash:profile:isoff-main:2011") ||
> > > > > -        av_stristr(p->buf, "3GPP:PSS:profile:DASH1")) {
> > > > > -        return AVPROBE_SCORE_MAX;
> > > > > -    }
> > > > > -    if (av_stristr(p->buf, "dash:profile")) {
> > > > > +        av_stristr(p->buf, "3GPP:PSS:profile:DASH1") ||
> > > > > +        av_stristr(p->buf, "dash:profile")) {
> > > > > +        if (!av_match_ext(p->filename, "mpd")) {
> > > > > +            av_log(NULL, AV_LOG_ERROR, "Not detecting dash with non
> > > > > standard extension\n");
> > > > > +            return 0;
> > > > > +        }
> > > > > +
> > > > >           return AVPROBE_SCORE_MAX;
> > > > >       }
> > > > 
> > > > Failing because it didn't match an extensions sort of goes against the
> > > > point of probing, which even has a low score return value that's
> > > > basically "it matched extension" as a sort of last resort.
> > > > 
> > > > I'd say wrap this in a FF_COMPLIANCE_STRICT check (since i assume the
> > > > spec does state mpd must be the extension), but i think we have no
> > > > access to the AVFormatContext here?
> > > 
> > > DASH is usually transferred over HTTP where file extensions are of minor
> > > interest, the relevant type information is in the Mime-Type header.
> > 
> > would anyone be opposed to return 0 from dash_probe() when
> > both the mime_type and the extension are wrong ?
> 
> I would.
> 
> probe() is for probing, not implementing security policies. IMO trying
> to fix security issues at the wrong layer will only lead to more
> confusion, more complexity, and LESS security.

YES i agree, probe is not for security policies

Its for probing but IMHO
If you have a
taxreport.pdf that parses correctly as jar and installs jRAT if you execute it
Then it would be valid for probe() to identify this as type exploit instead
of type jar. And doing so would be more secure.

This is really more along the line of thought here for hls too.
a file with avi/mkv/mov/mxf/mpg/mp4 extension is not a hls playlist
Could someone have added that extension by mistake, yes
similarly your jar file could be named .pdf by mistake. But thats not 
a good default assumtation and i dont think anyone would assume that
by default.

thx

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

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier

[-- 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 1/3] avformat/dashdec: fail on probing non mpd file extension

[Tobias Rapp]

On 09/05/2023 22:44, Michael Niedermayer wrote:

> On Tue, May 09, 2023 at 08:19:36AM +0200, Anton Khirnov wrote:
>> Quoting Michael Niedermayer (2023-05-09 00:35:08)
>>> [...]
>>> would anyone be opposed to return 0 from dash_probe() when
>>> both the mime_type and the extension are wrong ?
>> I would.
>>
>> probe() is for probing, not implementing security policies. IMO trying
>> to fix security issues at the wrong layer will only lead to more
>> confusion, more complexity, and LESS security.
> YES i agree, probe is not for security policies
>
> Its for probing but IMHO
> If you have a
> taxreport.pdf that parses correctly as jar and installs jRAT if you execute it
> Then it would be valid for probe() to identify this as type exploit instead
> of type jar. And doing so would be more secure.
>
> This is really more along the line of thought here for hls too.
> a file with avi/mkv/mov/mxf/mpg/mp4 extension is not a hls playlist
> Could someone have added that extension by mistake, yes
> similarly your jar file could be named .pdf by mistake. But thats not
> a good default assumtation and i dont think anyone would assume that
> by default.
>
> thx
>
> [...]

But if the application expects a HLS playlist would it really be a 
problem if the input file ends with .avi or some other extension? The 
probe function just doesn't know what the application expects. 
Expectation and actual input type are aligned after probe.

Regards, Tobias

_______________________________________________
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: fail on probing non mpd file extension

[Michael Niedermayer]

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

On Wed, May 10, 2023 at 08:44:31AM +0200, Tobias Rapp wrote:
> On 09/05/2023 22:44, Michael Niedermayer wrote:
> 
> > On Tue, May 09, 2023 at 08:19:36AM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2023-05-09 00:35:08)
> > > > [...]
> > > > would anyone be opposed to return 0 from dash_probe() when
> > > > both the mime_type and the extension are wrong ?
> > > I would.
> > > 
> > > probe() is for probing, not implementing security policies. IMO trying
> > > to fix security issues at the wrong layer will only lead to more
> > > confusion, more complexity, and LESS security.
> > YES i agree, probe is not for security policies
> > 
> > Its for probing but IMHO
> > If you have a
> > taxreport.pdf that parses correctly as jar and installs jRAT if you execute it
> > Then it would be valid for probe() to identify this as type exploit instead
> > of type jar. And doing so would be more secure.
> > 
> > This is really more along the line of thought here for hls too.
> > a file with avi/mkv/mov/mxf/mpg/mp4 extension is not a hls playlist
> > Could someone have added that extension by mistake, yes
> > similarly your jar file could be named .pdf by mistake. But thats not
> > a good default assumtation and i dont think anyone would assume that
> > by default.
> > 
> > thx
> > 
> > [...]
> 
> But if the application expects a HLS playlist would it really be a problem
> if the input file ends with .avi or some other extension? The probe function
> just doesn't know what the application expects. Expectation and actual input
> type are aligned after probe.

if the application is just for hls then sure you are correct but then also
why would the application even run probe ? it would be a waste of cpu cycles

from another direction, i think this viewpoint while true is too much
going to special case optimization. 

Maybe we can factor the hls probe in 2 cases. One would be
unambiguous hls (mime/extension content all matching)
ambigous hls (mime/extension not correct or maybe some very odd URLS in it but otherwise valid hls)

This would not loose any detected files but would give more details
to user apps and users to make the choice.
a user or app could then simply include or not include the ambigous hls
in the format whitelist or blacklist
This would also not complicate the API but just use the existing features

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

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