Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement 101 for cryptography package requirements #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Isaac-Matthews
Copy link

Increase the package requirement for the cryptography package to enable processing of RSA-PSS signatures.
cryptography >= 37.0.0

@stefanberger
Copy link
Contributor

I think the issue with requiring a more recent version of python-cryptography is related to older distros, such as CentOS 8 where, iirc, compiling python-cryptography requires Rust to be installed but it may not be available.

A possibility would be to runtime-check the installed version of cryptography like this and log the error if the version doesn't meet the requirements and fail the verification.

>>> import cryptography as c
>>> print(c.__version__)
37.0.2

What is the advantage of using IDevID and IAK over the existing EK certificate and vendor CA cert method?
Are you going to provide a command line option to the Rust client to pick the one rather than the other?

@stefanberger
Copy link
Contributor

Hm, it looks like PSS padding has been used for signature verification for quite some time:

https://github.com/keylime/keylime/blob/c16ec1656c28c1e60f62ff69abdb223fbf549b34/keylime/crypto.py#L84-L97

Even though python-cryptography may have added something regarding to PSS just recently, it has also existed for much longer:

https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst#06---2014-09-29

Do you really need anything that's not been provided by the library for a long time?

Here's the PSS code from python-cryptography 0.6:

https://github.com/pyca/cryptography/blob/0.6.x/cryptography/hazmat/primitives/asymmetric/padding.py#L28

@Isaac-Matthews
Copy link
Author

Thanks for your comments.

The advantage of using IDevID over EK is that IDevID should identify a device as opposed to just a TPM, and should contain the serial number of a device being registered. It also enables the possibility of using a single round trip to register. Ideally both should give way to LDevID/LAK so that we aren't using long lived keys. There is more of a discussion of motivation and plans in #81. The choice would be config options for the rust agent.

For PSS specifically, just using the PSS.MAX_LENGTH for the salt does not work for the signatures, the AUTO attribute is required. I believe it is possible to calculate the salt length from the signature but I can't find a source for how that is actually done, and I thought it would be better to use an already supported package to accomplish this instead.

@stefanberger
Copy link
Contributor

For PSS specifically, just using the PSS.MAX_LENGTH for the salt does not work for the signatures, the AUTO attribute is

   try:
        public_key.verify(
            base64.b64decode(signature),
            message,
            padding.PSS(mgf=padding.MGF1(hashes.SHA256()), salt_length=padding.PSS.MAX_LENGTH),
            hashes.SHA256(),
        )

This is the code we seem to have been using. I would also find it odd that PSS support has been there since python-cryptography v0.6 but it cannot be used for verification. Have you tried it like this?

@Isaac-Matthews
Copy link
Author

Yes I have tested with that but for some reason using the salt_length as MAX_LENGTH does not work. It works when using it as AUTO, but also works if I use an integer. I had assumed using AUTO would be the safer approach but if the rust requirement is problematic I can just hard code the numbers myself?

@stefanberger
Copy link
Contributor

The advantage of using IDevID over EK is that IDevID should identify a device as opposed to just a TPM,

I think for our purposes it's most important to know that we are talking to a TPM that can quote ...

@stefanberger
Copy link
Contributor

Yes I have tested with that but for some reason using the salt_length as MAX_LENGTH does not work. It works when using it as AUTO, but also works if I use an integer. I had assumed using AUTO would be the safer approach but if the rust requirement is problematic I can just hard code the numbers myself?

I think the easiest way would be for you to capture the data from your TPM and write a test case like these ones here so we can all look at it: https://github.com/keylime/keylime/blob/master/keylime/tpm/tpm_util_test.py

@THS-on
Copy link
Member

THS-on commented Sep 25, 2024

@Isaac-Matthews what is the state here? Do we need to update at some point or is it possible to work around it?

@Isaac-Matthews
Copy link
Author

This has been worked around here.

The cryptography version is checked, and if it is too low and IDevID is required all registrations will fail with this check as the block. (If IDevID is optional it will fall back to just using EK).

In my opinion we should update cryptography but it is not technically a requirement for this given the current runtime check. I don't think updating would add a rust requirement as it can be installed by wheel, and rust is only required when building from source.

If this isnt wanted I can close for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants