From b245bbb10ea3a94a4559071be09421bcb4a8caf8 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 24 Nov 2014 18:58:34 -0800 Subject: [PATCH] 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