Move EK cert verification to cert_utils #1156

sergio-correia · 2022-11-07T14:52:12Z

We had partially addressed the issue we encountered when parsing some
certificates with python-cryptography by writing cert_utils module that
provided a single helper to parse the pubkey from a certificate.

However, we still were doing the EK validation in tpm_main using
python-cryptography, which means we would fall into the same parsing
problem again, since during the validation it would read all the certs
in the tpm cert store to check if it is signing the presented EK.

We address this issue by moving the EK cert verification to cert_utils:
we use the same approach as before, i.e. we parse the cert with pyasn1
when python-cryptography fails, and then use python-cryptography to do
the actual signature verification, as before.

By moving the method to cert_utils, it also becomes simpler to test it,
so in this commit we more tests to verify the methods work as expected.

Additionally, the updated EK verification is also capable of handling
ECDSA signatures

keylime/cert_utils.py

+            except crypto_exceptions.InvalidSignature:
+                continue
+
+            logger.debug("EK cert matched cert: %s", cert_file)


THS-on

This refactor LGTM, only a few comments.

Where it makes sense also add type hints, to make it easier to check this code with mypy in the future.

.github/workflows/test.yml

THS-on · 2022-11-07T15:10:16Z

keylime/cert_utils.py

+
+
+def x509_der_cert(der_cert_data):
+    if isinstance(der_cert_data, str):


Can you add type hints that both bytes and str is accepted?

I added the type hints, but for now I changed to be in line with comments from Stefan, so x509_der_cert() current accepts bytes and x509_pem_cert() accepts str. Please let me know your thoughts.

keylime/cert_utils.py

keylime/tenant.py

THS-on · 2022-11-07T15:17:20Z

scripts/ek-openssl-verify

@@ -0,0 +1,96 @@
+#!/bin/sh


Can you add maybe also a reference to this script in the documentation? Thanks for adding an example!

THS-on · 2022-11-07T15:19:30Z

keylime/cert_utils.py

+logger = keylime_logging.init_logging("cert_utils")
+
+
+class UserError(Exception):


we have UserError defined multiple times in Keylime it is probably a good idea to move it into https://github.com/keylime/keylime/blob/master/keylime/common/exception.py

I will do this in a separate PR. I removed this definition from here, in any case.

keylime/cert_utils.py

aplanas · 2022-11-07T15:16:43Z

keylime/cert_utils.py

+        cert = x509.load_pem_x509_certificate(data=pem_cert_data, backend=default_backend())
+        return cert
+    except Exception:
+        return x509_der_cert(pem.readPemFromFile(io.StringIO(pem_cert_data)))


That confuses me, if is a call to read a PEM, why do we try a DER here? This make the operation non-symmetric (a call to read a PEM can try a DER, but no vice versa)

If symmetry is required, IMHO is a bit more clear if:

The DER function try the pyasm1 decoder after the conversion to bytes, capturing the pyasn1.error.SubstrateUnderrunError exception to see if it valid or not, and calling the load_der_... only one

Drop in the DER function the conversion to bytes (see later)

In the PEM function only try the load_pem_ ... call

Drop in the PEM function the conversion from bytes

Create a new x509_cert that order the calls to PEM / DER in the order that you suit, and do the conversion from / to bytes

Use only this function to load the certificates, and never call the PEM / DER versions outside this new function

PEM here is the DER data base64 encoded, with the added header and footer (e.g. ----BEGIN CERT...), so once we call pem.readPemFromFile() we have the actual bytes, which we can parse with the method that parses the DER data. I changed it slightly and added a comment, to try to clarify it a bit, but I am not sure I addressed your concern fully, so if you could recheck I would appreciate.

aplanas · 2022-11-07T15:25:04Z

keylime/cert_utils.py

+
+            logger.debug("EK cert matched cert: %s", cert_file)
+            return True
+    except Exception as e:


This can hide problems in the certification load under a misleading error message.

The try should be moved early, surrounding only tpm_ek_ca.cert_loader, because as it is now it is also catching any exception in the PEM / DER loads, that does not indicate any issue with the TPM.

I changed to be in line with your comment. Let me know it if is better now, please.

keylime/cert_utils.py

aplanas · 2022-11-07T15:34:24Z

keylime/cert_utils.py

+        logger.debug("External check script successfully to validated EK")
+        if proc.stdout is not None:
+            logger.info("ek_check output: %s", proc.stdout.decode("utf-8"))
+    except Exception as e:


