[FFmpeg-devel] [PATCH 2/2] avformat/hls: .ts is always ok even if its a mov/mp4

[Kacper Michajlow]

On Tue, 28 Jan 2025 at 22:44, Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> Hi
>
> On Tue, Jan 28, 2025 at 10:12:30PM +0200, Jan Ekström wrote:
> > On Tue, Jan 28, 2025 at 4:24 PM Michael Niedermayer
> > <michael at niedermayer.cc> wrote:
> > >
> > > Maybe fixes: 11435
> > >
> >
> > Do I understand correctly that the root issue that's being attempted
> > to be fixed by the initial patch set is that unusual demuxers were
> > possible to have been probed and opened through the HLS meta demuxer?
> > In that case I would say that instead of trying to make very nebulous
> > and easily breakable extension based checking, maybe this demuxer
> > should just limit its default usable input formats?
> >
> > To my knowledge the officially utilized container formats for HLS are
> > MPEG-TS, MP4-likes (fragmented mp4) and raw audio formats such as AAC,
> > MP3 or AC-3. One could check what hls.js or ExoPlayer support, and
> > that should be a generally mostly encompassing thing that does not
> > depend on what extensions are in use. Adding an AVOption to add
> > additional formats without code changes would then allow for some
> > outliers to be added by users.
>
> there is extended M3U
> https://en.wikipedia.org/wiki/M3U
>
> that allows a wide range of things in it
>
> our hls demuxer can read these, if we limit to mpeg-ts/mp4 we would remove
> support for these.
>
> having an AVOption that is needed for some files is bad because
> then people turn it on and if that makes it insecure ...
>
>
> >
> > Finally, the `-f` format definition option and whatever logic
> > underneath it is just splitting the name on comma and then matching.
> > There probably is a helper function for this in the code base. This
> > enables just matching against "mp4" or so. While I consider it
>
> yeah, you are correct thats ugly, ill fix that
>
> <JEEB> michaelni: alternatively I feel like since we've had meta demuxers have these issues having demuxers utilized that just read text from input, maybe we should just have some general meta demuxer logic which allows or disallows specific formats. allowlist is simpler since even if someone adds a new one it will not automatically be allowed (thus not enabling possible new "holes"), but on the other hand if
> <JEEB> we can notice such formats as new ones happen to get added then a blocklist based on a "not allowed in meta demuxers" or "may easily cause information exfiltration" per-format flag might also be quite possible
>
> there are many things that can be done, iam not against this
> the CVE IIRC explicitly suggested extension checks
>
>
> <nevcairiel> dont we already have tons of whitelists for things, having a demuxer whitelist sounds a lot better then doing it on extension, which is meaningless in msot cases - as seen here, ts is just used for anything
>
> we have tons of whitelists and i have suggested their use everywhere including
> the commit messages
>
> They have some problem though and that is they are unflexible if set to
> a fixed value by hand
> if you get a unknown generic input what whitelist do you use ?
> you need to have the demuxer on it that that file needs but you dont know
> which that is.
>
> I dont like hls and i dont like how our hls demuxer is "shared" with more
> generic EXTM3U code. I think these 2 should be seperate AVInputFormat
> in the same hls.c
> Where the true HLS should be more locked down like you describe by default
>
> but still teh more generic code benefits from simple checks we can
> throw at it.
> Simply blacklisting tty "demuxers" feels like a risky fix though
> because tty is used by an attacker because its simple not because its the only
> way to exfiltrate data. If you just run a raw decoder or pcm decoder or
> even something simple like a RLE decoder you can likely achieve the same
> its just an extra layer of work
>
> Iam not sure and iam not feeling confident on any hls fix. This is all
> more incremental improvments. Even if you can only run a TS demuxer or
> MOV demuxer over /etc/passwd, i would not feel safe

Hi,

I think the main point here is that FFmpeg is being forced to
implement "layers" of security that, at best, provide a false sense of
assurance. You should never run a large media processing library, one
that relies on dozens of third-party dependencies, without proper
sandboxing and strict access controls for relevant files.

That said, I do see some merit in extension filtering as a way to
reduce the scope of what FFmpeg attempts to read. However, we must
ensure that it aligns with the HLS specification and the practices of
major media providers. In #11435, I provided another example
(Twitter/X) that is currently being filtered in HEAD (88a8ba5c). (and
I'm sure there are servers that feeds files without extension and only
mime type)

I believe changes like this create more friction for users than actual
security benefits. I get it. Someone needed to hit their KPI by
submitting CVEs, and they found a marginally applicable case of a
highly unrealistic attack scenario.

But FFmpeg should be cautious about adopting questionable security
measures, such as:

> DASH playlists should restrict URIs to data:// and file:// unless otherwise specified with protocol_whitelist.

I mean, cool, but isn't DASH a Dynamic Adaptive Streaming over HTTP?

In summary, I believe the ability of FFmpeg to open or parse certain
formats is highly dependent on the deployment environment. If you
provide a service that allows foreign playlists to be opened on your
server, it is your responsibility to restrict access appropriately,
whether through sandboxing, firewalls, or by disabling unnecessary
demuxers and features in your FFmpeg binaries to minimize the attack
surface. There's even a useful configuration option to disable
networking if that suits your needs. For example, I fully expect my
libavformat to open DASH streams using the HTTP protocol, and I don’t
consider that a CVE issue simply because it has that capability.

- Kacper