Re: cvs: temporary file handling fixes

[Solar Designer]

Hi Derek and all,

Please excuse the lengthy quoting, but I feel that at least some
vendor-sec members will appreciate full context this one time.

On Fri, May 23, 2003 at 10:57:15AM -0400, Derek Robert Price wrote:
> Solar Designer wrote:
> >We've recently officially added CVS 1.11.5 to Openwall GNU/*/Linux.
> >One of the things this required is a review of CVS code for possible
> >unsafe temporary file handling and making the corresponding fixes.
> >Our patches are available via:
> >
> >cvs -d :pserver:anoncvs:anoncvs@anoncvs.owl.openwall.com:/cvs co 
> >Owl/packages/cvs
> >
> >Of course, the worst were the scripts under contrib/ and our fixes to
> >them require Todd Miller's mktemp (1.3.1 or newer) most of the time
> >(but not always, so that some may be applied to the official CVS even
> >if requiring mktemp is decided to be unacceptable).  There're
> >intentionally no fallbacks to not be fail-open.
> >
> >Also note the patch which makes CVS use vitmp, our wrapper around
> >the VIM editor.  Without it, the uses of vi by CVS are unsafe at
> >least with VIM 6.1.386 (the VIM 6.1 patchlevel we're at currently)
> >when $TMPDIR is set to a directory that VIM doesn't recognize as a
> >temporary file one.  It may be easily seen with strace how, without
> >vitmp, a temporary file is unlinked and then re-created under the
> >same name and without the use of O_EXCL.  vitmp is in the public
> >domain and may be had via:
> >
> >cvs -d :pserver:anoncvs:anoncvs@anoncvs.owl.openwall.com:/cvs co 
> >Owl/packages/vim/
> 
> At first glance, these fixes are mostly either misguided or already 
> incorporated in, at least, the 1.12 feature branch.

It is nice that some of these issues are now dealt with in the just
released CVS 1.11.6 and 1.12.1, -- thank you!

However, looking at 1.12.1, I notice that the only two scripts which
will now use mktemp (if enabled at configure time) are cvsbug and
rcs2log, and the uses by cvsbug are buggy in that the file name in
$TEMP will be re-used multiple times.  Yes, Red Hat has this bug in
their patch too.

I don't understand why you consider our fixing the other scripts in
contrib/ and the documentation misguided.

> The fixes that 
> might be usable are going to need at least ChangeLog entries to 
> accompany them,

Obviously, but:

- it doesn't make sense to write full ChangeLog entries before we know
the fixes are even getting in (and I don't expect you to include them
without any changes at all);

- CVS is just one of over 120 packages in Owl and we're primarily
concerned with making our distribution better; we also like to share
our changes with upstream maintainers, but we can't afford to spend
much extra time on the integration of our changes upstream.

> some may need more documentation or tests in sanity.sh, 
> and all will need to have their purposes explained more fully to be 
> accepted.  Please see the HACKING file in the top level of the CVS 
> source distribution for more on how to submit patches.  Please note in 
> particular that they should be sent to the <bug-cvs@gnu.org> mailing 
> list and not directly to me.

This all is fine with me (although I won't necessarily have the time
to submit any of this officially), but it doesn't make a valid
procedure for reporting security problems and proposing fixes to them.
In particular, I was looking for a (security) bug reporting address
that wouldn't automatically reach a public mailing list, -- but it
seems you find unsafe temporary file handling to be a minor enough
issue to be discussed in public.  This is OK with me, but I thought
that some vendor-sec members could prefer to handle it differently.

-- 
Alexander Peslyak <solar@openwall.com>
GPG key ID: B35D3598  fp: 6429 0D7E F130 C13E C929  6447 73C3 A290 B35D 3598
http://www.openwall.com - bringing security into open computing environments