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_at_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.?
Received on 2013-05-11 12:29:16 CEST