From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id B991B44C64 for ; Sun, 14 May 2023 20:43:43 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 52CD068BF26; Sun, 14 May 2023 23:43:40 +0300 (EEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id E96D168BE6B for ; Sun, 14 May 2023 23:43:33 +0300 (EEST) Received: (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id B8BC7E0004 for ; Sun, 14 May 2023 20:43:32 +0000 (UTC) Date: Sun, 14 May 2023 22:43:29 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20230514204329.GI1391451@pb2> References: <20230512202622.29531-1-leo.izen@gmail.com> <20230513145422.GG1391451@pb2> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avformat/hls: look for trailing GET headers with m3u8 extension check X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============5148180108766262610==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============5148180108766262610== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2zkT5PsbWu6kxoCU" Content-Disposition: inline --2zkT5PsbWu6kxoCU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, May 13, 2023 at 01:06:25PM -0400, Leo Izen wrote: >=20 >=20 > 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=3Dbar because it fails = the > > > file extension check. This commit strips the ?foo=3Dbar at the end be= fore > > > checking the file extension. > > >=20 > > > Signed-off-by: Leo Izen > > > --- > > > libavformat/hls.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > >=20 > > > 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 =3D strchr(p->filename, '?'); > > > + int match_ext; > > > + > > > + if (request_qmark) > > > + *request_qmark =3D '\0'; > > > + match_ext =3D av_match_ext(p->filename, "m3u8,hls,m3u"); > > > + if (request_qmark) > > > + *request_qmark =3D '?'; > > > + > > > + if (!match_ext) { > > > av_log(NULL, AV_LOG_ERROR, "Not detecting m3u8/hls with= non standard extension\n"); > > > return 0; > > > } > >=20 > > the av_match_ext here matches the probe code > > all should be fixed. Also differences between local files and urls shou= ld > > be considered in extension extraction >=20 > If you're requiring=20 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): >=20 > 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 filen= ames 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 >=20 > Do note that your original patch is not spec-compliant. RFC 8216 section 4 > says the following: >=20 > 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 >=20 >=20 > This implies that (1) .hls is not a valid extension if that is being used, > and=20 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 *h= ls* ,=20 C extension is something else, mime type is *h= ls* ,=20 D extension is not m3u8/m3u, mime type is no= t *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 [...] --=20 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 --2zkT5PsbWu6kxoCU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZGFH7QAKCRBhHseHBAsP qx23AJ90YlNQ1qlb9TvaCFzRulkqvy8l6QCdEO6czvCfVsfqmRggIPODRzNZv7Q= =pZ2v -----END PGP SIGNATURE----- --2zkT5PsbWu6kxoCU-- --===============5148180108766262610== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============5148180108766262610==--