Bug#344029: [EMAIL PROTECTED]: Bug#350954: DSA-960-1 security update breaks libmail-audit-perl when $ENV{HOME} is not set]
On Sat, Feb 04, 2006 at 02:59:25PM +0100, Martin Schulze wrote: Comments to the attached patch, which are least intrusive to the update we're already distributing? It's certainly the minimum required change. However, after this patch Mail::Audit is still leaving behind a file in /tmp every time it's used without $HOME, whether logging is enabled or not. And the documentation remains out of sync. (Naturally, it's your call to decide whether these should be fixed or not, but I just wanted to point them out.) FWIW, the patch in #350954 by Robert L Mathews addresses both of these issues. -- Niko Tyni [EMAIL PROTECTED] -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#344029: [EMAIL PROTECTED]: Bug#350954: DSA-960-1 security update breaks libmail-audit-perl when $ENV{HOME} is not set]
Niko Tyni wrote:
Hi security team,
I'm very sorry that you have to hear from me again :(
There's a regression in the patch for DSA-960-1, for both woody and sarge.
When $HOME is not set, Mail::Audit is now creating logfiles in cwd and
dying if it's not writable. This happens even if logging is turned off,
which makes the problem much more serious.
Doo, I have to agree that it is confusing to have tempdir() use different
parameters as tempfile(), but only partially.
I have not yet had a proper look at the proposed patches in #350954 and
the last message of #344029, but I wanted to make you aware of this.
Again, my apologies for the bad handling of this.
Comments to the attached patch, which are least intrusive to the
update we're already distributing?
Regards,
Joey
--
MIME - broken solution for a broken design. -- Ralf Baechle
Please always Cc to me when replying to me on the lists.
diff -u libmail-audit-perl-2.1/Audit.pm libmail-audit-perl-2.1/Audit.pm
--- libmail-audit-perl-2.1/Audit.pm
+++ libmail-audit-perl-2.1/Audit.pm
@@ -4,7 +4,13 @@
my $logging;
my $loglevel=3;
-my $logfile = /tmp/.getpwuid($).-audit.log;
+my $logfile;
+if (exists $ENV{HOME} and defined $ENV{HOME} and -d $ENV{HOME}) {
+ $logfile = $ENV{HOME}/.mail_audit.log;
+}
+else {
+ (undef,$logfile) = tempfile(mail_audit.log-X, DIR =
File::Spec-tmpdir);
+}
# --
# no user-modifiable parts below this line.
@@ -18,6 +24,8 @@
use vars qw($VERSION @ISA @EXPORT @EXPORT_OK $ASSUME_MSGPREFIX);
# @ISA will depend on whether the message is MIME; if it is, we'll be
MIME::Entity. if not, we'll be Mail::Internet.
use Fcntl ':flock';
+use File::Spec;
+use File::Temp qw(tempfile);
$ASSUME_MSGPREFIX = 0;
--- libmail-audit-perl-2.1.orig/Audit/MimeEntity.pm
+++ libmail-audit-perl-2.1/Audit/MimeEntity.pm
@@ -4,6 +4,7 @@
use strict;
use File::Path;
+use File::Temp qw(tempdir);
use MIME::Parser;
use MIME::Entity;
use Mail::Audit::MailInternet;
@@ -12,10 +13,12 @@
$VERSION = '2.0';
-$MIME_PARSER_TMPDIR = /tmp/.getpwuid($).-mailaudit;
-
my $parser = MIME::Parser-new();
+# Create a tempdir using File::Temp::tempdir, have it be destroyed at
+# END{} time.
+$MIME_PARSER_TMPDIR = tempdir(CLEANUP = 1);
+
my @to_rmdir;
sub autotype_new {
@@ -23,8 +26,6 @@
my $mailinternet = shift;
$parser-ignore_errors(1);
-mkdir ($MIME_PARSER_TMPDIR, 0777);
-if (! -d $MIME_PARSER_TMPDIR) { $MIME_PARSER_TMPDIR = /tmp }
$parser-output_under($MIME_PARSER_TMPDIR);
# todo: add eval error trapping. if there's a problem, return
Mail::Audit::MailInternet as a fallback.
diff -u libmail-audit-perl-2.1/Audit.pm libmail-audit-perl-2.1/Audit.pm
--- libmail-audit-perl-2.1/Audit.pm
+++ libmail-audit-perl-2.1/Audit.pm
@@ -6,10 +6,10 @@
my $loglevel=3;
my $logfile;
if (exists $ENV{HOME} and defined $ENV{HOME} and -d $ENV{HOME}) {
- $logfile = $ENV{HOME}/.mail_audit.log
+ $logfile = $ENV{HOME}/.mail_audit.log;
}
else {
- (undef,$logfile) = tempfile(mail_audit.log-X,TMPDIR=1);
+ (undef,$logfile) = tempfile(mail_audit.log-X, DIR =
File::Spec-tmpdir);
}
# --
@@ -24,6 +24,7 @@
use vars qw($VERSION @ISA @EXPORT @EXPORT_OK $ASSUME_MSGPREFIX);
# @ISA will depend on whether the message is MIME; if it is, we'll be
MIME::Entity. if not, we'll be Mail::Internet.
use Fcntl ':flock';
+use File::Spec;
use File::Temp qw(tempfile);
$ASSUME_MSGPREFIX = 0;
Bug#350954: DSA-960-1 security update breaks libmail-audit-perl when $ENV{HOME} is not set
Package: libmail-audit-perl
Version: 2.1-5sarge2
Tags: patch
The libmail-audit-perl 2.1-5sarge2 NMU update tries to fix the insecure
temporary file creation bug in Mail::Audit. However, the fix can cause
serious problems in cases where $ENV{HOME} is not set.
The problem is with this line at the top:
(undef,$logfile) = tempfile(mail_audit.log-X,TMPDIR=1);
The first problem with this is that TMPDIR=1 is not a valid option
for File::Temp's tempfile -- that option only works with tempdir(). So
that option is being ignored and the temporary file is being created in
the current working directory instead, which fails if the program
doesn't have write permission for that directory. This is easily
demonstrated as follows:
$ cd /
$ /usr/bin/env -i perl -e 'use Mail::Audit;'
Error in tempfile() using mail_audit.log-X: Parent directory (.)
is not writable at /usr/share/perl5/Mail/Audit.pm line 12
The correct usage to make tempfile() use a temporary directory would be
something like:
(undef,$logfile) =
tempfile(mail_audit.log-X, DIR = File::Spec-tmpdir);
The second problem with having the tempfile() line at the top of the
script when $ENV{HOME} is not present is that Mail::Audit now attempts
to create a temporary log file every time it is used, even if logging is
not enabled:
$ cd /tmp
$ /usr/bin/env -i perl -e 'use Mail::Audit;'
This creates an orphaned empty file in /tmp every time it's run.
The combination of these two problems is serious: in my case, a working
script that uses Mail::Audit and doesn't do any logging (which was
therefore not affected by the security bug in the first place) is now
trying, and failing, to create a temporary log file in the current
working directory every time the script is run, rendering the script
unusable.
A proposed solution is attached. This patch only calls tempfile() if
logging is actually used with a missing filename. And if logging is
used, it uses File::Spec-tmpdir to locate /tmp.
Although the code includes an extra use File::Spec for clarity, this
is not actually a new dependency, because the code already uses
File::Temp which depends on File::Spec anyway.
The patch also updates the documentation, which got out of sync when the
behavior changed from the security update.
Thanks!
--
Robert L Mathews, Tiger Technologieshttp://www.tigertech.net/
--- Audit.pm.orig 2006-02-01 10:09:05.0 -0800
+++ Audit.pm 2006-02-01 10:34:31.0 -0800
@@ -5,12 +5,6 @@
my $logging;
my $loglevel=3;
my $logfile;
-if (exists $ENV{HOME} and defined $ENV{HOME} and -d $ENV{HOME}) {
- $logfile = $ENV{HOME}/.mail_audit.log
-}
-else {
- (undef,$logfile) = tempfile(mail_audit.log-X,TMPDIR=1);
-}
# --
# no user-modifiable parts below this line.
@@ -25,6 +19,7 @@
# @ISA will depend on whether the message is MIME; if it is, we'll be MIME::Entity. if not, we'll be Mail::Internet.
use Fcntl ':flock';
use File::Temp qw(tempfile);
+use File::Spec;
$ASSUME_MSGPREFIX = 0;
@@ -124,8 +119,9 @@
You may also specify C log = $logfile to write a debugging log; you
can set the verbosity of the log with the Cloglevel key, on a scale of
1 to 4. If you specify a log level without a log file, logging will be
-written to F/tmp/you-audit.log where Fyou is replaced by your user
-name.
+written to F.mail_audit.log in your home directory if the HOME
+environment variable is set, or to F/tmp/mail_audit.log-X (where
+FX contains random characters) if not.
Usually, the delivery methods Caccept, Cpipe, and
Cresend are final; Mail::Audit will terminate when they
@@ -161,9 +157,19 @@
open LOG, /dev/null;
if (exists $opts{loglevel}) { $logging = 1; $loglevel = $opts{loglevel}; }
if (exists $opts{log}) { $logging = 1;$logfile = $opts{log}; }
-if ($logging) { open LOG, $logfile or open LOG, /dev/null;
- # this doesn't seem to propagate to the calling script. hmm.
- }
+if ($logging) {
+unless (defined $logfile) {
+if (exists $ENV{HOME} and defined $ENV{HOME} and -d $ENV{HOME}) {
+$logfile = $ENV{HOME}/.mail_audit.log
+}
+else {
+(undef,$logfile) =
+tempfile(mail_audit.log-X, DIR = File::Spec-tmpdir);
+}
+}
+open LOG, $logfile or open LOG, /dev/null;
+# this doesn't seem to propagate to the calling script. hmm.
+}
_log(1, -- new run at . localtime);
my $draft = Mail::Internet-new( exists $opts{data}? $opts{data} : \*STDIN, Modify=0 );
Bug#344029: [EMAIL PROTECTED]: Bug#350954: DSA-960-1 security update breaks libmail-audit-perl when $ENV{HOME} is not set]
Hi security team, I'm very sorry that you have to hear from me again :( There's a regression in the patch for DSA-960-1, for both woody and sarge. When $HOME is not set, Mail::Audit is now creating logfiles in cwd and dying if it's not writable. This happens even if logging is turned off, which makes the problem much more serious. I have not yet had a proper look at the proposed patches in #350954 and the last message of #344029, but I wanted to make you aware of this. Again, my apologies for the bad handling of this. -- Niko Tyni [EMAIL PROTECTED] -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
