Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 22/07/2014 09:59, IKEDA Soji a écrit :
> Guillaume,
>
> On Tue, 22 Jul 2014 09:28:02 +0200
> Guillaume Rousse <address@concealed> wrote:
>
> > Le 22/07/2014 03:58, IKEDA Soji a écrit :
> > > Hi,
> > >
> > > I prefer to on-memory processing.  I suppose secure information such
> > > as private keys would not be unneccessarily written into storage if
> > > at all possible --- even if they are removed soon, random blocks of
> > > disk will keep information --- let alone passphrases.
> > If you're able to access the content of a file owned by Sympa uid, with
> > owner-only read permission, you may as well read sympa configuration
> > file directly, where this password is also available. So why bother with
> > a FIFO here ?
>
> For example, imagine a user replaced her Sympa server and want to
> dispose older machine including disks.
>
> Passphrases are stored into specific volume as config parameters.
> So this volume should be paid special attention.  However, if
> passphrase had repeatedly been saved into other volumes, they should
> be treated as carefully as former should be.
That's an hypotetical scenario.

What's not hypotetical, hovewer, is that using FIFOs instead of plain 
old files makes the code slightly more complex (see attached patch, 
restoring old behaviour). And even worse, when computing temporary file 
name before creating them, you're also facing a whole set of security 
issues, generally refered as 'time-to-check-vs-time-to-use' (see various 
warnings in File::Temp manual). The usual way to avoid those problems is 
to use atomic operations (create file name and open file handle at 
once), which is unfortunatly not possible with FIFOs.

Basically, trading disk writes for race conditions doesn't constitute a 
proven security improvement, just a change between two different 
situations impossible to compare. All things equal, I prefer the 
simplest implementation. Especially for such kind of corner cases as 
password-protected secrets requiring to store password in clear-text 
somewhere else...

> FIFO (I don't stick to this method, though) does not store
> passed information to any perpetual storages.  I suppose that is why
> openssl supports FIFO.
openssl doesn't make any difference between a regular file and a fifo 
for this matter, but this doesn't change much here.
--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31
Index: src/lib/Sympa/Message.pm
===================================================================
--- src/lib/Sympa/Message.pm	(révision 11254)
+++ src/lib/Sympa/Message.pm	(copie de travail)
@@ -48,6 +48,7 @@
 use Carp qw(croak);
 use English qw(-no_match_vars);
 
+use File::Temp qw(tempfile);
 use HTML::Entities qw(encode_entities);
 use Mail::Address;
 use MIME::Charset;
@@ -698,18 +699,19 @@
         return undef;
     }
 
-    my $password_file;
+    my $password_fifo;
     if ($key_password) {
-        my $umask = umask;
-        umask 0077;
-        $password_file = File::Temp->new(
-            DIR    => $tmpdir,
-            UNLINK => $main::options{'debug'} ? 0 : 1
+        (undef, $password_fifo) = tempfile(
+            DIR  => $tmpdir,
+            OPEN => 0
         );
-
-        print $password_file $key_password;
-        close $password_file;
-        umask $umask;
+        if (!POSIX::mkfifo($password_fifo, 0600)) {
+           $main::logger->do_log(
+               Sympa::Logger::ERR,
+               'Unable to create fifo %s: %s', $password_fifo, $ERRNO
+           );
+           return undef;
+        }
     }
 
     ## try all keys/certs until one decrypts.
@@ -727,7 +729,7 @@
         my $command =
             "$openssl smime -decrypt -out $decrypted_message_file" . 
             " -recip $certfile -inkey $keyfile" .
-            ($password_file ? " -passin file:$password_file" : "" );
+            ($key_password ? " -passin file:$password_fifo" : "" );
         $main::logger->do_log(Sympa::Logger::DEBUG3, '%s', $command);
 
         my $command_handle;
@@ -740,6 +742,19 @@
             return undef;
         }
 
+        if ($key_password) {
+            my $password_fifo_handle;
+            if (!open($password_fifo_handle, '>', $password_fifo)) {
+               $main::logger->do_log(
+                   Sympa::Logger::ERR,
+                   'Unable to open fifo %s: %s', $password_fifo, $ERRNO
+               );
+               return undef;
+            }
+            print $password_fifo_handle $key_password;
+            close $password_fifo_handle;
+        }
+
         $self->{'entity'}->print($command_handle);
         close $command_handle;
 
@@ -761,6 +776,10 @@
         }
     }
 
+    if ($key_password) {
+        unlink $password_fifo unless $main::options{'debug'};
+    }
+
     unless ($decrypted_entity) {
         $main::logger->do_log(Sympa::Logger::ERR, 'Message could not be decrypted');
         return undef;
@@ -1404,18 +1423,19 @@
             unless $header =~ /^(content-type|content-transfer-encoding)$/i
     }
 
-    my $password_file;
+    my $password_fifo;
     if ($key_password) {
-        my $umask = umask;
-        umask 0077;
-        $password_file = File::Temp->new(
-            DIR    => $tmpdir,
-            UNLINK => $main::options{'debug'} ? 0 : 1
+        (undef, $password_fifo) = tempfile(
+            DIR  => $tmpdir,
+            OPEN => 0
         );
-
-        print $password_file $key_password;
-        close $password_file;
-        umask $umask;
+        if (!POSIX::mkfifo($password_fifo, 0600)) {
+           $main::logger->do_log(
+               Sympa::Logger::ERR,
+               'Unable to create fifo %s: %s', $password_fifo, $ERRNO
+           );
+           return undef;
+        }
     }
 
     my $signed_message_file = File::Temp->new(
@@ -1425,7 +1445,7 @@
 
     my $command = "$openssl smime -sign"                             .
         " -signer $cert -inkey $key " .  "-out $signed_message_file" .
-        ($password_file ? " -passin file:$password_file" : "" );
+        ($key_password ? " -passin file:$password_fifo" : "" );
     $main::logger->do_log(Sympa::Logger::DEBUG2, '%s', $command);
 
     my $command_handle;
@@ -1437,9 +1457,27 @@
         );
         return undef;
     }
+
+    if ($key_password) {
+        my $password_fifo_handle;
+        if (!open($password_fifo_handle, '>', $password_fifo)) {
+           $main::logger->do_log(
+               Sympa::Logger::ERR,
+               'Unable to open fifo %s: %s', $password_fifo, $ERRNO
+           );
+           return undef;
+        }
+        print $password_fifo_handle $key_password;
+        close $password_fifo_handle;
+    }
+
     $entity->print($command_handle);
     close $command_handle;
 
+    if ($key_password) {
+        unlink $password_fifo unless $main::options{'debug'};
+    }
+
     my $parser = MIME::Parser->new();
     $parser->output_to_core(1);
     my $signed_entity = $parser->read($signed_message_file);

Attachment: smime.p7s
Description: Signature cryptographique S/MIME