[rt-devel] Better handling of sendmail (Re: Bugfix for security patch on mod_perl)
paul.szabo at sydney.edu.au
paul.szabo at sydney.edu.au
Sat Jun 2 01:13:13 EDT 2012
Dear Alex,
>>> Using IPC::Open2, the child exit status is available in $?, precisely
>>> the same as when using ``. I am not aware of any failure modes
>>> involving loss of output ...
>>
>> If the eval dies with $SIG{PIPE} then it does not examine $?.
>
> If the eval dies with SIGPIPE, $? may not be valid. $? is only set by
> wait() or system(), neither of which are guaranteed to have happened if
> we receive SIGPIPE. See also the IPC::Open2 documentation which notes
> that exec failures are simply reported by way of SIGPIPE -- while this
> is non-optimal, it will be remedied in 4.0-trunk by use of IPC::Run3.
After a SIGPIPE you should use waitpid() to avoid zombies, and to get a
correct $?.
My current patches/comments below.
Cheers, Paul
Paul Szabo psz at maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/
School of Mathematics and Statistics University of Sydney Australia
$ diff -u /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm-DSA-2480-2 /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm
--- /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm-DSA-2480-2 2012-05-27 22:29:32.000000000 +1000
+++ /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm 2012-06-02 14:12:18.000000000 +1000
@@ -443,36 +443,111 @@
}
eval {
- # don't ignore CHLD signal to get proper exit code
- local $SIG{'CHLD'} = 'DEFAULT';
-
- # if something wrong with $mail->print we will get PIPE signal, handle it
- local $SIG{'PIPE'} = sub { die "program unexpectedly closed pipe" };
-
- # Make it look to open2 like STDIN is on FD 0, like it
- # should be; this is necessary because under mod_perl with
- # the perl-script handler, it's not. This causes our
- # child's "STDIN" (FD 10-ish) to be set to the pipe we want,
- # but FD 0 (which the exec'd sendmail assumes is STDIN) is
- # still open to /dev/null; this ends disasterously.
- local *STDIN = IO::Handle->new_from_fd( 0, "r" );
-
- require IPC::Open2;
- my ($mail, $stdout);
- my $pid = IPC::Open2::open2( $stdout, $mail, $path, @args )
- or die "couldn't execute program: $!";
-
- $args{'Entity'}->print($mail);
- close $mail or die "close pipe failed: $!";
-
- waitpid($pid, 0);
- if ($?) {
- # sendmail exit statuses mostly errors with data not software
- # TODO: status parsing: core dump, exit on signal or EX_*
- my $msg = "$msgid: `$path @args` exited with code ". ($?>>8);
- $msg = ", interrupted by signal ". ($?&127) if $?&127;
- $RT::Logger->error( $msg );
+ ##########
+ # Comments/patches 2 Jun 12 Paul Szabo psz at maths.usyd.edu.au
+ # Not claiming this is right or best: but "it works for me".
+ #####
+ # Dangerous to use IPC::Open2 or maybe IPC::Run3 (or maybe
+ # IPC::Run::SafeHandles): e.g. when we forget about those
+ # handles and to test patches... Seems wasteful also.
+ # I am puzzling about the
+ # local *STDIN = IO::Handle->new_from_fd( 0, "r" );
+ # patch: though STDIN is changed only locally, the fd stays
+ # open forever, and a rather similar
+ # local *STDOUT = IO::Handle->new_from_fd( 1, "w" );
+ # would cause many problems like
+ # RT: DBD::Pg::st execute failed: SSL SYSCALL error: Bad file descriptor at /usr/share/perl5/DBIx/SearchBuilder/Handle.pm line 509. (/usr/share/perl5/DBIx/SearchBuilder/Handle.pm:509)
+ # later. Curiously, STDERR seems "plain".
+ #####
+ # Keep STDOUT and STDERR: sendmail is often silent, but might
+ # say "No recipient addresses found in header" on STDOUT.
+ # Beware of pipes: if we die with $SIG{PIPE} then might never
+ # do waitpid and/or never examine $? for exit status.
+ # Sendmail seems to slurp in all its input and only then
+ # look at headers; future versions might examine headers
+ # on-the-fly and quit e.g. with "illegal address" with input
+ # left unread.
+ #####
+ ## don't ignore CHLD signal to get proper exit code
+ #local $SIG{'CHLD'} = 'DEFAULT';
+ #
+ ## if something wrong with $mail->print we will get PIPE signal, handle it
+ #local $SIG{'PIPE'} = sub { die "program unexpectedly closed pipe" };
+ #
+ ## Make it look to open2 like STDIN is on FD 0, like it
+ ## should be; this is necessary because under mod_perl with
+ ## the perl-script handler, it's not. This causes our
+ ## child's "STDIN" (FD 10-ish) to be set to the pipe we want,
+ ## but FD 0 (which the exec'd sendmail assumes is STDIN) is
+ ## still open to /dev/null; this ends disasterously.
+ #local *STDIN = IO::Handle->new_from_fd( 0, "r" );
+ #
+ #require IPC::Open2;
+ #my ($mail, $stdout);
+ #my $pid = IPC::Open2::open2( $stdout, $mail, $path, @args )
+ # or die "couldn't execute program: $!";
+ #
+ #$args{'Entity'}->print($mail);
+ #close $mail or die "close pipe failed: $!";
+ #
+ #waitpid($pid, 0);
+ #if ($?) {
+ # # sendmail exit statuses mostly errors with data not software
+ # # TODO: status parsing: core dump, exit on signal or EX_*
+ # my $msg = "$msgid: `$path @args` exited with code ". ($?>>8);
+ # $msg = ", interrupted by signal ". ($?&127) if $?&127;
+ # $RT::Logger->error( $msg );
+ #}
+ #####
+ #use File::Temp; # Done above
+ my $tmphdl = File::Temp->new() or die "Cannot create temp file for sendmail data\n";
+ my $tmpnam = $tmphdl->filename or die "Nameless temp file for sendmail data\n";
+ $args{'Entity'}->print($tmphdl) or die "Cannot write temp file for sendmail data\n";
+ close ($tmphdl) or die "Cannot close temp file for sendmail data\n";
+ # Tempting to use the simple:
+ #my $cmd = "$path @args < $tmpnam 2>&1";
+ #my $msg = `$cmd`;
+ # but that would be unsafe: though "$path @args" is mostly just
+ # "sendmail -oi -t", it might contain user-supplied email
+ # addresses, so nasty shell metacharacters.
+ # Might try to "fix" like:
+ #my $cmd; $cmd .= "\Q$_\E " foreach ($path, at args); $cmd .= "< $tmpnam 2>&1";
+ # but some args may contain blanks which should not be quoted.
+ #$RT::Logger->info( "PSz using command: $cmd" );
+ # Use a "safe version", somewhat following "man perlsec".
+ my $pid;
+ die "Cannot fork sendmail: $!\n" unless defined($pid = open(KID, "-|"));
+ if ($pid) { # parent
+ # Do nothing here, do later
+ } else { # child process
+ # Within eval so cannot use die(). Note: even exit is dodgy.
+ # Our STDIN etc are "funny": use POSIX fds instead,
+ # which are more direct anyway.
+ use POSIX;
+ my $fd = POSIX::open("$tmpnam");
+ (defined($fd) and $fd>=0) or warn("Cannot read temp file of sendmail data: $!"), POSIX::_exit(1);
+ # Right STDIN if needed
+ $fd == 0 or POSIX::dup2($fd,0) or warn("Cannot dup2($fd,0) for sendmail: $!"), POSIX::_exit(2);
+ # Right STDERR, our STDOUT should go to parent
+ POSIX::dup2(1,2) or warn("Cannot dup2(1,2) for sendmail: $!"), POSIX::_exit(3);
+ ## Sanitize environment
+ #delete @ENV{keys %ENV};
+ #$ENV{PATH} = "/bin:/usr/bin:/sbin:/usr/sbin"; # Minimal PATH including /usr/sbin for sendmail
+ #$ENV{SHELL} = '/bin/sh';
+ exec($path, @args) or warn("Cannot exec sendmail: $!\n"), POSIX::_exit(4);
+ warn("Should be dead after exec sendmail\n"), POSIX::_exit(5);
+ }
+ # do something
+ my $msg;
+ while (<KID>) {
+ $msg or $msg = "sendmail output: ";
+ $msg .= $_;
}
+ close KID or $msg .= "sendmail failed: $!";
+ $? and $msg .= "sendmail status: $? (exit code " . ($?>>8) . ", signal " . ($?&127) .")";
+ $msg and die "sendmail output: $msg\n";
+ unlink ($tmpnam) or die "Cannot delete temp file for sendmail data\n";
+ ##########
};
if ( $@ ) {
$RT::Logger->crit( "$msgid: Could not send mail with command `$path @args`: " . $@ );
More information about the rt-devel
mailing list