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
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,
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.
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.
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.
Received on 2013-03-05 14:25:14 CET