Bug#756565: lives: Numerous insecure temporary files used in smogrify

[salsaman]

On Tue, Sep 20, 2016 at 1:03 PM, James Cowgill <jcowgill at debian.org> wrote:

> Hi,
>
> [please don't change the subject to 'bug update' - it makes it harder to
> follow threads and is totally pointless]
>
>
I wasnt aware I was changing the subject - it seems like one can only add
comments to this bug by sending email and of course I would need to add a
topic to the email. Hopefully just replying will leave it alone then




> On 20/09/16 15:51, salsaman wrote:
> > I would prefer to keep $tmpdir as it is, I dont see any reason to change
> > it to $XDG_CACHE_HOME as this variable is only used internally to
> > smogrify. Also using /lives would be confusing as there is already a
> > .lives file and lives-dir which are both created in $HOME. Also the user
> > can select the location of $tmpdir at install time so it may be outside
> > $HOME.
>
> I was referring to moving the ~/livestmp directory, not the $tmpdir
> variable which is just an implementation detail. Incidentally since this
> directory is world writable, it's probably vulnerable to the same
> security bugs that /tmp is. Maybe what I said before is wrong and all
> files in $tmpdir are also vulnerable?
>
>

As I mentioned already, the location of this directory is selected by the
user the first time that LiVES is run.
There is nothing forcing it to be ~/livestmp.

The directory being world writeable is a separate bug which will be
addressed there.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=798043



> > Regarding the other files:
> >
> > /tmp/lives-symlinks is only used in the Dynebolic bootable disk.
> > Effectively this can be ignored, since it is not used in normal
> operation.
>
> Why does the code still exist then? Can you confirm that it can never
> ever be executed?
>
>



> I have not investigated much, but if it can be executed then it could
> quite easily be used to allow editing of any file in a user's home
> directory.
>
> Attacker: ln -s $HOME_OTHER /tmp/lives-symlinks/$ALL_HANDLES
>
> $linksdir = "/tmp/lives-symlinks/$handle/";
> [some lines]
> $com="/bin/mkdir -p \"$linksdir\"";
> [above command exits without error]
> smog_system("/bin/chmod -R 777 \"$linksdir\"");
>
> [boom]
>
> In this particular case the kernel default security settings will
> probably stop you doing this, but that doesn't mean you can ignore the
> issue. As the kernel reports an error when doing this, smogrify will
> probably fail as well.
>
>

I rechecked and you may be correct in this case. I started reworking this
code now. /tmp will no longer used, instead all the symlinks will be
created in the individual clip directories. In addition, the directory was
set with chmod 777, now it will be created with mode 700.




> > /tmp/.smogrify.* is intended to be used when an external script calls
> > "smogrify get_tempdir <key>". This may be employed by external scripts
> > to get the working directory location for LiVES. The token <key> is
> > appended to the end of the filename so this can be considered secure.
> > The LiVES application itself never calls this.
>
> Even using a random <key> doesn't make it secure. You have to ensure any
> files in /tmp exist and are owned by the user running lives, before
> attempting to read or write to them. Otherwise an attacker could put a
> symlink there and cause the lives user to write to an arbitrary location.
>
>

OK. I will do as you recommend.



> For example, simply grepping the lives source reveals:
> lives-plugins/plugins/playback/video/oggstream.c:
> dummyvar=system("smogrify get_tempdir oggstream");
>
> This allows any user to truncate any file owned by the lives user by
> simply creating a symlink, and waiting for smogrify to be run.
>  ln -s $IMPORTANT_FILE /tmp/.smogrify.oggstream
>
>
Thanks for pointing that out.
The only other place where I know /tmp is used is in
lives-plugins/marcos-encoders. For example in lives_mkv_encoder we have:
temp_dir = tempfile.mkdtemp('', '.lives-', '/tmp/')

However the description of (Python) mkdtemp suggests that this should be
safe:
https://docs.python.org/2/library/tempfile.html

"Creates a temporary directory in the most secure manner possible. There
are no race conditions in the directory’s creation. The directory is
readable, writable, and searchable only by the creating user ID."



>
> > Actually I have a TODO there: allow a (optional) second parameter to
> > override the default directory location (/tmp); so I shall implement
> > this so that scripts can provide an alternative to /tmp if they wish.
> >
> >
> > /tmp/.smogval : this is not used, I believe you are again confused by
> > $tmpdir which as noted points to the working directory (e.g.
> > ~/livestmp). There IS a minor bug where "/tmp/.smogval*" is removed by a
> > cleanup operation. This code should be removed as it is no longer
> relevant.
>
> I just grepped for /tmp. That file might be ok in that case.
>
> > The above changes are trivial and I will update here as soon as they are
> > done.
>
> Thanks,
> James
>
>

So to summarise - issues to be addressed:

- remove old unneeded code with rm /tmp/.smogval
- do not create symlinks in /tmp, instead create them in the clip
directories
- in the case where values are written to /tmp for external scripts
(smogrify get_tempdir), check that the file exists and is owned by the user

Is there anything else I have missed ?

Regards,
Gabriel.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-multimedia-maintainers/attachments/20160920/79113ad1/attachment.html>