security/acme-client: do not depend on legacy code, as discussed in #2028

This commit is contained in:
Frank Wall
2020-10-04 22:59:10 +02:00
parent 3284a2b895
commit 66d51168cf
4 changed files with 55 additions and 34 deletions
@@ -57,7 +57,7 @@ class LeAccount extends LeCommon
$this->setEnvironment();
// Store acme filenames
$this->acme_args[] = exec_safe('--home %s', self::ACME_HOME_DIR);
$this->acme_args[] = LeUtils::execSafe('--home %s', self::ACME_HOME_DIR);
}
/**
@@ -118,8 +118,8 @@ class LeAccount extends LeCommon
$acmecmd = '/usr/local/sbin/acme.sh '
. '--createAccountKey '
. implode(' ', $this->acme_args) . ' '
. exec_safe('--accountkeylength %s', self::ACME_ACCOUNT_KEY_LENGTH) . ' '
. exec_safe('--accountconf %s', $account_conf_file);
. LeUtils::execSafe('--accountkeylength %s', self::ACME_ACCOUNT_KEY_LENGTH) . ' '
. LeUtils::execSafe('--accountconf %s', $account_conf_file);
LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug);
$proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env);
@@ -226,7 +226,7 @@ class LeAccount extends LeCommon
$acmecmd = '/usr/local/sbin/acme.sh '
. '--registeraccount '
. implode(' ', $this->acme_args) . ' '
. exec_safe('--accountconf %s', $this->account_conf_file);
. LeUtils::execSafe('--accountconf %s', $this->account_conf_file);
LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug);
$proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env);
@@ -30,7 +30,6 @@ namespace OPNsense\AcmeClient;
// Load legacy functions
require_once("certs.inc"); // used in import()
require_once("util.inc"); // for exec_safe()
use OPNsense\Core\Config;
use OPNsense\AcmeClient\LeAccount;
@@ -88,11 +87,11 @@ class LeCertificate extends LeCommon
$this->cert_fullchain_file = (string)sprintf(self::ACME_FULLCHAIN_FILE, $this->config->id);
// Store acme filenames
$this->acme_args[] = exec_safe('--home %s', self::ACME_HOME_DIR);
$this->acme_args[] = exec_safe('--certpath %s', $this->cert_file);
$this->acme_args[] = exec_safe('--keypath %s', $this->cert_key_file);
$this->acme_args[] = exec_safe('--capath %s', $this->cert_chain_file);
$this->acme_args[] = exec_safe('--fullchainpath %s', $this->cert_fullchain_file);
$this->acme_args[] = LeUtils::execSafe('--home %s', self::ACME_HOME_DIR);
$this->acme_args[] = LeUtils::execSafe('--certpath %s', $this->cert_file);
$this->acme_args[] = LeUtils::execSafe('--keypath %s', $this->cert_key_file);
$this->acme_args[] = LeUtils::execSafe('--capath %s', $this->cert_chain_file);
$this->acme_args[] = LeUtils::execSafe('--fullchainpath %s', $this->cert_fullchain_file);
}
/**
@@ -437,7 +436,7 @@ class LeCertificate extends LeCommon
$acmecmd = '/usr/local/sbin/acme.sh '
. '--remove '
. implode(' ', $this->acme_args) . ' '
. exec_safe('--domain %s', (string)$this->config->name);
. LeUtils::execSafe('--domain %s', (string)$this->config->name);
LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug);
$proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env);
@@ -528,8 +527,8 @@ class LeCertificate extends LeCommon
$acmecmd = '/usr/local/sbin/acme.sh '
. '--revoke '
. implode(' ', $this->acme_args) . ' '
. exec_safe('--domain %s', (string)$this->config->name) . ' '
. exec_safe('--accountconf %s', $account_conf_file);
. LeUtils::execSafe('--domain %s', (string)$this->config->name) . ' '
. LeUtils::execSafe('--accountconf %s', $account_conf_file);
LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug);
$proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env);
@@ -4,7 +4,11 @@
* Copyright (C) 2017-2020 Frank Wall
* Copyright (C) 2015 Deciso B.V.
* Copyright (C) 2010 Jim Pingle <jimp@pfsense.org>
* Copyright (C) 2010 Ermal Luçi
* Copyright (C) 2008 Shrew Soft Inc. <mgrooms@shrew.net>
* Copyright (C) 2005-2006 Colin Smith <ethethlay@gmail.com>
* Copyright (C) 2004-2007 Scott Ullrich <sullrich@gmail.com>
* Copyright (C) 2003-2004 Manuel Kasper <mk@neon1.net>
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -49,6 +53,27 @@ class LeUtils
return rtrim(strtr(base64_encode($str), '+/', '-_'), '=');
}
/**
* Properly escape arguments to prevent shell command injection.
* This is a copy of the exec_safe() legacy function.
* @param string $format The format string to use.
* @param string $args The arguments to escape.
* @return array|string The formatted and escaped arguments.
*/
public static function execSafe($format, $args = array())
{
if (!is_array($args)) {
/* just in case there's only one argument */
$args = array($args);
}
foreach ($args as $id => $arg) {
$args[$id] = escapeshellarg($arg);
}
return vsprintf($format, $args);
}
// Copied from system_camanager.php.
public static function local_ca_import(&$ca, $str, $key = "", $serial = 0)
{
@@ -34,9 +34,6 @@ use OPNsense\Core\Config;
use OPNsense\AcmeClient\LeAccount;
use OPNsense\AcmeClient\LeUtils;
// Load legacy functions
require_once("util.inc"); // for exec_safe()
/**
* LeValidation stub file, contains shared logic for all validation methods.
* @package OPNsense\AcmeClient
@@ -82,8 +79,8 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
// Store acme hook
switch ((string)$this->config->method) {
case 'dns01':
$this->acme_args[] = exec_safe('--dns %s', (string)$this->config->dns_service);
$this->acme_args[] = exec_safe('--dnssleep %s', (string)$this->config->dns_sleep);
$this->acme_args[] = LeUtils::execSafe('--dns %s', (string)$this->config->dns_service);
$this->acme_args[] = LeUtils::execSafe('--dnssleep %s', (string)$this->config->dns_sleep);
break;
case 'http01':
$this->acme_args[] = '--webroot /var/etc/acme-client/challenges';
@@ -91,11 +88,11 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
}
// Store acme filenames
$this->acme_args[] = exec_safe('--home %s', self::ACME_HOME_DIR);
$this->acme_args[] = exec_safe('--certpath %s', sprintf(self::ACME_CERT_FILE, $this->cert_id));
$this->acme_args[] = exec_safe('--keypath %s', sprintf(self::ACME_KEY_FILE, $this->cert_id));
$this->acme_args[] = exec_safe('--capath %s', sprintf(self::ACME_CHAIN_FILE, $this->cert_id));
$this->acme_args[] = exec_safe('--fullchainpath %s', sprintf(self::ACME_FULLCHAIN_FILE, $this->cert_id));
$this->acme_args[] = LeUtils::execSafe('--home %s', self::ACME_HOME_DIR);
$this->acme_args[] = LeUtils::execSafe('--certpath %s', sprintf(self::ACME_CERT_FILE, $this->cert_id));
$this->acme_args[] = LeUtils::execSafe('--keypath %s', sprintf(self::ACME_KEY_FILE, $this->cert_id));
$this->acme_args[] = LeUtils::execSafe('--capath %s', sprintf(self::ACME_CHAIN_FILE, $this->cert_id));
$this->acme_args[] = LeUtils::execSafe('--fullchainpath %s', sprintf(self::ACME_FULLCHAIN_FILE, $this->cert_id));
return true;
}
@@ -165,7 +162,7 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
$acmecmd = '/usr/local/sbin/acme.sh '
. "--${acme_action} "
. implode(' ', $this->acme_args) . ' '
. exec_safe('--accountconf %s', $account_conf_file);
. LeUtils::execSafe('--accountconf %s', $account_conf_file);
LeUtils::log_debug('running acme.sh command: ' . (string)$acmecmd, $this->debug);
$proc = proc_open($acmecmd, $proc_desc, $proc_pipes, null, $proc_env);
@@ -215,7 +212,7 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
$key_length = $length;
}
$this->acme_args[] = exec_safe('--keylength %s', $key_length);
$this->acme_args[] = LeUtils::execSafe('--keylength %s', $key_length);
$this->cert_keylength = $length;
}
@@ -232,7 +229,7 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
$this->cert_challengealias = $challengealias;
// Main domain for acme
$this->acme_args[] = exec_safe('--domain %s', $certname);
$this->acme_args[] = LeUtils::execSafe('--domain %s', $certname);
// Main domain: Use DNS alias mode for domain validation?
// https://github.com/Neilpang/acme.sh/wiki/DNS-alias-mode
@@ -241,14 +238,14 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
case 'automatic':
$name = '_acme-challenge.' . ltrim((string)$this->cert_name, '*.');
if ($dst = dns_get_record($name, DNS_CNAME)) {
$this->acme_args[] = exec_safe('--domain-alias %s', $dst[0]['target']);
$this->acme_args[] = LeUtils::execSafe('--domain-alias %s', $dst[0]['target']);
}
break;
case 'domain':
$this->acme_args[] = exec_safe('--domain-alias %s', (string)$this->cert_domainalias);
$this->acme_args[] = LeUtils::execSafe('--domain-alias %s', (string)$this->cert_domainalias);
break;
case 'challenge':
$this->acme_args[] = exec_safe('--challenge-alias %s', (string)$this->cert_challengealias);
$this->acme_args[] = LeUtils::execSafe('--challenge-alias %s', (string)$this->cert_challengealias);
break;
}
}
@@ -256,7 +253,7 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
// altNames
if (!empty((string)$this->cert_altnames)) {
foreach (explode(",", (string)$this->cert_altnames) as $altname) {
$this->acme_args[] = exec_safe('--domain %s', $altname);
$this->acme_args[] = LeUtils::execSafe('--domain %s', $altname);
// altNames: Use DNS alias mode for domain validation?
// https://github.com/Neilpang/acme.sh/wiki/DNS-alias-mode
@@ -265,14 +262,14 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
case 'automatic':
$name = "_acme-challenge." . ltrim($altname, '*.');
if ($dst = dns_get_record($name, DNS_CNAME)) {
$this->acme_args[] = exec_safe('--domain-alias %s', $dst[0]['target']);
$this->acme_args[] = LeUtils::execSafe('--domain-alias %s', $dst[0]['target']);
}
break;
case 'domain':
$this->acme_args[] = exec_safe('--domain-alias %s', (string)$this->cert_domainalias);
$this->acme_args[] = LeUtils::execSafe('--domain-alias %s', (string)$this->cert_domainalias);
break;
case 'challenge':
$this->acme_args[] = exec_safe('--challenge-alias %s', (string)$this->cert_challengealias);
$this->acme_args[] = LeUtils::execSafe('--challenge-alias %s', (string)$this->cert_challengealias);
break;
}
}
@@ -296,6 +293,6 @@ abstract class Base extends \OPNsense\AcmeClient\LeCommon
*/
public function setRenewal(int $interval = 60)
{
$this->acme_args[] = exec_safe('--days %s', (string)$interval);
$this->acme_args[] = LeUtils::execSafe('--days %s', (string)$interval);
}
}