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

Should we mark evidence extension as critical? #9

Open
imlk0 opened this issue Feb 7, 2023 · 11 comments
Open

Should we mark evidence extension as critical? #9

imlk0 opened this issue Feb 7, 2023 · 11 comments

Comments

@imlk0
Copy link
Contributor

imlk0 commented Feb 7, 2023

The current definition of evidence extension is:

- Evidence (required): X.509 cert extension, a byte string of encoded tagged CBOR data
- OID: `tcg-dice-tagged-evidence` (2.23.133.5.4.9)
- CBOR tag: a CBOR tag registered with IANA or some other registry serves as evidence format ID
- CBOR data: this data covered by the tag contains the evidence (including custom claims). Its format is defined by the CBOR tag.

And, accroding to TCG DICE specification draft, the criticality flag of evidence extension should be marked critical.
However, in practice, some TLS libraries may not have a suitable api to handle a custom extension of the critical attribute. For example, openssl does not provide a way to get the custom extension that causes the X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION error.

So maybe we can mark the extension as non-critical, for now.

Any comments are welcome.

@imlk0 imlk0 changed the title The criticality flag of evidence extension should be marked critical Should we mark evidence extension as critical ? Feb 14, 2023
@imlk0 imlk0 changed the title Should we mark evidence extension as critical ? Should we mark evidence extension as critical? Feb 14, 2023
@dimakuv
Copy link

dimakuv commented Feb 14, 2023

I second what @KB5201314 wrote.

The mbedtls library used by Gramine also has the limitation of "fail on unrecognized critical cert extension": https://github.com/Mbed-TLS/mbedtls/blob/ab1f3c153a90d6943c174613b715d19dfddfc83a/library/x509_crt.c#L847-L851

The rationale for this can be read here: https://github.com/Mbed-TLS/mbedtls/blob/development/docs/3.0-migration-guide.md#remove-the-config-option-mbedtls_x509_allow_unsupported_critical_extension

@jiazhang0
Copy link

jiazhang0 commented Feb 14, 2023

My thoughts are:

  • From a long term, this extension should be marked as "critical", because finally this extension which can reflect the device identity of peer TEE would be popular. So a future browser as tls client must verify this extension for security purpose (and thus show a new icon in the URL bar similar to HTTPS icon).
  • From a short term, this extension should be marked as "non-critical" as a recommendation documented in practical interoperable ra-tls spec for backward compatibility. For example, a web server running in TEE can issue a tls certificate with this extension, but we don't expect to see the failure if a tls client doesn't support this extension.
  • In addition, for the caller of mbedtls, such as gramine, I suggest gramine explicitly implement the callback for this extension from a long term. In fact, no matter how this extension is marked, gramine's code doesn't need to be modified again. @dimakuv @KB5201314

@shnwc
Copy link
Contributor

shnwc commented Feb 17, 2023

Agree that if TLS libraries (like openssl or mbedtls) do not support extracting critical extensions, then we will have to mark the quote extension to non-critical for now.

But semantically this extension should be critical, a verifier that can't process the quote should fail.

For reference, openenclave implements RA-TLS on both openssl and mbedtls. As implemented in oe_verify_attestation_certificate(), as part of cert verification, it calls oe_cert_find_extension() to extract the quote extension and verifies the quote. There are separate openssl and mbedtls implementations of oe_cert_find_extension(). Will these implementations fail if the quote extension is marked critical?

@dimakuv
Copy link

dimakuv commented Feb 20, 2023

There are separate openssl and mbedtls implementations of oe_cert_find_extension(). Will these implementations fail if the quote extension is marked critical?

I didn't look deeply how they are implemented in OE, but in general yes -- OpenSSL and mbedTLS first call their own "parse and verify" logic on the certificate, and only afterwards they call custom verification callbacks.

Well, it should be easy to check how OE behaves on unknown critical extensions. Maybe one of the OE devs can do such an experiment?

