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

Remove MBEDTLS_X509_CHECK_*_KEY_USAGE #4405

Closed
chris-jones-arm opened this issue Apr 23, 2021 · 3 comments · Fixed by #4619
Closed

Remove MBEDTLS_X509_CHECK_*_KEY_USAGE #4405

chris-jones-arm opened this issue Apr 23, 2021 · 3 comments · Fixed by #4619
Assignees
Labels
component-x509 enhancement size-s Estimated task size: small (~2d)

Comments

@chris-jones-arm
Copy link
Contributor

chris-jones-arm commented Apr 23, 2021

Context

There are two configuration options, MBEDTLS_X509_CHECK_KEY_USAGE and MBEDTLS_X509_CHECK_EXTENDED_KEY_USAGE, which are used to enable the library's verification of the keyUsage and extendedKeyUsage fields of an x509 certificate. This verification is an important step and disabling it can cause security issues however it is possible to either check or not check key usage at runtime (via the callback parameter in mbedtls_x509_crt_verify) as opposed to compile time.


Rationale

There is no reason to provide redundant compile time options when they can be equivalently done at at runtime with even more flexibility.


Work items for 3.0

In this case because both options are enabled by default we want to ensure that when removing the options verification of the key usage fields is still done by default. A user can disable the verification by using a callback which clears the corresponding flags when they've been set.

  • Remove MBEDTLS_X509_CHECK_KEY_USAGE and MBEDTLS_X509_CHECK_EXTENDED_KEY_USAGE from the configuration.
  • Ensure the library still performs the verification by default.
  • Ensure the changelog/migration guide is updated to give the migration path for users.
  • By the end git grep should return no instances of either option.
@mpg
Copy link
Contributor

mpg commented Apr 26, 2021

There is no reason to provide redundant compile time options when they can be equivalently done at at runtime with even more flexibility.

I'm not sure I'm fully convinced by this rationale: a reason could be to save code size. According to my measurements (arm-none-eabi-gcc -Os -mcpu=cortext-m0) each option is about 200 bytes of code size. In my book that's a bit of a grey area: is that enough to matter?

OTOH, I agree that those are security checks, so there's something to be said for always including them. Also, in terms of testing, runtime options are more convenient than compile-time options I think.

So, overall I'm a bit on the fence on this one.

A user can disable the verification by using a callback which does not check these fields.

I think that should be "a callback which clears the corresponding flags when they've been set".

@gilles-peskine-arm
Copy link
Contributor

In what scenarios would it be ok to not check the (extended) key usage? I guess when your root CA defines a special-purpose PKI where all certificates are for a specific usage anyway?

I'm trying to determine whether security is a reason to have these options always turned on, or at least to make users jump through hoops (possibly including a code size cost) if they want to bypass the checks.

@mpg
Copy link
Contributor

mpg commented Apr 27, 2021

Yes, I think the only case where you don't need to check those fields is when certificate usage is part of your PKI design - which only happens if (1) you control the entire PKI and (2) you designed it well.

Now, I can easily imagine scenarios where (1) is true at the beginning of the project but things change down the line. (I don't need to tell you that people sometimes make plans based on optimistic assumptions that don't always hold over time ;) ) In that case, there's a risk that people initially disable those options and then forget to enable later when things change.

Well, thinking while writing: if you control the entire PKI then you can choose to either (a) set these extensions to appropriate values in all certificates, or (b) omit them from all certificates - and then no checks will be done.

So, I don't really think there's ever a good reason to want to bypass the checks, so we shouldn't make that easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-x509 enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants