Re: demux_mf: improve format string processing

 
On Thursday, April 29, 2021, 04:19:11 PM GMT+3, Alexander Strasser <[email protected]> wrote:

>
> Hi avih!
>
> Seems your message is still stuck in the ml mod queue. I quote it
> entirely in my reply below.

It does seem so. I guess the moderator is busy :) Thanks.


> On 2021-04-28 08:58 +0000, avih wrote:
>
> > As for patch 2/2, as far as I can tell mpv is unaffected. Right after
> > calling open_mf_pattern, if the number of files is less than 1 then
> > the mf result is rejected - before trying to probe it. See:
> > https://github.com/mpv-player/mpv/blob/master/demux/demux_mf.c#L365
>
> Ah sorry, I didn't notice while scanning through the code. Should be
> fine then.

No worries. I didn't know this either till I looked it up. Thanks for
the heads up.


> > Re "everything is lost if the attacker can control the command line",
> > at least in mpv it could also be part of a playlist, which could be
> > hosted remotely, as in: mpv https://evil.com/evil.m3u
>
> Playlists are not really safely supported in MPlayer since
> forever I fear.
>
> We have two options guarding playlist playback somewhat:
>
> 1. -playlist <filename>
> 2. -allow-dangerous-playlist-parsing

I'm not familiar with mplayer options, but the second option sounds
useful for cases where users configure their browser to use mplayer
to handle '.m3u' file extensions.


> To me it seems unfortunately never to be a good idea to use
> playlists with MPlayer which one can't check.
>
> Do you know how mpv secures playlist playback?

I'm not familiar with the subject and I don't know if this existed in
mplayer before mpv, but mpv has a way to limit usage by the type of
origin. Shortly after we pushed the format fix, we also limited the
mf protocol to the local filesystem:
 https://github.com/mpv-player/mpv/commit/9c120ded


> > I don't know whether or not mplayer is affected by this case, but
> > if it is, then using mp_asprintf is likely not enough, because it's
> > still vulnerable to abuse of the format via non-matching arguments,
> > and even writing to somewhat arbitrary memory locations using %n .
>
> Agree. IMHO allowing complete printf format language is too much
> for mf. Though I'm not really sure how much is useful.

Well, in our patch we allowed a very limited format, which we thought
is both useful enough and also relatively easy to secure + implement.

We accept any number of %%, and exactly one conversion specifier at
the format of the form %[.][NUM]d where NUM is 1-3 digits.

This allows to specify field width (optionally with 0 flag), or
precision. All other conversion specifiers are rejected as invalid.


> We could probably just use av_get_frame_filename from libavformat,
> which is quite limited, but probably would satisfy 99.99999% of
> our users.

I don't know enough about this to make a meaningful comment.


> > If you're interested in a deeper analysis of this vulnerability and
> > possible attacks, Stefan Schiller (the reporter) has written this:
> > https://devel0pment.de/?p=2217
>
> Nice write-up.

Agreed completely, and then some :)


> Thanks,
> Alexander

Cheers,
avih