From 8ad18cbe6e27ab6125510ccf0ce07b624a431356 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Fri, 1 Sep 2017 07:57:30 -0700 Subject: Use ffdhe2048 Nginx DH params to fix Weak-DH bug (#4973) * Rename plugins.common.install_ssl_options_conf to plugins.common.install_version_controlled_file * Install ssl_dhparams file * Add installation test * Add ssl_dhparam option when making a server block ssl * add install_ssl_dhparams to Installer common plugin class * Remove redundant code and tests * update MANIFEST.in --- MANIFEST.in | 1 + certbot-apache/certbot_apache/configurator.py | 2 +- .../certbot_apache/tests/configurator_test.py | 2 +- certbot-nginx/certbot_nginx/configurator.py | 16 +++--- .../certbot_nginx/tests/configurator_test.py | 17 ++++--- certbot/constants.py | 18 ++++++- certbot/plugins/common.py | 59 ++++++++++++++-------- certbot/plugins/common_test.py | 48 ++++++++++++------ certbot/ssl-dhparams.pem | 8 +++ 9 files changed, 118 insertions(+), 53 deletions(-) create mode 100644 certbot/ssl-dhparams.pem diff --git a/MANIFEST.in b/MANIFEST.in index 18393e3e1..434a156b7 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -6,3 +6,4 @@ include linter_plugin.py recursive-include docs * recursive-include examples * recursive-include certbot/tests/testdata * +include certbot/ssl-dhparams.pem diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 9e3eb4139..4960d08d7 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1989,5 +1989,5 @@ def install_ssl_options_conf(options_ssl, options_ssl_digest): # XXX if we ever try to enforce a local privilege boundary (eg, running # certbot for unprivileged users via setuid), this function will need # to be modified. - return common.install_ssl_options_conf(options_ssl, options_ssl_digest, + return common.install_version_controlled_file(options_ssl, options_ssl_digest, constants.os_constant("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 23242d091..5805a8b31 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1497,7 +1497,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertEqual(mock_logger.warning.call_args[0][0], - "%s has been manually modified; updated ssl configuration options " + "%s has been manually modified; updated file " "saved to %s. We recommend updating %s for security purposes.") self.assertEqual(crypto_util.sha256sum(constants.os_constant("MOD_SSL_CONF_SRC")), self._current_ssl_options_hash()) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index aac8c1237..7e55ff0ea 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -157,6 +157,8 @@ class NginxConfigurator(common.Installer): install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest) + self.install_ssl_dhparams() + # Set Version if self.version is None: self.version = self.get_version() @@ -449,11 +451,13 @@ class NginxConfigurator(common.Installer): snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() - ssl_block = ( - [['\n ', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)], - ['\n ', 'ssl_certificate', ' ', snakeoil_cert], - ['\n ', 'ssl_certificate_key', ' ', snakeoil_key], - ['\n ', 'include', ' ', self.mod_ssl_conf]]) + ssl_block = ([ + ['\n ', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)], + ['\n ', 'ssl_certificate', ' ', snakeoil_cert], + ['\n ', 'ssl_certificate_key', ' ', snakeoil_key], + ['\n ', 'include', ' ', self.mod_ssl_conf], + ['\n ', 'ssl_dhparam', ' ', self.ssl_dhparams], + ]) self.parser.add_server_directives( vhost, ssl_block, replace=False) @@ -822,5 +826,5 @@ def nginx_restart(nginx_ctl, nginx_conf): def install_ssl_options_conf(options_ssl, options_ssl_digest): """Copy Certbot's SSL options file into the system's config dir if required.""" - return common.install_ssl_options_conf(options_ssl, options_ssl_digest, + return common.install_version_controlled_file(options_ssl, options_ssl_digest, constants.MOD_SSL_CONF_SRC, constants.ALL_SSL_OPTIONS_HASHES) diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 1f9d3e253..6a917204c 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -226,8 +226,9 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '5001', 'ssl'], ['ssl_certificate', 'example/fullchain.pem'], ['ssl_certificate_key', 'example/key.pem'], - ['include', self.config.mod_ssl_conf]] - ]], + ['include', self.config.mod_ssl_conf], + ['ssl_dhparam', self.config.ssl_dhparams], + ]]], parsed_example_conf) self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']], parsed_server_conf) @@ -244,8 +245,9 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '5001', 'ssl'], ['ssl_certificate', '/etc/nginx/fullchain.pem'], ['ssl_certificate_key', '/etc/nginx/key.pem'], - ['include', self.config.mod_ssl_conf]] - ], + ['include', self.config.mod_ssl_conf], + ['ssl_dhparam', self.config.ssl_dhparams], + ]], 2)) def test_deploy_cert_add_explicit_listen(self): @@ -268,8 +270,9 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '5001', 'ssl'], ['ssl_certificate', 'summer/fullchain.pem'], ['ssl_certificate_key', 'summer/key.pem'], - ['include', self.config.mod_ssl_conf]] - ], + ['include', self.config.mod_ssl_conf], + ['ssl_dhparam', self.config.ssl_dhparams], + ]], parsed_migration_conf[0]) @mock.patch("certbot_nginx.configurator.tls_sni_01.NginxTlsSni01.perform") @@ -601,7 +604,7 @@ class InstallSslOptionsConfTest(util.NginxTest): with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertEqual(mock_logger.warning.call_args[0][0], - "%s has been manually modified; updated ssl configuration options " + "%s has been manually modified; updated file " "saved to %s. We recommend updating %s for security purposes.") self.assertEqual(crypto_util.sha256sum(constants.MOD_SSL_CONF_SRC), self._current_ssl_options_hash()) diff --git a/certbot/constants.py b/certbot/constants.py index dfdfcc0e8..557ccd4c6 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -1,6 +1,7 @@ """Certbot constants.""" -import os import logging +import os +import pkg_resources from acme import challenges @@ -112,3 +113,18 @@ FORCE_INTERACTIVE_FLAG = "--force-interactive" EFF_SUBSCRIBE_URI = "https://supporters.eff.org/subscribe/certbot" """EFF URI used to submit the e-mail address of users who opt-in.""" + +SSL_DHPARAMS_DEST = "ssl-dhparams.pem" +"""Name of the ssl_dhparams file as saved in `IConfig.config_dir`.""" + +SSL_DHPARAMS_SRC = pkg_resources.resource_filename( + "certbot", "ssl-dhparams.pem") +"""Path to the nginx ssl_dhparams file found in the Certbot distribution.""" + +UPDATED_SSL_DHPARAMS_DIGEST = ".updated-ssl-dhparams-pem-digest.txt" +"""Name of the hash of the updated or informed ssl_dhparams as saved in `IConfig.config_dir`.""" + +ALL_SSL_DHPARAMS_HASHES = [ + '9ba6429597aeed2d8617a7705b56e96d044f64b07971659382e426675105654b', +] +"""SHA256 hashes of the contents of all versions of SSL_DHPARAMS_SRC""" diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 7bb019163..f605eb751 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -103,7 +103,7 @@ class Plugin(object): class Installer(Plugin): - """An installer base class with reverter methods defined. + """An installer base class with reverter and ssl_dhparam methods defined. Installer plugins do not have to inherit from this class. @@ -197,6 +197,24 @@ class Installer(Plugin): except errors.ReverterError as err: raise errors.PluginError(str(err)) + @property + def ssl_dhparams(self): + """Full absolute path to ssl_dhparams file.""" + return os.path.join(self.config.config_dir, constants.SSL_DHPARAMS_DEST) + + @property + def updated_ssl_dhparams_digest(self): + """Full absolute path to digest of updated ssl_dhparams file.""" + return os.path.join(self.config.config_dir, constants.UPDATED_SSL_DHPARAMS_DIGEST) + + def install_ssl_dhparams(self): + """Copy Certbot's ssl_dhparams file into the system's config dir if required.""" + return install_version_controlled_file( + self.ssl_dhparams, + self.updated_ssl_dhparams_digest, + constants.SSL_DHPARAMS_SRC, + constants.ALL_SSL_DHPARAMS_HASHES) + class Addr(object): r"""Represents an virtual host address. @@ -368,51 +386,50 @@ class TLSSNI01(object): return response -def install_ssl_options_conf(options_ssl, options_ssl_digest, mod_ssl_conf_src, - all_ssl_options_hashes): - """Copy Certbot's SSL options file into the system's config dir if required. +def install_version_controlled_file(dest_path, digest_path, src_path, all_hashes): + """Copy a file into an active location (likely the system's config dir) if required. - :param str options_ssl: destination path for file containing ssl options - :param str options_ssl_digest: path to save a digest of options_ssl in - :param str mod_ssl_conf_src: path to file containing ssl options found in distribution - :param list all_ssl_options_hashes: hashes of every released version of options_ssl + :param str dest_path: destination path for version controlled file + :param str digest_path: path to save a digest of the file in + :param str src_path: path to version controlled file found in distribution + :param list all_hashes: hashes of every released version of the file """ - current_ssl_options_hash = crypto_util.sha256sum(mod_ssl_conf_src) + current_hash = crypto_util.sha256sum(src_path) def _write_current_hash(): - with open(options_ssl_digest, "w") as f: - f.write(current_ssl_options_hash) + with open(digest_path, "w") as f: + f.write(current_hash) def _install_current_file(): - shutil.copyfile(mod_ssl_conf_src, options_ssl) + shutil.copyfile(src_path, dest_path) _write_current_hash() # Check to make sure options-ssl.conf is installed - if not os.path.isfile(options_ssl): + if not os.path.isfile(dest_path): _install_current_file() return # there's already a file there. if it's up to date, do nothing. if it's not but # it matches a known file hash, we can update it. # otherwise, print a warning once per new version. - active_file_digest = crypto_util.sha256sum(options_ssl) - if active_file_digest == current_ssl_options_hash: # already up to date + active_file_digest = crypto_util.sha256sum(dest_path) + if active_file_digest == current_hash: # already up to date return - elif active_file_digest in all_ssl_options_hashes: # safe to update + elif active_file_digest in all_hashes: # safe to update _install_current_file() else: # has been manually modified, not safe to update # did they modify the current version or an old version? - if os.path.isfile(options_ssl_digest): - with open(options_ssl_digest, "r") as f: + if os.path.isfile(digest_path): + with open(digest_path, "r") as f: saved_digest = f.read() # they modified it after we either installed or told them about this version, so return - if saved_digest == current_ssl_options_hash: + if saved_digest == current_hash: return # there's a new version but we couldn't update the file, or they deleted the digest. # save the current digest so we only print this once, and print a warning _write_current_hash() - logger.warning("%s has been manually modified; updated ssl configuration options " + logger.warning("%s has been manually modified; updated file " "saved to %s. We recommend updating %s for security purposes.", - options_ssl, mod_ssl_conf_src, options_ssl) + dest_path, src_path, dest_path) # test utils used by certbot_apache/certbot_nginx (hence diff --git a/certbot/plugins/common_test.py b/certbot/plugins/common_test.py index cece2c5f9..8ce68bbb5 100644 --- a/certbot/plugins/common_test.py +++ b/certbot/plugins/common_test.py @@ -79,14 +79,16 @@ class PluginTest(unittest.TestCase): "--mock-foo-bar", dest="different_to_foo_bar", x=1, y=None) -class InstallerTest(unittest.TestCase): +class InstallerTest(test_util.ConfigTestCase): """Tests for certbot.plugins.common.Installer.""" def setUp(self): + super(InstallerTest, self).setUp() + os.mkdir(self.config.config_dir) from certbot.plugins.common import Installer with mock.patch("certbot.plugins.common.reverter.Reverter"): - self.installer = Installer(config=mock.MagicMock(), + self.installer = Installer(config=self.config, name="Installer") self.reverter = self.installer.reverter @@ -163,6 +165,20 @@ class InstallerTest(unittest.TestCase): self.assertRaises( errors.PluginError, installer_func, *passed_args, **passed_kwargs) + def test_install_ssl_dhparams(self): + self.installer.install_ssl_dhparams() + self.assertTrue(os.path.isfile(self.installer.ssl_dhparams)) + + def _current_ssl_dhparams_hash(self): + from certbot.constants import SSL_DHPARAMS_SRC + return crypto_util.sha256sum(SSL_DHPARAMS_SRC) + + def test_current_file_hash_in_all_hashes(self): + from certbot.constants import ALL_SSL_DHPARAMS_HASHES + self.assertTrue(self._current_ssl_dhparams_hash() in ALL_SSL_DHPARAMS_HASHES, + "Constants.ALL_SSL_DHPARAMS_HASHES must be appended" + " with the sha256 hash of self.config.ssl_dhparams when it is updated.") + class AddrTest(unittest.TestCase): """Tests for certbot.client.plugins.common.Addr.""" @@ -314,11 +330,11 @@ class TLSSNI01Test(unittest.TestCase): achall.response(achall.account_key).z_domain.decode("utf-8")) -class InstallSslOptionsConfTest(test_util.TempDirTestCase): - """Tests for certbot.plugins.common.install_ssl_options_conf.""" +class InstallVersionControlledFileTest(test_util.TempDirTestCase): + """Tests for certbot.plugins.common.install_version_controlled_file.""" def setUp(self): - super(InstallSslOptionsConfTest, self).setUp() + super(InstallVersionControlledFileTest, self).setUp() self.hashes = ["someotherhash"] self.dest_path = os.path.join(self.tempdir, "options-ssl-dest.conf") self.hash_path = os.path.join(self.tempdir, ".options-ssl-conf.txt") @@ -330,19 +346,19 @@ class InstallSslOptionsConfTest(test_util.TempDirTestCase): self.hashes.append(crypto_util.sha256sum(path)) def _call(self): - from certbot.plugins.common import install_ssl_options_conf - install_ssl_options_conf(self.dest_path, - self.hash_path, - self.source_path, - self.hashes) + from certbot.plugins.common import install_version_controlled_file + install_version_controlled_file(self.dest_path, + self.hash_path, + self.source_path, + self.hashes) - def _current_ssl_options_hash(self): + def _current_file_hash(self): return crypto_util.sha256sum(self.source_path) def _assert_current_file(self): self.assertTrue(os.path.isfile(self.dest_path)) self.assertEqual(crypto_util.sha256sum(self.dest_path), - self._current_ssl_options_hash()) + self._current_file_hash()) def test_no_file(self): self.assertFalse(os.path.isfile(self.dest_path)) @@ -369,9 +385,9 @@ class InstallSslOptionsConfTest(test_util.TempDirTestCase): self.assertFalse(mock_logger.warning.called) self.assertTrue(os.path.isfile(self.dest_path)) self.assertEqual(crypto_util.sha256sum(self.source_path), - self._current_ssl_options_hash()) + self._current_file_hash()) self.assertNotEqual(crypto_util.sha256sum(self.dest_path), - self._current_ssl_options_hash()) + self._current_file_hash()) def test_manually_modified_past_file_warns(self): with open(self.dest_path, "a") as mod_ssl_conf: @@ -381,10 +397,10 @@ class InstallSslOptionsConfTest(test_util.TempDirTestCase): with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertEqual(mock_logger.warning.call_args[0][0], - "%s has been manually modified; updated ssl configuration options " + "%s has been manually modified; updated file " "saved to %s. We recommend updating %s for security purposes.") self.assertEqual(crypto_util.sha256sum(self.source_path), - self._current_ssl_options_hash()) + self._current_file_hash()) # only print warning once with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() diff --git a/certbot/ssl-dhparams.pem b/certbot/ssl-dhparams.pem new file mode 100644 index 000000000..9b182b720 --- /dev/null +++ b/certbot/ssl-dhparams.pem @@ -0,0 +1,8 @@ +-----BEGIN DH PARAMETERS----- +MIIBCAKCAQEA//////////+t+FRYortKmq/cViAnPTzx2LnFg84tNpWp4TZBFGQz ++8yTnc4kmz75fS/jY2MMddj2gbICrsRhetPfHtXV/WVhJDP1H18GbtCFY2VVPe0a +87VXE15/V8k1mE8McODmi3fipona8+/och3xWKE2rec1MKzKT0g6eXq8CrGCsyT7 +YdEIqUuyyOP7uWrat2DX9GgdT0Kj3jlN9K5W7edjcrsZCwenyO4KbXCeAvzhzffi +7MA0BM0oNC9hkXL+nOmFg/+OTxIy7vKBg8P+OxtMb61zO7X8vC7CIAXFjvGDfRaD +ssbzSibBsu/6iGtCOGEoXJf//////////wIBAg== +-----END DH PARAMETERS----- -- cgit v1.2.3