From ea6bec851b8ce980930fc7439b4fd636d34ab527 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 23 Nov 2014 18:21:56 -0800 Subject: [PATCH 1/8] Add utility functions for checking CSR and privkey --- letsencrypt/client/crypto_util.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/letsencrypt/client/crypto_util.py b/letsencrypt/client/crypto_util.py index 017b13e85..db6270dec 100644 --- a/letsencrypt/client/crypto_util.py +++ b/letsencrypt/client/crypto_util.py @@ -198,3 +198,31 @@ def get_cert_info(filename): "pub_key": "RSA " + str(cert.get_pubkey().size() * 8), } +def valid_csr(csr_filename): + """Check if csr_filename is a valid CSR. (Currently, could raise + non-X.509-related errors such as IOError associated with problems + reading the file.)""" + try: + csr = M2Crypto.X509.load_request(csr_filename) + return bool(csr.verify(csr.get_pubkey())) + except M2Crypto.X509.X509Error: + return False + +def valid_privkey(privkey_filename): + """Check if privkey_filename is a valid RSA private key. (Currently, + could raise non-RSA-related errors such as IOError associated with + problems reading the file.)""" + try: + privkey = M2Crypto.RSA.load_key(privkey_filename) + return bool(privkey.check_key()) + except M2Crypto.RSA.RSAError: + return False + +def csr_matches_pubkey(csr_filename, privkey_filename): + """Check if the private key in the file corresponds to the subject + public key in the CSR.""" + csr = M2Crypto.X509.load_request(csr_filename) + privkey = M2Crypto.RSA.load_key(privkey_filename) + csr_pub = csr.get_pubkey().get_rsa().pub() + privkey_pub = privkey.pub() + return csr_pub == privkey_pub From 7e71bccf28fd430c4b288606a57cc742c9107484 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 14:10:35 -0800 Subject: [PATCH 2/8] Add documentation for parameters of new functions --- letsencrypt/client/crypto_util.py | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/letsencrypt/client/crypto_util.py b/letsencrypt/client/crypto_util.py index db6270dec..f563a0d92 100644 --- a/letsencrypt/client/crypto_util.py +++ b/letsencrypt/client/crypto_util.py @@ -201,7 +201,14 @@ def get_cert_info(filename): def valid_csr(csr_filename): """Check if csr_filename is a valid CSR. (Currently, could raise non-X.509-related errors such as IOError associated with problems - reading the file.)""" + reading the file.) + + :param csr_filename: Path to the purported CSR file. + :type csr_filename: str + + :returns: Validity of CSR. + :rtype: bool""" + try: csr = M2Crypto.X509.load_request(csr_filename) return bool(csr.verify(csr.get_pubkey())) @@ -211,7 +218,14 @@ def valid_csr(csr_filename): def valid_privkey(privkey_filename): """Check if privkey_filename is a valid RSA private key. (Currently, could raise non-RSA-related errors such as IOError associated with - problems reading the file.)""" + problems reading the file.) + + :param privkey_filename: Path to the purported private key file. + :type privkey_filename: str + + :returns: Validity of private key. + :rtype: bool""" + try: privkey = M2Crypto.RSA.load_key(privkey_filename) return bool(privkey.check_key()) @@ -220,7 +234,17 @@ def valid_privkey(privkey_filename): def csr_matches_pubkey(csr_filename, privkey_filename): """Check if the private key in the file corresponds to the subject - public key in the CSR.""" + public key in the CSR. + + :param csr_filename: Path to the purported CSR file. + :type csr_filename: str + + :param privkey_filename: Path to the purported private key file. + :type privkey_filename: str + + :returns: Correspondence of private key to CSR subject public key. + :rtype: bool""" + csr = M2Crypto.X509.load_request(csr_filename) privkey = M2Crypto.RSA.load_key(privkey_filename) csr_pub = csr.get_pubkey().get_rsa().pub() From 2e8cdd071a25ddcca45406f36397d4a1b0d6d48e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 14:16:29 -0800 Subject: [PATCH 3/8] Integrate CSR and private key validation steps into client --- letsencrypt/client/client.py | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 590714d6f..58a4a4a09 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -48,12 +48,32 @@ class Client(object): self.key_file = private_key # If CSR is provided, the private key should also be provided. - # TODO: Make sure key was actually used in CSR - # TODO: Make sure key has proper permissions if self.csr_file and not self.key_file: logger.fatal("Please provide the private key file used in \ generating the provided CSR") sys.exit(1) + # If CSR is provided, it must be readable and valid. + try: + if self.csr_file and not crypto.util.valid_csr(self.csr_file): + logger.fatal("The provided CSR is not a valid CSR") + sys.exit(1) + except IOError, e: + logger.fatal("The provided CSR could not be read") + sys.exit(1) + # If key is provided, it must be readable and valid. + try: + if self.key_file and not crypto.util.valid_privkey(self.key_file): + logger.fatal("The provided key is not a valid key") + sys.exit(1) + except IOError, e: + logger.fatal("The provided key could not be read") + sys.exit(1) + # If CSR and key are provided, the key must be the same key used + # in the CSR. + if self.csr_file and self.key_file and not csr_matches_pubkey(self.csr_file, self.key_file): + logger.fatal("The provided key is not the same key referred to by \ + the CSR file") + sys.exit(1) self.server_url = "https://%s/acme/" % self.server From 35c34ec6d4127550208c9a7a432a90a21f7c0358 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 15:52:22 -0800 Subject: [PATCH 4/8] PEP8 formatting fixes --- letsencrypt/client/crypto_util.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/crypto_util.py b/letsencrypt/client/crypto_util.py index f563a0d92..4c5cdbffd 100644 --- a/letsencrypt/client/crypto_util.py +++ b/letsencrypt/client/crypto_util.py @@ -54,8 +54,8 @@ def create_sig(msg, key_file, nonce=None, nonce_len=CONFIG.NONCE_SIZE): logger.debug('%s signed as %s' % (msg_with_nonce, signature)) - n_bytes = binascii.unhexlify(leading_zeros(hex(key.n)[2:].replace("L", ""))) - e_bytes = binascii.unhexlify(leading_zeros(hex(key.e)[2:].replace("L", ""))) + n_bytes = binascii.unhexlify(leading_zeros(hex(key.n)[2:].rstrip("L"))) + e_bytes = binascii.unhexlify(leading_zeros(hex(key.e)[2:].rstrip("L"))) return { "nonce": le_util.b64_url_enc(nonce), @@ -198,6 +198,7 @@ def get_cert_info(filename): "pub_key": "RSA " + str(cert.get_pubkey().size() * 8), } + def valid_csr(csr_filename): """Check if csr_filename is a valid CSR. (Currently, could raise non-X.509-related errors such as IOError associated with problems @@ -215,6 +216,7 @@ def valid_csr(csr_filename): except M2Crypto.X509.X509Error: return False + def valid_privkey(privkey_filename): """Check if privkey_filename is a valid RSA private key. (Currently, could raise non-RSA-related errors such as IOError associated with @@ -232,6 +234,7 @@ def valid_privkey(privkey_filename): except M2Crypto.RSA.RSAError: return False + def csr_matches_pubkey(csr_filename, privkey_filename): """Check if the private key in the file corresponds to the subject public key in the CSR. From c741d969debf71689aff599294d8f30bc06a4ec1 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 15:52:48 -0800 Subject: [PATCH 5/8] Correct PEM/DER return behavior --- letsencrypt/client/client.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 58a4a4a09..abe8ec87b 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -70,7 +70,8 @@ class Client(object): sys.exit(1) # If CSR and key are provided, the key must be the same key used # in the CSR. - if self.csr_file and self.key_file and not csr_matches_pubkey(self.csr_file, self.key_file): + if self.csr_file and self.key_file and \ + not csr_matches_pubkey(self.csr_file, self.key_file): logger.fatal("The provided key is not the same key referred to by \ the CSR file") sys.exit(1) @@ -615,10 +616,8 @@ class Client(object): def get_key_csr_pem(self, csr_return_format='der'): """ Returns key and CSR using provided files or generating new files if - necessary. Both will be saved in pem format on the filesystem. - The CSR can optionally be returned in DER format as the CSR cannot be - loaded back into M2Crypto. - """ + necessary. Both will be saved in PEM format on the filesystem. + The CSR can optionally be returned in DER format.""" key_pem = None csr_pem = None if not self.key_file: @@ -647,17 +646,17 @@ class Client(object): csr_f.close() logger.info("Creating CSR: %s" % self.csr_file) else: - # TODO fix this der situation try: - csr_pem = open(self.csr_file).read().replace("\r", "") + csr = M2Crypto.X509.load_request(self.csr_file) + csr_pem, csr_der = csr.as_pem(), csr.as_der() except: logger.fatal("Unable to open CSR file: %s" % self.csr_file) sys.exit(1) if csr_return_format == 'der': return key_pem, csr_der - - return key_pem, csr_pem + else: + return key_pem, csr_pem # def choice_of_ca(self): # choices = self.get_cas() From b4492042d90c1cbd036088b2fbe6959689c92eda Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 15:59:32 -0800 Subject: [PATCH 6/8] For crypto.util, read crypto_util --- letsencrypt/client/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index abe8ec87b..1e61f5d30 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -54,7 +54,7 @@ class Client(object): sys.exit(1) # If CSR is provided, it must be readable and valid. try: - if self.csr_file and not crypto.util.valid_csr(self.csr_file): + if self.csr_file and not crypto_util.valid_csr(self.csr_file): logger.fatal("The provided CSR is not a valid CSR") sys.exit(1) except IOError, e: @@ -62,7 +62,7 @@ class Client(object): sys.exit(1) # If key is provided, it must be readable and valid. try: - if self.key_file and not crypto.util.valid_privkey(self.key_file): + if self.key_file and not crypto_util.valid_privkey(self.key_file): logger.fatal("The provided key is not a valid key") sys.exit(1) except IOError, e: From 4453d3fab89c592e94b7dfe0381e6b59e392ac5f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 24 Nov 2014 16:02:30 -0800 Subject: [PATCH 7/8] Add a missing crypto_util --- letsencrypt/client/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 1e61f5d30..621e0ab82 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -70,8 +70,8 @@ class Client(object): sys.exit(1) # If CSR and key are provided, the key must be the same key used # in the CSR. - if self.csr_file and self.key_file and \ - not csr_matches_pubkey(self.csr_file, self.key_file): + if self.csr_file and self.key_file and not \ + crypto_util.csr_matches_pubkey(self.csr_file, self.key_file): logger.fatal("The provided key is not the same key referred to by \ the CSR file") sys.exit(1) From b245bbb10ea3a94a4559071be09421bcb4a8caf8 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 24 Nov 2014 18:58:34 -0800 Subject: [PATCH 8/8] Added name check and attempted to clean up code - is PEP8 conformant --- letsencrypt/client/client.py | 66 ++++++++++++++++++------------- letsencrypt/client/crypto_util.py | 49 ++++++++++++++++++----- 2 files changed, 78 insertions(+), 37 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 621e0ab82..2c976b34c 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -47,34 +47,7 @@ class Client(object): self.csr_file = cert_signing_request self.key_file = private_key - # If CSR is provided, the private key should also be provided. - if self.csr_file and not self.key_file: - logger.fatal("Please provide the private key file used in \ - generating the provided CSR") - sys.exit(1) - # If CSR is provided, it must be readable and valid. - try: - if self.csr_file and not crypto_util.valid_csr(self.csr_file): - logger.fatal("The provided CSR is not a valid CSR") - sys.exit(1) - except IOError, e: - logger.fatal("The provided CSR could not be read") - sys.exit(1) - # If key is provided, it must be readable and valid. - try: - if self.key_file and not crypto_util.valid_privkey(self.key_file): - logger.fatal("The provided key is not a valid key") - sys.exit(1) - except IOError, e: - logger.fatal("The provided key could not be read") - sys.exit(1) - # If CSR and key are provided, the key must be the same key used - # in the CSR. - if self.csr_file and self.key_file and not \ - crypto_util.csr_matches_pubkey(self.csr_file, self.key_file): - logger.fatal("The provided key is not the same key referred to by \ - the CSR file") - sys.exit(1) + self.__validate_csr_key_cli() self.server_url = "https://%s/acme/" % self.server @@ -119,6 +92,9 @@ class Client(object): # Get key and csr to perform challenges _, csr_der = self.get_key_csr_pem() + if not crypto_util.csr_matches_names(self.csr_file, self.names): + raise Exception("CSR subject does not contain the specified name") + # Perform Challenges responses, challenge_objs = self.verify_identity(challenge_msg) # Get Authorization @@ -658,6 +634,40 @@ class Client(object): else: return key_pem, csr_pem + def __validate_csr_key_cli(self): + """ + Verifies that the client key and csr arguments are valid and correspond + to one another""" + # If CSR is provided, the private key should also be provided. + if self.csr_file and not self.key_file: + logger.fatal(("Please provide the private key file used in " + "generating the provided CSR")) + sys.exit(1) + # If CSR is provided, it must be readable and valid. + try: + if self.csr_file and not crypto_util.valid_csr(self.csr_file): + logger.fatal("The provided CSR is not a valid CSR") + sys.exit(1) + except IOError, e: + raise Exception("The provided CSR could not be read") + # If key is provided, it must be readable and valid. + try: + if self.key_file and not crypto_util.valid_privkey(self.key_file): + logger.fatal("The provided key is not a valid key") + sys.exit(1) + except IOError, e: + raise Exception("The provided key could not be read") + + # If CSR and key are provided, the key must be the same key used + # in the CSR. + if self.csr_file and self.key_file: + try: + if not crypto_util.csr_matches_pubkey(self.csr_file, + self.key_file): + raise Exception("The key and CSR do not match") + except IOError, e: + raise Exception("The key or CSR files could not be read") + # def choice_of_ca(self): # choices = self.get_cas() # message = ("Pick a Certificate Authority. " diff --git a/letsencrypt/client/crypto_util.py b/letsencrypt/client/crypto_util.py index 4c5cdbffd..2196f25ba 100644 --- a/letsencrypt/client/crypto_util.py +++ b/letsencrypt/client/crypto_util.py @@ -148,12 +148,12 @@ def make_ss_cert(key_file, domains): cert.set_not_before(current) cert.set_not_after(expire) - name = cert.get_subject() - name.C = "US" - name.ST = "Michigan" - name.L = "Ann Arbor" - name.O = "University of Michigan and the EFF" - name.CN = domains[0] + subject = cert.get_subject() + subject.C = "US" + subject.ST = "Michigan" + subject.L = "Ann Arbor" + subject.O = "University of Michigan and the EFF" + subject.CN = domains[0] cert.set_issuer(cert.get_subject()) cert.add_ext(M2Crypto.X509.new_extension('basicConstraints', 'CA:FALSE')) @@ -199,10 +199,15 @@ def get_cert_info(filename): } +# WARNING: the csr and private key file are possible attack vectors for TOCTOU +# We should either... +# A. Do more checks to verify that the CSR is trusted/valid +# B. Audit the parsing code for vulnerabilities + def valid_csr(csr_filename): - """Check if csr_filename is a valid CSR. (Currently, could raise - non-X.509-related errors such as IOError associated with problems - reading the file.) + """Check if csr_filename is a valid CSR for the given domains. + (Currently, could raise non-X.509-related errors such as IOError + associated with problems reading the file.) :param csr_filename: Path to the purported CSR file. :type csr_filename: str @@ -217,6 +222,32 @@ def valid_csr(csr_filename): return False +def csr_matches_names(csr_filename, domains): + """Check if csr_filename contains the subject of one of the domains + M2Crypto currently does not expose the OpenSSL interface to + also check the SAN extension. This is insufficient for full testing + (Currently, could raise non-X.509-related errors such as IOError + associated with problems reading the file.) + + :param csr_filename: Path to the purported CSR file. + :type csr_filename: str + + :param domains: domains the csr should contain + :type domains: list + + :returns: If the csr subject contains one of the domains + :rtype: bool""" + + try: + csr = M2Crypto.X509.load_request(csr_filename) + subject = csr.get_subject() + + return subject.CN in domains + + except M2Crypto.X509.X509Error: + return False + + def valid_privkey(privkey_filename): """Check if privkey_filename is a valid RSA private key. (Currently, could raise non-RSA-related errors such as IOError associated with