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

[James Cowgill]

Hi,

On 20/09/16 18:09, salsaman wrote:
> 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.

That's fine, although I don't think it should be the default.

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

Ok.

>>> 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.
[...]
> 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.

That sounds good.

>> 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."

Yep mkdtemp should be secure so there's no problem here.

> 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

Thinking about this some more, there is a slight race condition here if
the user deletes the file after the checks, but before it's written. I
think the best fix would break the smogrify API unfortunately. One
alternative is to use to use open(2)'s O_CREATE | O_EXCL flags, but this
will only work if the file does not exist beforehand.

> Is there anything else I have missed ?

I think that's everything I found.

Thanks,
James

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-multimedia-maintainers/attachments/20160922/7dfd84e0/attachment.sig>