there should be an specific exception to indicate an error with the subproces

Done, please re-check.

stefanberger · 2022-11-07T15:39:10Z

keylime/cert_utils.py

+
+def verify_ek_script(script, env, cwd):
+    if script is None:
+        logger.warning("External check script (%s) not specified", script)


Not sure what the current direction is for keylime but consider the f-strings with {script} ?

consider the f-strings

That makes me wonder if we want this for logs or not. The logic behind log.level("str", params) (note that % is not used) is that interpolation only happens iff the call is made. With f-strings we move out from lazy interpolation.

Not sure that is relevant, tho.

scripts/ek-openssl-verify

keylime/cert_utils.py

scripts/ek-openssl-verify

mpeters · 2022-11-09T19:33:27Z

This looks good to me but I'll wait until @aplanas @stefanberger and @THS-on can re-comment after you most recent changes. Would be good to get this fixed for 6.5.2

keylime/cert_utils.py

stefanberger · 2022-11-09T21:31:40Z

LGTM

In both check_tpm_cert_store and cert_loader(). This will make it simpler for testing the certs in the store. Additionaly, let's use a dict containing the file name as the key for the cert data, as it may be helpful when debugging cert-related issues. Signed-off-by: Sergio Correia <scorreia@redhat.com>

We had partially addressed the issue we encountered when parsing some certificates with python-cryptography by writing cert_utils module that provided a single helper to parse the pubkey from a certificate. However, we still were doing the EK validation in tpm_main using python-cryptography, which means we would fall into the same parsing problem again, since during the validation it would read all the certs in the tpm cert store to check if it is signing the presented EK. We address this issue by moving the EK cert verification to cert_utils: we use the same approach as before, i.e. we parse the cert with pyasn1 when python-cryptography fails, and then use python-cryptography to do the actual signature verification, as before. By moving the method to cert_utils, it also becomes simpler to test it, so in this commit we more tests to verify the methods work as expected. Additionally, the updated EK verification is also capable of handling ECDSA signatures, so this commit also fixes an issue that had been previously reported. Hovewer, malformed certs that are not parsed by python-cryptography still pose an issue as python-cryptography will be unable to verify their signatures. One possible way to deal with those would be to use different tools (e.g. openssl) to do the signature verification, with the `ek_check_script' option. Signed-off-by: Sergio Correia <scorreia@redhat.com>

Keylime allows to specify an optional script to check the EK and/or EK certtificate against an allowlist or any other additional EK processing you want to do, via the `ek_check_script' option in tenant.conf. In this commit we move the code path that runs this external script to cert_utils, so it becomes simpler to test, and we also provide a sample `ek-openssl-verify' script that uses openssl to do the validation of an EK certificate provided via the EK_CERT env var, against the certificates in the TPM cert store. Signed-off-by: Sergio Correia <scorreia@redhat.com>

THS-on · 2022-11-10T08:51:32Z

/packit retest-failed

THS-on · 2022-11-10T09:58:08Z

@kkaarreell @ansasaki the measured boot unit test fails on centos 9. Any idea why?

sergio-correia · 2022-11-10T10:00:22Z

@kkaarreell @ansasaki the measured boot unit test fails on centos 9. Any idea why?

It fails in Centos 8 stream. From what I saw here quickly, the tpm2-tools version there (4.1.1) does not have tpm2_eventlog, so the test fails.

THS-on · 2022-11-10T10:01:49Z

It fails in Centos 8 stream. From what I saw here quickly, the tpm2-tools version there (4.1.1) does not have tpm2_eventlog, so the test fails.

Yeah you are right, so the test can just add a skip if the the TPM version is below 4.1.1

kkaarreell · 2022-11-10T10:13:11Z

Correct, in E2E tests we are disabling measured boot tests for C8S.

ansasaki · 2022-11-10T10:50:58Z

I'll create a PR to skip the measured boot log parsing test for old tpm2-tools versions

THS-on · 2022-11-10T12:09:28Z

@ansasaki thank you. @sergio-correia I'll merge this PR now, because the failure has nothing to do with this PR.

sergio-correia mentioned this pull request Nov 7, 2022

Identity quote failure when using TPM devices with manufacturer certificates signed by ECC keys #1070

Closed

github-advanced-security bot found potential problems Nov 7, 2022

View reviewed changes

sergio-correia marked this pull request as draft November 7, 2022 15:20

THS-on requested changes Nov 7, 2022

View reviewed changes