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