@shnwc
Copy link
Contributor

shnwc commented Feb 22, 2023

I didn't look deeply how they are implemented in OE, but in general yes -- OpenSSL and mbedTLS first call their own "parse and verify" logic on the certificate, and only afterwards they call custom verification callbacks.

@dimakuv Agree with your assessment. Let's put a note in the design document that the criticality flag of this extension will be cleared.

imlk0 added a commit to imlk0/rats-tls that referenced this issue Feb 22, 2023
Mark evidence extensions as non-critical, since major TLS libraries (such as openssl or mbedtls) do not support extracting critical extensions.

See: CCC-Attestation/interoperable-ra-tls#9

Signed-off-by: Kun Lai <[email protected]>
imlk0 added a commit to imlk0/rats-tls that referenced this issue Feb 22, 2023
Mark evidence extensions as non-critical, since many TLS libraries (such as openssl or mbedtls) do not support extracting critical extensions.

See: CCC-Attestation/interoperable-ra-tls#9

Signed-off-by: Kun Lai <[email protected]>
@shnwc
Copy link
Contributor

shnwc commented Feb 24, 2023

As commented in PR #11, @KB5201314 @dimakuv please let us know when you have a chance to investigate the options discussed there. The best is to be able to keep the criticality flag set.

@DemiMarie
Copy link

The mbedtls library used by Gramine also has the limitation of "fail on unrecognized critical cert extension": https://github.com/Mbed-TLS/mbedtls/blob/ab1f3c153a90d6943c174613b715d19dfddfc83a/library/x509_crt.c#L847-L851

You can work around this with a custom callback.

There are separate openssl and mbedtls implementations of oe_cert_find_extension(). Will these implementations fail if the quote extension is marked critical?

I didn't look deeply how they are implemented in OE, but in general yes -- OpenSSL and mbedTLS first call their own "parse and verify" logic on the certificate, and only afterwards they call custom verification callbacks.

At least mbedTLS allows custom callbacks.

imlk0 added a commit to imlk0/rats-tls that referenced this issue Feb 27, 2023
Mark evidence extensions as non-critical, since many TLS libraries (such as openssl or mbedtls) do not support extracting critical extensions.

See: CCC-Attestation/interoperable-ra-tls#9

Signed-off-by: Kun Lai <[email protected]>
@dimakuv
Copy link

dimakuv commented Feb 27, 2023

@DemiMarie Thanks, I didn't know this callback exists.

I will experiment with mbedtls_x509_crt_parse_der_with_ext_cb() and will report on my results here.

haosanzi pushed a commit to inclavare-containers/rats-tls that referenced this issue Feb 27, 2023
Mark evidence extensions as non-critical, since many TLS libraries (such as openssl or mbedtls) do not support extracting critical extensions.

See: CCC-Attestation/interoperable-ra-tls#9

Signed-off-by: Kun Lai <[email protected]>
@dimakuv
Copy link

dimakuv commented Feb 27, 2023

@DemiMarie @shnwc I don't think the mbedtls_x509_crt_parse_der_with_ext_cb() callback helps us.

See my explanation in this freshly opened GitHub issue: Mbed-TLS/mbedtls#7182

@DemiMarie
Copy link

@dimakuv is it possible to turn of Mbed TLS’s own certificate verification and do everything manually? I had to make this change in rustls to make it suitable for use with libp2p-quic.

@dimakuv
Copy link

dimakuv commented Feb 27, 2023

@dimakuv is it possible to turn of Mbed TLS’s own certificate verification and do everything manually?

I do not know of such a switch/config in mbedTLS. Of course, one could just use a different TLS library on top of mbedTLS's libmbedcrypto.so (low-level crypto) and libmbedx509.so (X.509 certs), but this sounds like an overkill.

I hope that mbedTLS gurus will propose some solutions in Mbed-TLS/mbedtls#7182.

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

No branches or pull requests

5 participants