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 1C38844CA3 for ; Sun, 14 May 2023 21:04:10 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0FBA068BF73; Mon, 15 May 2023 00:04:07 +0300 (EEST) Received: from mail-yb1-f174.google.com (mail-yb1-f174.google.com [209.85.219.174]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7481B689B83 for ; Mon, 15 May 2023 00:04:00 +0300 (EEST) Received: by mail-yb1-f174.google.com with SMTP id 3f1490d57ef6-ba7831cc80bso219816276.0 for ; Sun, 14 May 2023 14:04:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684098239; x=1686690239; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=npi0NmA46G3pF8mf1mxupNgbzRgoxIpWJk8h2mjQG9M=; b=QTSTc9g7zjPyZnMHAFQvIwiGf/Q3kBAZMkArlRXPQnrxas2yFWYXjug5MavJeYhfO6 fTQkg0UgoIzhr1OefU6gAvnBGXnTHww8U/KeqltFD5mT15i2qq2gFSH6vW/0E3nPcu26 KbTT4wl+2z+M6nSxXKN7y0AJNgX+DHJ830zgF4itgDhd4cuEYXYi/3Xy0CkH3A5XoatG aVYNqveqmcRvjP9jNNd+bqjSmJamU7C02++6lSWHAykesCVRZMiRqEa7atzoa3fja6uK xywd53Mp/3lh0U1aryuEFqXsJO+Cz5MNPdTnocRgOIp2HJh+za8HhmzFM+GS0VjVjNwg Qm0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684098239; x=1686690239; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=npi0NmA46G3pF8mf1mxupNgbzRgoxIpWJk8h2mjQG9M=; b=h0vAD1kFLZfdWHqCrxjNYHvfrgXzGM4++csLGp0GiMrof64O6aemlxEcOm/5oqe7z5 AzKyvXqD7nJgCCbTIvbQUKbUXP4vpGy9a3AuNcJvzs6qzjCK/KOAAi/TCDLmPR033mJi 93eK79YvdukvpixyQj5KZp0RBQ87bHhPxy2l2TXBunIQM/NxPPNbt/jbJrPfaO6wt8bF Nt2vF30gAES8OVok+dG/TEuvPuHGoLiEQ1k20ugizQPQWQ+nJfx0ydDZFvczjanUVZvQ G9X3ovgDcLR1PWu0+WTTQ88nWevIPQD39dX+j4uMNc98HD6RAzVEBan+GyxkqjHM8WnG kGFw== X-Gm-Message-State: AC+VfDwJXf24qwjpK+pjVQk1Ofx58ko9BTQD8fwyMyZMm13sojGUPDrs 1SbSeoHIoVf9Gqp2+gxcO/kTPd+8y/0= X-Google-Smtp-Source: ACHHUZ6xTXjdvJA2Am1SixSgxuzrThaDtsc3mC8k1My9EfIg0PdQJuSnw3P6RV794rTI+aInO//voA== X-Received: by 2002:a25:698f:0:b0:ba7:498a:46f with SMTP id e137-20020a25698f000000b00ba7498a046fmr5079549ybc.2.1684098238667; Sun, 14 May 2023 14:03:58 -0700 (PDT) Received: from [192.168.1.35] (c-98-224-219-15.hsd1.mi.comcast.net. [98.224.219.15]) by smtp.gmail.com with ESMTPSA id p6-20020a259986000000b00b9b1d09ed18sm6441860ybo.33.2023.05.14.14.03.57 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 14 May 2023 14:03:58 -0700 (PDT) Message-ID: Date: Sun, 14 May 2023 17:03:57 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 To: ffmpeg-devel@ffmpeg.org References: <20230512202622.29531-1-leo.izen@gmail.com> <20230513145422.GG1391451@pb2> <20230514204329.GI1391451@pb2> Content-Language: en-US-large From: Leo Izen In-Reply-To: <20230514204329.GI1391451@pb2> 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 >>>> --- >>>> 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. > > 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. > 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 ? > I actually suggest we *not reject by file extension* > >> (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 > Yes, you're missing that if the *contents* contain *HLS* contents then we shouldn't refuse to probe the file based on the filename. That's not how *any of the other probe options* work. Using filename to determine file type instead of contents is not security. It's actually the opposite of security. _______________________________________________ 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".