Avoid shell commandline processing in CA.pl

The CA.pl script used to build single-string string commandlines to pass
to a shell via `system(command_string)`.  That was fragile and not a best
practice.

This PR replaces `system(command_string)` with `system { executable } @argv`,
which avoids the shell whenever possible (at least Unix-like systems and
Windows).  The only question mark is whether some sort of quoting is
needed for VMS to preserve the case of commandline arguments even when
processes are spawned directly, rather than via the shell.

Unfortunately, given the way that some environment variables and
command-line options are used to construct the commands to run,
the result is still brittle.  The CA.pl utility really should
be replaced with something better.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27432)

diff --git a/apps/CA.pl.in b/apps/CA.pl.in
index 2fcc8268567fdf549aa703bb12546186b2755231..71b05f0078ce607579833e2c51de9c2b4bc86d11 100644 (file)
--- a/apps/CA.pl.in
+++ b/apps/CA.pl.in
@@ -19,14 +19,17 @@ my @OPENSSL_CMDS = ("req", "ca", "pkcs12", "x509", "verify");
 
 my $openssl = $ENV{'OPENSSL'} // "openssl";
 $ENV{'OPENSSL'} = $openssl;
+my @openssl = split(" ", $openssl);
+
 my $OPENSSL_CONFIG = $ENV{"OPENSSL_CONFIG"} // "";
+my @OPENSSL_CONFIG = split(" ", $OPENSSL_CONFIG);
 
 # Command invocations.
-my $REQ = "$openssl req $OPENSSL_CONFIG";
-my $CA = "$openssl ca $OPENSSL_CONFIG";
-my $VERIFY = "$openssl verify";
-my $X509 = "$openssl x509";
-my $PKCS12 = "$openssl pkcs12";
+my @REQ = (@openssl, "req", @OPENSSL_CONFIG);
+my @CA = (@openssl, "ca", @OPENSSL_CONFIG);
+my @VERIFY = (@openssl, "verify");
+my @X509 = (@openssl, "x509");
+my @PKCS12 = (@openssl, "pkcs12");
 
 # Default values for various configuration settings.
 my $CATOP = "./demoCA";
@@ -34,10 +37,10 @@ my $CAKEY = "cakey.pem";
 my $CAREQ = "careq.pem";
 my $CACERT = "cacert.pem";
 my $CACRL = "crl.pem";
-my $DAYS = "-days 365";
-my $CADAYS = "-days 1095";     # 3 years
-my $EXTENSIONS = "-extensions v3_ca";
-my $POLICY = "-policy policy_anything";
+my @DAYS = qw(-days 365);
+my @CADAYS = qw(-days 1095);   # 3 years
+my @EXTENSIONS = qw(-extensions v3_ca);
+my @POLICY = qw(-policy policy_anything);
 my $NEWKEY = "newkey.pem";
 my $NEWREQ = "newreq.pem";
 my $NEWCERT = "newcert.pem";
@@ -45,7 +48,7 @@ my $NEWP12 = "newcert.p12";
 
 # Commandline parsing
 my %EXTRA;
-my $WHAT = shift @ARGV || "";
+my $WHAT = shift @ARGV // "";
 @ARGV = parse_extra(@ARGV);
 my $RET = 0;
 
