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

[Leo Izen]

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.

> 
> 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.