Re: PATCH contrib/log (fixes taint problems)

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Kenneth,

Kenneth Dombrowski <kenneth@cs.columbia.edu> writes:

> Attached is a patch for contrib/log (as found in the cvs-1.12.13 source)
> 
> It is generated with `diff -c` (...I think that is all that is
> necessary?)

Well, it is okay. I personally like 'diff -u'
better, but a context diff is typically good
enough.

> It fixes the tainted data problems 
> 
> To overcome the taint checks, ENV{PATH} is set
> to /bin:/usr/bin:/usr/local/bin, so if a Mail is
> not found there, a symlink will be necessary

It would be better to consider the use of the
Mail::Sendmail perl package which has
theoretically been installed by the host
administrator to use a reasonable mail program or
SMTP directly.

> It also corrects the FIXME notes re: allowing
> multiple -f options (to support logging to
> multiple files

Reasonable.

> It also no longer requires a -f option (having
> to add -f /dev/null to send an email always
> bugged me)

Okay.

> The log message header is formatted a little
> differently,

This is a problem. I would much rather have two
separate patches. One to address the tainting
problem and another to consider the changes to the
format of the generated log message.

> and is now printed to STDOUT (without the status
> stuff) unless the new -q switch is present. I
> hope the format change won't break too many
> parsing scripts, but I always wanted to modify
> it to add module/server/cvsroot data

If you wanted to have a new switch to change to a
new format, that would be okay, but if we could
avoid keeping the tainting stuff separate from the
reformatting that would be best.

> There is now an example loginfo line showing how
> to both write to a log & send an email for each
> commit

Good.

> the log message is written to disk before
> attempting to open the pipe to the mail program
> (and written to STDOUT before trying to open the
> file on disk), so they are no longer
> inter-dependant

Okay.

> comments are welcome, please be aware I am not
> subscribed to this list
> 
> thanks, 
> Kenneth Dombrowski 

Terminating your lines with extra whitespace is
not desirable and in general reformatting changes
(adding extra '#' to blank lines) makes it harder
to see the security fixes in your patch.

Fwiw: I recommend against use of

chomp(my $server = `hostname`);

and in favor of the CPAN Sys::Hostname package.
Again, if the user installed it, then you don't
need to worry if there is or is not a 'hostname'
command on the system, or if you need to use
'uname -n' to get the same information as it
is handled by the perl package.

The idiom:

  my $stdin = join('', <STDIN>);

is probably not as effecient as

  my $slash = $/;
  undef $/;
  my $stdin = <STDIN>; # slurp all of STDIN
  $/ = $slash;

So, if you could apply your patch to the log.pl
rather than the generated copy of the 'log'
command, the patch might apply more cleanly.
(Hint: @PERL@ and @PACKAGE_BUGREPORT@ need to be
there instead of the expanded text '/usr/bin/perl'
and 'bug-cvs@nongnu.org' ...

Note also that the author's comments about hating
perl should probably be preserved.

        Thank you,
        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFE/3ZPCg7APGsDnFERApYyAJsFSJ1JqvmrOTEhxofNR+yn0SuFhQCeMBGO
EXlLFrKW3s596Q0q+tQ2sAo=
=4Le2
-----END PGP SIGNATURE-----