[FFmpeg-devel] [PATCH] avformat/hls: look for trailing GET headers with m3u8 extension check

[Michael Niedermayer]

On Sat, May 13, 2023 at 01:06:25PM -0400, Leo Izen wrote:
> 
> 
> On 5/13/23 10:54, Michael Niedermayer wrote:
> > On Fri, May 12, 2023 at 04:26:22PM -0400, Leo Izen wrote:
> > > After commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1 we refuse to use
> > > URLs of the form https://foo.bar/baz.m3u8?foo=bar because it fails the
> > > file extension check. This commit strips the ?foo=bar at the end before
> > > checking the file extension.
> > > 
> > > Signed-off-by: Leo Izen <leo.izen at gmail.com>
> > > ---
> > >   libavformat/hls.c | 11 ++++++++++-
> > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > > index 11e345b280..6a97cced17 100644
> > > --- a/libavformat/hls.c
> > > +++ b/libavformat/hls.c
> > > @@ -2534,7 +2534,16 @@ static int hls_probe(const AVProbeData *p)
> > >           strstr(p->buf, "#EXT-X-TARGETDURATION:") ||
> > >           strstr(p->buf, "#EXT-X-MEDIA-SEQUENCE:")) {
> > > -        if (!av_match_ext(p->filename, "m3u8,hls,m3u")) {
> > > +        char *request_qmark = strchr(p->filename, '?');
> > > +        int match_ext;
> > > +
> > > +        if (request_qmark)
> > > +            *request_qmark = '\0';
> > > +        match_ext = av_match_ext(p->filename, "m3u8,hls,m3u");
> > > +        if (request_qmark)
> > > +            *request_qmark = '?';
> > > +
> > > +        if (!match_ext) {
> > >               av_log(NULL, AV_LOG_ERROR, "Not detecting m3u8/hls with non standard extension\n");
> > >               return 0;
> > >           }
> > 
> > the av_match_ext here matches the probe code
> > all should be fixed. Also differences between local files and urls should
> > be considered in extension extraction
> 
> If you're requiring 

That way you word this is a little odd


> that we check that a file is local before stripping
> tailing request headers, how would you check if a file is local? having a
> scheme:// is not sufficient to make that check, as file:// is a valid
> scheme. You could check for https?:// I suppose, but the spec doesn't
> actually require that HTTP be used (section 2):
> 
>    Data SHOULD be carried over HTTP [RFC7230], but,
>    in general, a URI can specify any protocol that can reliably transfer
>    the specified resource on demand.

ATM the extension handling across the codebase treats everything like filenames
not like URIs, "?" has no special meaning.
You add unconditional special meaning to "?" in one function ignoring
everything else. I dont think thats improving the overall extension handling

But lets consider:
file:///home/myname/myfile.m3u8?file.avi
/home/myname/myfile.m3u8?file.avi
http:/server/myfile.m3u8?file.avi

The first is odd, iam not sure what "?file.avi" is and i wonder if we
could simply reject this at file protocol level.
If its accepted, I think it would map to /home/myname/myfile.m3u8 on disk
not "/home/myname/myfile.m3u8?file.avi"
Thats also how my web browser seems to interpret a file:///... the ?foobar part seems
stripped

OTOH /home/myname/myfile.m3u8?file.avi is a avi file with avi extension
its oddly named but its valid

the 3rd is a m3u8 file/script or whatever with file.avi as a parameter


> 
> Do note that your original patch is not spec-compliant. RFC 8216 section 4
> says the following:
> 
>    Each Playlist file MUST be identifiable either by the path component
>    of its URI or by HTTP Content-Type.  In the first case, the path MUST
>    end with either .m3u8 or .m3u.  In the second, the HTTP Content-Type
>    MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl".  Clients
>    SHOULD refuse to parse Playlists that are not so identified.

The MUST statements sound like a muxer/server side requirement.
the SHOULD would affect us and tells us to reject not to accept


> 
> 
> This implies that (1) .hls is not a valid extension if that is being used,
> and 

do you suggest we should not accept .hls files ?


> (2) a valid HLS mimetype in a content-type header is sufficient to mark
> a file as HLS regardless of the extension used.

There are at least 4 cases here
A extension is m3u8/m3u
B extension is a well known non hls type (txt,avi,mkv,...), mime type is *hls* , 
C extension is something else,                              mime type is *hls* , 
D extension is not m3u8/m3u,                                mime type is not *hls*

In case of A and C we should detect hls by default, thats needed so our code
works without annoying the user
In D we should not detect hls, this is the SHOULD in the RFC

The B case is a oddball, does this case exist in non malicious cases ?


This matter is touching quite a few seperate areas so its very possible iam
missing something

thx

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230514/e3950a70/attachment.sig>