Bug#756565:

[salsaman]

This bug can be closed with the release of LiVES 2.8.1

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

[salsaman]

All issues noted above have been fixed. In addition:

- the terminology has been changed throughout to try to be less confusing.
The directory is now referred to as the "LiVES working directory"
everywhere.
For example prefs->tmpdir is now prefs->workdir in the C code, and $tmpdir
is now $workdir in Perl. The only exception is in the .lives config file
where the text  and  must be retained for backwards
compatilbility.

- there were a couple of playback plugins where /tmp was the default for
creating a fifo file inside. Even though the user could overwrite this, the
default has now been changed to create these files in the LiVES working
directory.

- the command "smogrify get_tempdir" has been left alone for backwards
compatibility but is marked as deprecated in the source file. A new
directive "smogrify get_workdir" has been created, this latter version
writes to stdout and other applications may read this with popen().

- a couple of places where LiVES was creating temporary files in /tmp have
been altered to create these in the working directory instead.

I believe that all issues have been addressed. I will continue testing and
examining the code over the next few days to confirm this.


Relevant patches:
https://sourceforge.net/p/lives/code/2570
https://sourceforge.net/p/lives/code/2571
https://sourceforge.net/p/lives/code/2572
https://sourceforge.net/p/lives/code/2573
https://sourceforge.net/p/lives/code/2577

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

[salsaman]

On Thu, Sep 22, 2016 at 7:56 PM, James Cowgill  wrote:


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


Actually I just had a much simpler idea. Since we are only interested in
getting the value, I can alter this function so that the value is written
to stdout instead of to /tmp.

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



signature.asc
Description: OpenPGP digital signature

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

[salsaman]

On Tue, Sep 20, 2016 at 1:03 PM, James Cowgill  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 ". This may be employed by external scripts
> > to get the working directory location for LiVES. The token  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  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 

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

[James Cowgill]

Hi,

[please don't change the subject to 'bug update' - it makes it harder to
follow threads and is totally pointless]

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?

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

> /tmp/.smogrify.* is intended to be used when an external script calls
> "smogrify get_tempdir ". This may be employed by external scripts
> to get the working directory location for LiVES. The token  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  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.

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


> 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



signature.asc
Description: OpenPGP digital signature

Bug#756565: bug update

[salsaman]

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.



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.


/tmp/.smogrify.* is intended to be used when an external script calls
"smogrify get_tempdir ". This may be employed by external scripts to
get the working directory location for LiVES. The token  is appended
to the end of the filename so this can be considered secure. The LiVES
application itself never calls this.

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.


The above changes are trivial and I will update here as soon as they are
done.

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

[James Cowgill]

Hi,

On 20/09/16 02:56, salsaman wrote:
> first of all, I am the main developer of LiVES. Please cc the address
> salsaman+li...@gmail.com  to all
> future bugs related to LiVES.

You should go to https://tracker.debian.org/pkg/lives and press the
Subscribe button in the top right corner and you'll automatically get
CCed on all bug reports.

> Secondly, there is incorrect information in this bug report.
>>>
> 
> You'll see that $curtmpdir is set to /tmp/smogrify, via code such as:
> 
> $handle=$ARGV[1];
> $curtmpdir="$tmpdir/$handle";
> 
>>>
> 
> In fact $tmpdir is a bit of a misnomer, it points to the LiVES working
> directory, which is created for LiVES at install and chosen by the user,
> (or a subdirectory of this). $handle is a random number generated for
> the clip. So in this case it would be something like
> /home/user/livestmp/34736474/ or
> /home/user/livestmp/setname/clips/434637826/

I agree that the use of $tmpdir in this case should be fine, though as
the other bug report states, ~/livestmp is an annoying name. Probably
$XDG_CACHE_HOME/lives would be better.

> In fact /tmp is not used at all.
> 
> If there is a genuine problem here I would be happy to correct it.

I'm not sure about that though. Briefly looking at smogrify, I think the
use of /tmp for these files are still insecure:

/tmp/.smogrify.*
/tmp/.smogval*
/tmp/lives-symlinks/

Thanks,
James



signature.asc
Description: OpenPGP digital signature

Bug#756565: Bug update

[salsaman]

Hi,
first of all, I am the main developer of LiVES. Please cc the address
salsaman+li...@gmail.com to all future bugs related to LiVES.

Secondly, there is incorrect information in this bug report.
>>

You'll see that $curtmpdir is set to /tmp/smogrify, via code such as:

$handle=$ARGV[1];
$curtmpdir="$tmpdir/$handle";

>>

In fact $tmpdir is a bit of a misnomer, it points to the LiVES working
directory, which is created for LiVES at install and chosen by the user,
(or a subdirectory of this). $handle is a random number generated for the
clip. So in this case it would be something like
/home/user/livestmp/34736474/ or
/home/user/livestmp/setname/clips/434637826/


In fact /tmp is not used at all.

If there is a genuine problem here I would be happy to correct it.


Regards,

Gabriel.





http://lives-video.com
https://www.openhub.net/accounts/salsaman

Bug#756565: CVE

[Henri Salo]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Have you requested CVE already? If you want I can verify this issue and create
the request.

- ---
Henri Salo
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.12 (GNU/Linux)

iEYEARECAAYFAlQOzeYACgkQXf6hBi6kbk8dlgCdFm+h5UIJ80dqKfB0oojjiQBq
OCEAoJkfLRSS8t9AOTYcN2oATzqMQFwF
=Tynm
-END PGP SIGNATURE-


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#756565: CVE

[Steve Kemp]

On Tue Sep 09, 2014 at 12:52:38 +0300, Henri Salo wrote:

 Have you requested CVE already? If you want I can verify this issue and create
 the request.

  I have not, the lack of update to the bug report made it slip my mind.

  If you'd like to confirm the issues, which shouldn't be hard, and
 request one then please do feel free.

Steve
--


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

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

[Steve Kemp]

Package: lives
Version: 1.6.2
Severity: important
Tags: security


lives contains a perl script, smogrify, which is what does
a lot of the work.

I don't want to point out line-by-line all the issues in the
smogrify script, but please consider significantly overhauling it.

There are numerous insecure uses of temporary files.  For example:

if ($command eq get_window_id) {
smog_system(xwininfo  \$curtmpdir/tmpinfo\);

smog_system(grep \Window id:\ \$curtmpdir/tmpinfo\  
\$curtmpdir/tmpinfo2\);
if (defined(open IN, $curtmpdir/tmpinfo2)) {
read IN,$win_id,128;
close IN;
}

You'll see that $curtmpdir is set to /tmp/smogrify, via code such as:

$handle=$ARGV[1];
$curtmpdir=$tmpdir/$handle;

To investigate all the issues is beyond my free timeframe, but I'd suggest
a  decent starting point is to run the whole system under strace and grep
for /tmp in open|close|unlink|creat calls.

Steve
--

-- System Information:
Debian Release: 7.6
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 3.14-0.bpo.1-amd64 (SMP w/8 CPU cores)
Locale: LANG=en_US.UTF8, LC_CTYPE=en_US.UTF8 (charmap=UTF-8) (ignored: LC_ALL 
set to en_US.UTF8)
Shell: /bin/sh linked to /bin/dash


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org