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

[Michael Niedermayer]

On Sun, May 14, 2023 at 05:03:57PM -0400, Leo Izen wrote:
> 
> 
> On 5/14/23 16:43, Michael Niedermayer wrote:
> > 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
> 
> Your ridiculous "security" check rejects files that have a .m3u8 extension
> because of query strings. This is a bug, and it needs to be fixed.

yes theres a bug and we should fix that bug


> 
> > 
> > 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"
> 
> 
> This is incorrect. Try it by naming a file "foo.m3u8?bar.txt" and run
> xdg-open 'file:///home/leo/foo.m3u8?bar.txt' and you will find that it opens
> it.

What is incorrect ?
we have some tools that will interpret "file:///home/leo/foo.m3u8?bar.txt" as
/home/leo/foo.m3u8?bar.txt and some /home/leo/foo.m3u8 on disk

I think that makes that sort of file URLs ambigous, dont you agree ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/706a6a64/attachment.sig>