| Message ID | 20230506132503.9524-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/dashdec: fail on probing non mpd file extension | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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?
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
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".
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? [...]
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".
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 [...]
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.
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
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 [...]
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 [...]
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
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 [...]
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; }
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(-)