Re: svn commit: r1452780 - /subversion/trunk/subversion/libsvn_subr/subst.c

[Stefan Sperling <st...@elego.de>]

On Sat, May 11, 2013 at 01:28:26PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Tue, Mar 05, 2013 at 14:24:32 +0100:
> > If the temp file is within a system-wide temp dir, and we end up expanding
> 
> When exactly is that going to happen?  You call svn_stream_open_unique()
> with dirpath != NULL, so the tempdir it will use is ./ (neither /tmp or
> .svn/tmp/).

It's not going to live in the system tempdir in the case under discussion.

My point was that I would not like the code to be reused in a context
where the file could end up in /tmp (where dirpath is a system tempdir).
We could add a comment to explain the problem -- then I would be fine
with any approach that works.

> > in which case this doesn't really matter. If the working copy contains
> > secrets it had better be within a directory that untrusted users cannot
> > access. But I think the code should at least set a good example.
> > 
> 
> So, can someone explain to me what the "other" race condition is?  The
> current code is racy because DST_TMP has (0777 &~ umask) perms while it
> is being written to.

DST_TMP has more restrictive permissions than that. It is created with
the mkstemp() function under the hood. So we can assume that the perms
of DST_TMP are configured such that only the user running svn can access
the file. So we always start off with something like 600 on DST_TMP.
And we need to preserve the permission bits of the existing DST
(whatever they are) when renaming DST_TMP on top of it.

Re: svn commit: r1452780 - /subversion/trunk/subversion/libsvn_subr/subst.c

[Branko Čibej <br...@wandisco.com>]

On 11.05.2013 14:10, Stefan Sperling wrote:
> On Sat, May 11, 2013 at 01:50:50PM +0200, Branko Čibej wrote:
>> On 11.05.2013 13:31, Stefan Sperling wrote:
>>> DST_TMP has more restrictive permissions than that. It is created with
>>> the mkstemp() function under the hood. So we can assume that the perms
>>> of DST_TMP are configured such that only the user running svn can
>>> access the file. So we always start off with something like 600 on
>>> DST_TMP. And we need to preserve the permission bits of the existing
>>> DST (whatever they are) when renaming DST_TMP on top of it. 
>> So, on Unix, you chmod DST_TMP to DST's permissions before renaming.
> Let me reiterate...
>
> In this case, chmod DST_TMP usually means *expanding* DST_TMP's permissions
> from 600 to something like 644, potentially giving other people read access
> to DST_TMP for a brief amount of time (after chmod and before the rename).
>
> If that is done with DST_TMP living in the system temp dir, this opens
> the possibility for other users to read the file, which might contain
> some secret. Hence I'd prefer to rename first, and then chmod.

Yes, that's safer in general. It leaves the other race (e.g., editors
not being able to read the file), but that's indeed secondary.

> If we rename first the working file DST temporarily has more restrictive
> permissions but there is no risk of exposing secrets to other users.
> The location of DST is assumed to be safely protected, e.g. via perms
> on a parent directory of DST.
>
> This is not a practical issue for the case at hand, because we know that
> the tempfile is in .svn/tmp, i.e. it is protected in the same way as DST
> already is. So I'm willing to chmod/rename in any order people prefer,
> but will add a comment in case we chmod first, to alert people tempted to
> reuse this code in a different case where DST_TMP lives in the system tempdir.
>
> Does that make sense?

+1

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com

Re: svn commit: r1452780 - /subversion/trunk/subversion/libsvn_subr/subst.c

[Stefan Sperling <st...@elego.de>]

On Sat, May 11, 2013 at 01:50:50PM +0200, Branko Čibej wrote:
> On 11.05.2013 13:31, Stefan Sperling wrote:
> > DST_TMP has more restrictive permissions than that. It is created with
> > the mkstemp() function under the hood. So we can assume that the perms
> > of DST_TMP are configured such that only the user running svn can
> > access the file. So we always start off with something like 600 on
> > DST_TMP. And we need to preserve the permission bits of the existing
> > DST (whatever they are) when renaming DST_TMP on top of it. 
> 
> So, on Unix, you chmod DST_TMP to DST's permissions before renaming.

Let me reiterate...

In this case, chmod DST_TMP usually means *expanding* DST_TMP's permissions
from 600 to something like 644, potentially giving other people read access
to DST_TMP for a brief amount of time (after chmod and before the rename).

If that is done with DST_TMP living in the system temp dir, this opens
the possibility for other users to read the file, which might contain
some secret. Hence I'd prefer to rename first, and then chmod.

If we rename first the working file DST temporarily has more restrictive
permissions but there is no risk of exposing secrets to other users.
The location of DST is assumed to be safely protected, e.g. via perms
on a parent directory of DST.

This is not a practical issue for the case at hand, because we know that
the tempfile is in .svn/tmp, i.e. it is protected in the same way as DST
already is. So I'm willing to chmod/rename in any order people prefer,
but will add a comment in case we chmod first, to alert people tempted to
reuse this code in a different case where DST_TMP lives in the system tempdir.

Does that make sense?

Re: svn commit: r1452780 - /subversion/trunk/subversion/libsvn_subr/subst.c

[Branko Čibej <br...@wandisco.com>]

On 11.05.2013 13:31, Stefan Sperling wrote:
> DST_TMP has more restrictive permissions than that. It is created with
> the mkstemp() function under the hood. So we can assume that the perms
> of DST_TMP are configured such that only the user running svn can
> access the file. So we always start off with something like 600 on
> DST_TMP. And we need to preserve the permission bits of the existing
> DST (whatever they are) when renaming DST_TMP on top of it. 

So, on Unix, you chmod DST_TMP to DST's permissions before renaming. I
don't remember offhand what you do on Windows.

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com