On Wed, Jun 22, 2011 at 09:29:29AM +0300, Daniel Shahaf wrote:
> Noorul Islam K M wrote on Wed, Jun 22, 2011 at 11:37:13 +0530:
> >
> > I am not really sure whether this patch is correct one since the comment
> > in diff_editor.c says this
> >
> > /* This is the file that will contain the pristine repository version. It
> > is created in the admin temporary area. This file continues to exists
> > until after the diff callback is run, at which point it is deleted. */
> >
> > I can't think of the drawbacks of creating this particular temp file in
> > /tmp.
> >
>
> >From looking at the code, svn_io_open_unique_file3() would force the
> file to have a mode of 0600|umask() instead of just 0600; i.e., the
> contents of the files being diffed would leak, even if the wc root dir
> was mode 0700 (in which case they don't currently leak).
>
> Is that a problem?
It would be better to keep the perms of the temporary file at 600 indeed.
> (The comment in svn_io_open_unique_file3() indicates that there's some
> historical reason, catering to specific API consumers, for extending the
> permissions of the tempfile.)
The problem was that some tempfiles with mode 600 would be installed
from the temp area (wherever it is) into the working copy.
So working files suddenly had 600 perms, whereas before they had whatever
the umask said. This did cause regression test failures.
I punted on fixing the callers at that point (way back at the subconf
2009 hackathon in Munich) and made the tempfile function take care of it.
This should be revisited to fix the tempfile leakage problem you
describe (which some people would consider a security bug...)
Received on 2011-06-22 10:26:00 CEST