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

[Daniel Shahaf <da...@elego.de>]

Stefan Sperling wrote on Tue, Mar 05, 2013 at 14:24:32 +0100:
> On Tue, Mar 05, 2013 at 02:07:11PM +0100, Bert Huijben wrote:
> > > @@ -1743,7 +1743,12 @@ svn_subst_copy_and_translate4(const char
> > >      }
> > > 
> > >    /* Now that dst_tmp contains the translated data, do the atomic rename.
> > > */
> > > -  return svn_error_trace(svn_io_file_rename(dst_tmp, dst, pool));
> > > +  SVN_ERR(svn_io_file_rename(dst_tmp, dst, pool));
> > > +
> > > +  /* Preserve the source file's permission bits. */
> > > +  SVN_ERR(svn_io_copy_perms(src, dst, pool));
> > > +
> > > +  return SVN_NO_ERROR;
> > >  }
> > 
> > There is a possible race condition here, which might be resolved by copying the permissions before moving. 
> > (But most likely you just get a different race condition by switching the order)
> 
> Yes, and I believe the other race conditition is more dangerous.
> 
> 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/).

> read permission on the file in the system-wide temp dir, we might end up
> leaking secrets to observers that manage to access the temporary file after
> perms have been set but before the file is renamed.
> 
> Of course, most of the time the temp file will be within the .svn/tmp area,

Again: AFAICT, the tempfile will live in ./, neither /tmp nor .svn/tmp/.
Is that not the case?

> 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.  Creating DST_TMP and immediately chmod'ing it
would also be racy (since we don't use the third argument to open(2)), 
but that race would last a shorter amount of time, and therefore is
preferable to the current trunk@HEAD code...

> Ideally, we'd have an API we could use to obtain file permissions at the
> beginning of the translation operation, and one to set those perms when
> the operation is done. But svn_io_* doesn't provide that.
> 

It seems to me a "best" fix here would be to use the three-argument form
of open(2) --- i.e., to create the tempfile with the right permissions
to begin with, and at the end of translation just rename(2) the tempfile
without having to worry about permissions.

> BTW, svn_io_copy_file() also copies perms to the temp file before renaming it,
> and is thus vulnerable to a potential data leak as described above.
> I think we should fix that. But again, that function uses the .svn/tmp dir
> and not a system-wide one.

Again, that function passes svn_dirent_dirname(dst) for dirpath, so it
won't use the system-wide tempdir.?

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

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.