@@ -53,23 +56,23 @@ my $RET = 0;
 # |EXTRA{CMD}| with list of values.
 sub parse_extra
 {
+    my @args;
     foreach ( @OPENSSL_CMDS ) {
-        $EXTRA{$_} = '';
+        $EXTRA{$_} = [];
     }
-
-    my @result;
-    while ( scalar(@_) > 0 ) {
-        my $arg = shift;
-        if ( $arg !~ m/-extra-([a-z0-9]+)/ ) {
-            push @result, $arg;
+    while (@_) {
+        my $arg = shift(@_);
+        if ( $arg !~ m{^-extra-(\w+)$} ) {
+            push @args, $arg;
             next;
         }
-        $arg =~ s/-extra-//;
-        die("Unknown \"-${arg}-extra\" option, exiting")
-            unless scalar grep { $arg eq $_ } @OPENSSL_CMDS;
-        $EXTRA{$arg} .= " " . shift;
+        $arg = $1;
+        die "Unknown \"-extra-${arg}\" option, exiting\n"
+            unless grep { $arg eq $_ } @OPENSSL_CMDS;
+        # XXX no quoting of arguments with internal whitespace supported
+        push @{$EXTRA{$arg}}, split(" ", shift(@_));
     }
-    return @result;
+    return @args;
 }
 
 
@@ -112,9 +115,9 @@ sub copy_pemfile
 # Wrapper around system; useful for debugging.  Returns just the exit status
 sub run
 {
-    my $cmd = shift;
-    print "====\n$cmd\n" if $verbose;
-    my $status = system($cmd);
+    my ($cmd, @args) = @_;
+    print "====\n$cmd @args\n" if $verbose;
+    my $status = system {$cmd} $cmd, @args;
     print "==> $status\n====\n" if $verbose;
     return $status >> 8;
 }
@@ -133,17 +136,15 @@ EOF
 
 if ($WHAT eq '-newcert' ) {
     # create a certificate
-    $RET = run("$REQ -new -x509 -keyout $NEWKEY -out $NEWCERT $DAYS"
-            . " $EXTRA{req}");
+    $RET = run(@REQ, qw(-new -x509 -keyout), $NEWKEY, "-out", $NEWCERT, @DAYS, @{$EXTRA{req}});
     print "Cert is in $NEWCERT, private key is in $NEWKEY\n" if $RET == 0;
 } elsif ($WHAT eq '-precert' ) {
     # create a pre-certificate
-    $RET = run("$REQ -x509 -precert -keyout $NEWKEY -out $NEWCERT $DAYS"
-            . " $EXTRA{req}");
+    $RET = run(@REQ, qw(-x509 -precert -keyout), $NEWKEY, "-out", $NEWCERT, @DAYS, @{$EXTRA{req}});
     print "Pre-cert is in $NEWCERT, private key is in $NEWKEY\n" if $RET == 0;
 } elsif ($WHAT =~ /^\-newreq(\-nodes)?$/ ) {
     # create a certificate request
-    $RET = run("$REQ -new" . (defined $1 ? " $1" : "") . " -keyout $NEWKEY -out $NEWREQ $EXTRA{req}");
+    $RET = run(@REQ, "-new", (defined $1 ? ($1,) : ()), "-keyout", $NEWKEY, "-out", $NEWREQ, @{$EXTRA{req}});
     print "Request is in $NEWREQ, private key is in $NEWKEY\n" if $RET == 0;
 } elsif ($WHAT eq '-newca' ) {
     # create the directory hierarchy
@@ -176,48 +177,45 @@ if ($WHAT eq '-newcert' ) {
         copy_pemfile($FILE,"${CATOP}/$CACERT", "CERTIFICATE");
     } else {
         print "Making CA certificate ...\n";
-        $RET = run("$REQ -new -keyout ${CATOP}/private/$CAKEY"
-                . " -out ${CATOP}/$CAREQ $EXTRA{req}");
-        $RET = run("$CA -create_serial"
-                . " -out ${CATOP}/$CACERT $CADAYS -batch"
-                . " -keyfile ${CATOP}/private/$CAKEY -selfsign"
-                . " $EXTENSIONS"
-                . " -infiles ${CATOP}/$CAREQ $EXTRA{ca}") if $RET == 0;
+        $RET = run(@REQ, qw(-new -keyout), "${CATOP}/private/$CAKEY",
+                   "-out", "${CATOP}/$CAREQ", @{$EXTRA{req}});
+        $RET = run(@CA, qw(-create_serial -out), "${CATOP}/$CACERT", @CADAYS,
+                   qw(-batch -keyfile), "${CATOP}/private/$CAKEY", "-selfsign",
+                   @EXTENSIONS, "-infiles", "${CATOP}/$CAREQ", @{$EXTRA{ca}})
+            if $RET == 0;
         print "CA certificate is in ${CATOP}/$CACERT\n" if $RET == 0;
     }
 } elsif ($WHAT eq '-pkcs12' ) {
     my $cname = $ARGV[0];
     $cname = "My Certificate" unless defined $cname;
-    $RET = run("$PKCS12 -in $NEWCERT -inkey $NEWKEY"
-            . " -certfile ${CATOP}/$CACERT -out $NEWP12"
-            . " -export -name \"$cname\" $EXTRA{pkcs12}");
-    print "PKCS #12 file is in $NEWP12\n" if $RET == 0;
+    $RET = run(@PKCS12, "-in", $NEWCERT, "-inkey", $NEWKEY,
+               "-certfile", "${CATOP}/$CACERT", "-out", $NEWP12,
+               qw(-export -name), $cname, @{$EXTRA{pkcs12}});
+    print "PKCS#12 file is in $NEWP12\n" if $RET == 0;
 } elsif ($WHAT eq '-xsign' ) {
-    $RET = run("$CA $POLICY -infiles $NEWREQ $EXTRA{ca}");
+    $RET = run(@CA, @POLICY, "-infiles", $NEWREQ, @{$EXTRA{ca}});
 } elsif ($WHAT eq '-sign' ) {
-    $RET = run("$CA $POLICY -out $NEWCERT"
-            . " -infiles $NEWREQ $EXTRA{ca}");
+    $RET = run(@CA, @POLICY, "-out", $NEWCERT,
+               "-infiles", $NEWREQ, @{$EXTRA{ca}});
     print "Signed certificate is in $NEWCERT\n" if $RET == 0;
 } elsif ($WHAT eq '-signCA' ) {
-    $RET = run("$CA $POLICY -out $NEWCERT"
-            . " $EXTENSIONS -infiles $NEWREQ $EXTRA{ca}");
+    $RET = run(@CA, @POLICY, "-out", $NEWCERT, @EXTENSIONS,
+               "-infiles", $NEWREQ, @{$EXTRA{ca}});
     print "Signed CA certificate is in $NEWCERT\n" if $RET == 0;
 } elsif ($WHAT eq '-signcert' ) {
-    $RET = run("$X509 -x509toreq -in $NEWREQ -signkey $NEWREQ"
-            . " -out tmp.pem $EXTRA{x509}");
-    $RET = run("$CA $POLICY -out $NEWCERT"
-            .  "-infiles tmp.pem $EXTRA{ca}") if $RET == 0;
+    $RET = run(@X509, qw(-x509toreq -in), $NEWREQ, "-signkey", $NEWREQ,
+               qw(-out tmp.pem), @{$EXTRA{x509}});
+    $RET = run(@CA, @POLICY, "-out", $NEWCERT,
+               qw(-infiles tmp.pem), @{$EXTRA{ca}}) if $RET == 0;
     print "Signed certificate is in $NEWCERT\n" if $RET == 0;
 } elsif ($WHAT eq '-verify' ) {
     my @files = @ARGV ? @ARGV : ( $NEWCERT );
     foreach my $file (@files) {
-        # -CAfile quoted for VMS, since the C RTL downcases all unquoted
-        # arguments to C programs
-        my $status = run("$VERIFY \"-CAfile\" ${CATOP}/$CACERT $file $EXTRA{verify}");
+        my $status = run(@VERIFY, "-CAfile", "${CATOP}/$CACERT", $file, @{$EXTRA{verify}});
         $RET = $status if $status != 0;
     }
 } elsif ($WHAT eq '-crl' ) {
-    $RET = run("$CA -gencrl -out ${CATOP}/crl/$CACRL $EXTRA{ca}");
+    $RET = run(@CA, qw(-gencrl -out), "${CATOP}/crl/$CACRL", @{$EXTRA{ca}});
     print "Generated CRL is in ${CATOP}/crl/$CACRL\n" if $RET == 0;
 } elsif ($WHAT eq '-revoke' ) {
     my $cname = $ARGV[0];
@@ -225,10 +223,10 @@ if ($WHAT eq '-newcert' ) {
         print "Certificate filename is required; reason optional.\n";
         exit 1;
     }
-    my $reason = $ARGV[1];
-    $reason = " -crl_reason $reason"
-        if defined $reason && crl_reason_ok($reason);
-    $RET = run("$CA -revoke \"$cname\"" . $reason . $EXTRA{ca});
+    my @reason;
+    @reason = ("-crl_reason", $ARGV[1])
+        if defined $ARGV[1] && crl_reason_ok($ARGV[1]);
+    $RET = run(@CA, "-revoke", $cname, @reason, @{$EXTRA{ca}});
 } else {
     print STDERR "Unknown arg \"$WHAT\"\n";
     print STDERR "Use -help for help.\n";

diff --git a/test/recipes/80-test_ca.t b/test/recipes/80-test_ca.t
index 916f952a0c3e15abd7d2f6f41aaa5dad1c6c544e..a4be5ff6bf515f7b63990b24f9b1f104ab318a30 100644 (file)
--- a/test/recipes/80-test_ca.t
+++ b/test/recipes/80-test_ca.t
@@ -21,9 +21,7 @@ setup("test_ca");
 $ENV{OPENSSL} = cmdstr(app(["openssl"]), display => 1);
 
 my $cnf = srctop_file("test","ca-and-certs.cnf");
-my $std_openssl_cnf = '"'
-    . srctop_file("apps", $^O eq "VMS" ? "openssl-vms.cnf" : "openssl.cnf")
-    . '"';
+my $std_openssl_cnf = srctop_file("apps", $^O eq "VMS" ? "openssl-vms.cnf" : "openssl.cnf");
 
 sub src_file {
     return srctop_file("test", "certs", shift);
@@ -37,19 +35,19 @@ require_ok(srctop_file("test", "recipes", "tconversion.pl"));
 
  SKIP: {
      my $cakey = src_file("ca-key.pem");
-     $ENV{OPENSSL_CONFIG} = qq(-config "$cnf");
+     $ENV{OPENSSL_CONFIG} = qq(-config $cnf);
      skip "failed creating CA structure", 4
          if !ok(run(perlapp(["CA.pl","-newca",
                              "-extra-req", "-key $cakey"], stdin => undef)),
                 'creating CA structure');
 
      my $eekey = src_file("ee-key.pem");
-     $ENV{OPENSSL_CONFIG} = qq(-config "$cnf");
+     $ENV{OPENSSL_CONFIG} = qq(-config $cnf);
      skip "failed creating new certificate request", 3
          if !ok(run(perlapp(["CA.pl","-newreq",
                              '-extra-req', "-outform DER -section userreq -key $eekey"])),
                 'creating certificate request');
-     $ENV{OPENSSL_CONFIG} = qq(-rand_serial -inform DER -config "$std_openssl_cnf");
+     $ENV{OPENSSL_CONFIG} = qq(-rand_serial -inform DER -config $std_openssl_cnf);
      skip "failed to sign certificate request", 2
          if !is(yes(cmdstr(perlapp(["CA.pl", "-sign"]))), 0,
                 'signing certificate request');
@@ -61,7 +59,7 @@ require_ok(srctop_file("test", "recipes", "tconversion.pl"));
          if disabled("ct");
 
      my $eekey2 = src_file("ee-key-3072.pem");
-     $ENV{OPENSSL_CONFIG} = qq(-config "$cnf");
+     $ENV{OPENSSL_CONFIG} = qq(-config $cnf);
      ok(run(perlapp(["CA.pl", "-precert", '-extra-req', "-section userreq -key $eekey2"], stderr => undef)),
         'creating new pre-certificate');
 }