Better handling of sendmail (Re: Bugfix for security patch on mod_perl)

paul.szabo · May 29, 2012, 7:46pm

I feel that the invocation of sendmail in RT/Interface/Email.pm is
“wrong”:

uses IPC::Open2 instead of plain open($mail,“| $path @args”) though
it never attempts to read STDOUT, loses STDERR without Open3
uses pipe which can have consequences for invoker, including losing
the exit status or output of subprocess
I propose a more standard, simpler, handling of the sendmail process,
in a way which captures the exit status and all the STDERR or STDOUT
output.
Patch below is against the Debian 3.8 version (I believe it would go
cleanly into 4.0 also). Please see
http://bugs.debian.org/674865
also.

Cheers, Paul

Paul Szabo psz@maths.usyd.edu.au Paul Szabo, Sydney Mathematics & Statistics
School of Mathematics and Statistics University of Sydney Australia

diff -u /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm.bak /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm

— /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm.bak 2012-05-19 22:37:07.000000000 +1000
+++ /usr/share/request-tracker3.8/lib/RT/Interface/Email.pm 2012-05-29 06:40:37.000000000 +1000
@@ -443,28 +443,40 @@
}

     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" };

```
       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 );
```
```
       }
```

       ## 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" };

```
       #
```
```
       #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 $tmp_fh = File::Temp->new() or die "Cannot create temp file for sendmail data\n";

       my $tmp_fn = $tmp_fh->filename or die "Nameless temp file for sendmail data\n";

       $args{'Entity'}->print($tmp_fh) or die "Cannot write temp file for sendmail data\n";

       close ($tmp_fh) or die "Cannot close temp file for sendmail data\n";

       my $cmd = "$path @args < $tmp_fn 2>&1";

       #$RT::Logger->info( "PSz using command: $cmd" );

```
       my $msg = `$cmd`;
```

       $? and $msg .= "sendmail status: $? (exit code " . ($?>>8) . ", signal " . ($?&127) .")";

       $msg and die "sendmail output: $msg\n";

       unlink ($tmp_fn) or die "Cannot delete temp file for sendmail data\n";
   };
   if ( $@ ) {
       $RT::Logger->crit( "$msgid: Could not send mail with command `$path @args`: " . $@ );

Alex_Vandiver · May 29, 2012, 8:17pm

I feel that the invocation of sendmail in RT/Interface/Email.pm is
“wrong”:

uses IPC::Open2 instead of plain open($mail,“| $path @args”) though
it never attempts to read STDOUT, loses STDERR without Open3

Using either the two-argument form of open, or ``, makes the call
vulnerable to shell injection in @args – which is precisely the
vulnerability that this change is meant to protect against.
Additionally, STDOUT is intentionally not read from, for parity with the

/dev/null in the previous version of the code. Finally, STDERR is
connected to the parent process’s STDERR, which is not lost, but rather
goes directly into the Apache error log in all suggested deployment
options.

In a future version, we intend to move to using the more robust
IPC::Run3, when we will likely start explicitly logging STDOUT and
STDERR errors using RT’s logging infrastructure; this was not a valid
option as part of the security patches, as it would have added an
additional dependency in the 3.8 series.

uses pipe which can have consequences for invoker, including losing
the exit status or output of subprocess

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 – not that sendmail, exim, postfix, or qmail
has ever been reported to produce any to STDOUT, or that any version of
the code has required examining it.

I propose a more standard, simpler, handling of the sendmail process,
in a way which captures the exit status and all the STDERR or STDOUT
output.

Using this patch makes you vulnerable to CVE-2011-4458, remote execution
of code. Please read the history of the codepath in question, available
in git, for a complete exploration of why the techniques you are
proposing are either incorrect, unnecessary, or have security
implications.

Alex

paul.szabo · May 29, 2012, 10:19pm

Dear Alex,

I feel that the invocation of sendmail in RT/Interface/Email.pm is
“wrong”:

uses IPC::Open2 instead of plain open($mail,“| $path @args”) though
it never attempts to read STDOUT, loses STDERR without Open3

Using either the two-argument form of open, or ``, makes the call
vulnerable to shell injection in @args – which is precisely the
vulnerability that this change is meant to protect against.

Sorry I missed the reason for the change: I never seen, never looked
for, any history of the code (no comments there to warn of dangers).

In a future version, we intend to move to using the more robust
IPC::Run3, when we will likely start explicitly logging STDOUT and
STDERR errors using RT’s logging infrastructure …

Thanks, this seems under control, then.

uses pipe which can have consequences for invoker, including losing
the exit status or output of subprocess

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

Please let me know if you would want me to submit, or if you were
willing to consider, alternative patches avoiding shell metacharacter
issues in $cmd.

Cheers, Paul

Paul Szabo psz@maths.usyd.edu.au Paul Szabo, Sydney Mathematics & Statistics
School of Mathematics and Statistics University of Sydney Australia

Alex_Vandiver · May 30, 2012, 6:48pm

Sorry I missed the reason for the change: I never seen, never looked
for, any history of the code (no comments there to warn of dangers).

Apologies if I was overly grumpy; but I consider the perils of two-arg
open, system, and backticks with unchecked user-supplied arguments to be
generally well-known. I’m happy to accept patches to the comments in
the area which you think would clarify the issue.

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.

Please let me know if you would want me to submit, or if you were
willing to consider, alternative patches avoiding shell metacharacter
issues in $cmd.

Using $cmd is fundamentally insecure, as it brings the shell into the
mix; attempting to escape shell metacharacters to stay one step ahead of
the game is pointless. I see no compelling reason to move away from
tools that were designed to solve this problem securely, to ones which
require jumping through more hoops.

Alex

paul.szabo · June 2, 2012, 5:13am

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@maths.usyd.edu.au Paul Szabo, Sydney Mathematics & Statistics
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@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,@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`: " . $@ );