demux_mf: improve format string processing
Forgive me, not in a patch format... copied from irc: <JEEB> since this code originates in mplayer and I wasn't able to find an up-to-date web-viewable SVN repo, this might be applicable to mplayer as well https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabda...
Hi all! @Compn: Thanks for forwarding. Many thanks to the mpv folks for letting us know about this. avih and jeeb are Bcc'ed on this mail. Maybe you are interested in our take on this or further collaboration. There is also a second patch for an entirely different problem linked below, that might be applicable to mpv in demux_mf.c : probe_format . On 2021-04-09 12:10 -0700, Patriot ACT wrote:
Forgive me, not in a patch format... copied from irc:
<JEEB> since this code originates in mplayer and I wasn't able to find an up-to-date web-viewable SVN repo, this might be applicable to mplayer as well
https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabda...
That code is still in MPlayer. After some discussion with Reimar, we concluded that mostly everything is lost if the attacker can control the command line or more in general the arguments passed to the mplayer process. The sprintf is bad anyway, so I painfully came up with a patch to replace it. While doing that I promptly stumbled over another bug. Both patches sent here in this thread: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2021-April/073973.html 1. libmpdemux/mf: Replace sprintf by mp_asprintf 2. libmpdemux/demux_mf: Don't crash if no paths were selected Starting to wonder how exactly this functionality came to happen and evolved, I did a little code archaeology. The core functionality is from 2 commits at 6th and 7th February of 2002. That is after I started using MPlayer around 2001, but years before I started reading or even working on MPlayer's code base. On the first day it included support for globbing and on its second day, support for printf format strings was added. Over the years mf:// was expanded. In March 2002 support for "comma delimited filenames" was added. In November 2005 support for using a text file containing the file names one per line was added. In all those years I think the format of the URL was never documented. That part the man page describes as `filemask|@listfile`. In MEncoder's HTML docs some amount of documentation-by-sample command lines can be found. Finding out this wasn't documented for almost 20 year, I actually think it would make sense to have this documentation and to rethink what it should look like. If we can come up with a good and mostly backward compatible sane documentation for this feature I would like to see it reimplemented from scratch. Not sure when or if it will ever happen, but as a first step in that direction I will start with a documentation for the current state of mf: ---8<--- mf://[<mf spec>] The *mf spec* is a specification from which the input paths should be deduced. It can be one of these: <glob pattern>|<printf format>|<comma delimited paths>|@<list file> If the *mf spec* is left out, a shell globbing pattern of `*` is used. ## list file If the first character of after *mf spec* is `@`, the remaining characters will be used as a path to open as a file. If the file is readable, each line will be interpreted as one path for the multi file input. ## comma delimited paths If *mf spec* contains a `,` it is considered to be list of comma delimited paths, that will be used as input paths in that order. ## glob pattern If it **doesn't** contain a `%` it is assumed to be a glob pattern. The glob pattern will be evaluated and the resulting list of files if any, will be used as paths for the multi file input. If the glob pattern doesn't contain a `*`, it will be appended. ## printf format If *mf spec* **didn't** yet match any of the cases described above, it will be considered to be a printf format string. This string will be used to render an input path with a single int argument. This argument is a counter starting at zero, incrementing by one after each call. Should it happen that for, in sum, 5 of any of the rendered paths the file status (stat) couldn't be retrieved, the path sequence will be considered finished. --->8--- WARNING: I reverse engineered the description from the source code, but didn't actually test if all my assertions are correct. Best regards, Alexander
Hello Alexander, On 28.04.21 08:47, Alexander Strasser wrote:
[...] Not sure when or if it will ever happen, but as a first step in that direction I will start with a documentation for the current state of mf:
If you do re-implement this functionality, it might be nice to have a way of "quoting" special characters. This is just a suggestion, of course. I'll add some details below. If you do re-implement this functionality, and if you do add some kind of quoting mechanism, I would prefer consistent quoting (at least) inside the <mf spec>, e.g., a backslash ('\') removes the special meaning from the following character, including "\\" to get a single backslash ('\'). As opposed to, e.g., doubling special characters if they shall be taken literally in one of the sub-specifications. (I did not check if MPlayer already supports some kind of quoting, since I have not yet needed something like this.)
---8<---
mf://[<mf spec>]
The *mf spec* is a specification from which the input paths should be deduced. It can be one of these:
<glob pattern>|<printf format>|<comma delimited paths>|@<list file>
If the *mf spec* is left out, a shell globbing pattern of `*` is used.
## list file
If the first character of after *mf spec* is `@`, the remaining characters will be used as a path to open as a file.
If a file name starts with '@' this can probably worked around by using "./@...", thus quoting does not seem to be essential here.
If the file is readable, each line will be interpreted as one path for the multi file input.
## comma delimited paths
If *mf spec* contains a `,` it is considered to be list of comma delimited paths, that will be used as input paths in that order.
This seems a bit more restrictive in that file names that contain a comma would need some way to prevent this rule from kicking in ("quoting").
## glob pattern
If it **doesn't** contain a `%` it is assumed to be a glob pattern.
It might be helpful to have a method of allowing '%' in a glob pattern.
The glob pattern will be evaluated and the resulting list of files if any, will be used as paths for the multi file input.
If the glob pattern doesn't contain a `*`, it will be appended.
## printf format
If *mf spec* **didn't** yet match any of the cases described above,
So this seems to be keyed on seeing '%' and thus inferring a printf format string. I would think it would be nice to have a method of quoting '%', but not necessarily the printf method "%%".
it will be considered to be a printf format string. This string will be used to render an input path with a single int argument. This argument is a counter starting at zero, incrementing by one after each call.
Should it happen that for, in sum, 5 of any of the rendered paths the file status (stat) couldn't be retrieved, the path sequence will be considered finished.
--->8---
WARNING: I reverse engineered the description from the source code, but didn't actually test if all my assertions are correct.
Thanks, Erik
Hi Erik! On 2021-04-28 10:18 +0200, Erik Auerswald wrote:
On 28.04.21 08:47, Alexander Strasser wrote:
[...] Not sure when or if it will ever happen, but as a first step in that direction I will start with a documentation for the current state of mf:
If you do re-implement this functionality, it might be nice to have a way of "quoting" special characters. This is just a suggestion, of course. I'll add some details below.
If you do re-implement this functionality, and if you do add some kind of quoting mechanism, I would prefer consistent quoting (at least) inside the <mf spec>, e.g., a backslash ('\') removes the special meaning from the following character, including "\\" to get a single backslash ('\'). As opposed to, e.g., doubling special characters if they shall be taken literally in one of the sub-specifications.
(I did not check if MPlayer already supports some kind of quoting, since I have not yet needed something like this.)
Agree. That will need some serious thinking though. Regarding your following comments, they cover mostly what I thought when reading the code and even some more aspects. The most annoying thing is probably that you can't avoid the <comma delimited paths> interpretation. E.g. if your files contain a `,` in their names, you need to rename them or put them in a list file and load that with @<list file> or use a glob pattern that hides the `,`. For my taste mf:// works a bit too much in the do-what-i-mean fashion. That's fine and simple for ad-hoc usage most of the time, but it's not very nice if you want to use it e.g. out of another program. Thanks for your thoughts on this, Alexander
---8<---
mf://[<mf spec>]
The *mf spec* is a specification from which the input paths should be deduced. It can be one of these:
<glob pattern>|<printf format>|<comma delimited paths>|@<list file>
If the *mf spec* is left out, a shell globbing pattern of `*` is used.
## list file
If the first character of after *mf spec* is `@`, the remaining characters will be used as a path to open as a file.
If a file name starts with '@' this can probably worked around by using "./@...", thus quoting does not seem to be essential here.
If the file is readable, each line will be interpreted as one path for the multi file input.
## comma delimited paths
If *mf spec* contains a `,` it is considered to be list of comma delimited paths, that will be used as input paths in that order.
This seems a bit more restrictive in that file names that contain a comma would need some way to prevent this rule from kicking in ("quoting").
## glob pattern
If it **doesn't** contain a `%` it is assumed to be a glob pattern.
It might be helpful to have a method of allowing '%' in a glob pattern.
The glob pattern will be evaluated and the resulting list of files if any, will be used as paths for the multi file input.
If the glob pattern doesn't contain a `*`, it will be appended.
## printf format
If *mf spec* **didn't** yet match any of the cases described above,
So this seems to be keyed on seeing '%' and thus inferring a printf format string. I would think it would be nice to have a method of quoting '%', but not necessarily the printf method "%%".
it will be considered to be a printf format string. This string will be used to render an input path with a single int argument. This argument is a counter starting at zero, incrementing by one after each call.
Should it happen that for, in sum, 5 of any of the rendered paths the file status (stat) couldn't be retrieved, the path sequence will be considered finished.
--->8---
WARNING: I reverse engineered the description from the source code, but didn't actually test if all my assertions are correct.
Thanks, Erik
On Wednesday, April 28, 2021, 09:47:32 AM GMT+3, Alexander Strasser <eclipse7@gmx.net> wrote:
<JEEB> since this code originates in mplayer and I wasn't able to find an up-to-date web-viewable SVN repo, this might be applicable to mplayer as well
https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabda...
That code is still in MPlayer.
After some discussion with Reimar, we concluded that mostly everything is lost if the attacker can control the command line or more in general the arguments passed to the mplayer process.
The sprintf is bad anyway, so I painfully came up with a patch to replace it. While doing that I promptly stumbled over another bug. Both patches sent here in this thread: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2021-April/073973.html
1. libmpdemux/mf: Replace sprintf by mp_asprintf 2. libmpdemux/demux_mf: Don't crash if no paths were selected
Thanks. CC Stefan Schiller - the original reporter. 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 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 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 . 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 Thanks again, avih
Hi avih! Seems your message is still stuck in the ml mod queue. I quote it entirely in my reply below. On 2021-04-28 08:58 +0000, avih wrote:
On Wednesday, April 28, 2021, 09:47:32 AM GMT+3, Alexander Strasser <eclipse7@gmx.net> wrote:
<JEEB> since this code originates in mplayer and I wasn't able to find an up-to-date web-viewable SVN repo, this might be applicable to mplayer as well
https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabda...
That code is still in MPlayer.
After some discussion with Reimar, we concluded that mostly everything is lost if the attacker can control the command line or more in general the arguments passed to the mplayer process.
The sprintf is bad anyway, so I painfully came up with a patch to replace it. While doing that I promptly stumbled over another bug. Both patches sent here in this thread: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2021-April/073973.html
1. libmpdemux/mf: Replace sprintf by mp_asprintf 2. libmpdemux/demux_mf: Don't crash if no paths were selected
Thanks.
CC Stefan Schiller - the original reporter.
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.
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 For better or worse it might be easier to use playlists in the GUI. 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 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. We could probably just use av_get_frame_filename from libavformat, which is quite limited, but probably would satisfy 99.99999% of our users.
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.
Thanks again, avih
Thanks, Alexander
On Thursday, April 29, 2021, 04:19:11 PM GMT+3, Alexander Strasser <eclipse7@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: 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
On 2021-04-28 08:47 +0200, Alexander Strasser wrote: [...]
---8<---
mf://[<mf spec>]
The *mf spec* is a specification from which the input paths should be deduced. It can be one of these:
<glob pattern>|<printf format>|<comma delimited paths>|@<list file>
If the *mf spec* is left out, a shell globbing pattern of `*` is used.
## list file
If the first character of after *mf spec* is `@`, the remaining characters will be used as a path to open as a file. If the file is readable, each line will be interpreted as one path for the multi file input.
## comma delimited paths
If *mf spec* contains a `,` it is considered to be list of comma delimited paths, that will be used as input paths in that order.
## glob pattern
If it **doesn't** contain a `%` it is assumed to be a glob pattern. The glob pattern will be evaluated and the resulting list of files if any, will be used as paths for the multi file input.
If the glob pattern doesn't contain a `*`, it will be appended.
## printf format
If *mf spec* **didn't** yet match any of the cases described above, it will be considered to be a printf format string. This string will be used to render an input path with a single int argument. This argument is a counter starting at zero, incrementing by one after each call.
Should it happen that for, in sum, 5 of any of the rendered paths the file status (stat) couldn't be retrieved, the path sequence will be considered finished.
--->8---
WARNING: I reverse engineered the description from the source code, but didn't actually test if all my assertions are correct.
One correction I have noticed while working on the code lately. Only one of the pattern types * comma delimited paths * glob pattern * printf format will be tried depending on the contents of <mf spec>. If it contains a comma it will be: comma delimited paths If it doesn't contain a comma and doesn't contain % it will be: glob pattern Else it will be: printf format Alexander
participants (4)
-
Alexander Strasser -
avih -
Erik Auerswald -
Patriot ACT