[MPlayer-dev-eng] demux_mf: improve format string processing
avihpit at yahoo.com
Thu Apr 29 18:32:00 EEST 2021
On Thursday, April 29, 2021, 04:19:11 PM GMT+3, Alexander Strasser <eclipse7 at gmx.net> 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:
> > 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 :)
More information about the MPlayer-dev-eng