From b25f51a5dc3831f9358637e4372bf76b76d4f864 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Fri, 18 Apr 2025 14:18:09 +1000 Subject: [PATCH] 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 Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/27432) (cherry picked from commit 0b1bdef38ef1e3369a7bcde1b9a6eabe44b10e54) --- apps/CA.pl.in | 114 +++++++++++++++++++------------------- test/recipes/80-test_ca.t | 12 ++-- 2 files changed, 61 insertions(+), 65 deletions(-) diff --git a/apps/CA.pl.in b/apps/CA.pl.in index 2fcc8268567..71b05f0078c 100644 --- 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 916f952a0c3..a4be5ff6bf5 100644 --- 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'); } -- 2.47.2