Support allowing specific unknown critical extensions

[samuel40791765]

Issues:

Resolves V1726629971

Description of changes:

As of today, AWS-LC returns an error as part of verification if there are any critical extensions present in the certificate being validated. There have been asks to set a custom OID on the issued certificates to ensure that additional validation is performed by customers after or during the handshake. The intention is to prevent accidental mis-use of these certificates without that extra validation.

To support this, we've decided to add two new APIs for this use case.

1. X509_STORE_CTX_add_custom_crit_oid adds an oid as an ASN1_OBJECT to the list of "known" critical extension OIDs in ctx. Typical OpenSSL/AWS-LC behavior returns an error if there are any unknown critical extensions present within the certificates being validated. This function lets users specify custom OIDs of any critical extensions that are within the certificates being validated, that they wish to allow. The callback mechanism enabled with X509_STORE_CTX_set_verify_crit_oids must be set for this feature to enabled.

2. X509_STORE_CTX_set_verify_crit_oids enables the X509_STORE_CTX_verify_crit_oids_cb with X509_STORE_CTX. Consumers should be performing additional validation against the custom extension oids after or during the handshake. This callback forces users to validate their custom OIDs when processing unknown custom critical extensions. The X509_STORE_CTX_verify_crit_oids_cb callback function gives the user the current certificate being validated as x509 and a stack of ASN1_OBJECTs representing unknown critical extension OIDs that were found in x509 and match those previously registered via|X509_STORE_CTX_add_custom_crit_oid as oids.

This should not effect any existing consumers of X509_verify_cert. Any existence of an unknown critical extension will still cause the entire verification to be aborted. Only consumers that have enabled the callback and set specific OIDs with ASN1_OBJECT can circumvent the check and trigger the verification to pass.

Testing:

Test certs were generated by the team asking for this feature.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 91.59664% with 10 lines in your changes missing coverage. Please review.

Project coverage is 78.78%. Comparing base (d34e08b) to head (eb9ba6e).

Files with missing lines	Patch %	Lines
crypto/x509/x509_vfy.c	86.66%	8 Missing ⚠️
crypto/x509/x509_test.cc	96.61%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2377      +/-   ##
==========================================
+ Coverage   78.76%   78.78%   +0.02%     
==========================================
  Files         620      620              
  Lines      107961   108076     +115     
  Branches    15330    15349      +19     
==========================================
+ Hits        85032    85151     +119     
+ Misses      22272    22265       -7     
- Partials      657      660       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Mark-Simulacrum]

I'm not sure how I feel about this callback shape. A caller setting this to validate the oid list they also configured is still the same feels relatively useless. A caller setting this to validate the rest of the certificate as part of the AWS-LC validation is plausible, but also not particularly necessary -- since they can just do their validation after or before the AWS-LC validation.

For example, s2n-tls already has a similar verification callback (https://github.com/aws/s2n-tls/blob/1c5798b82442067bace943f748f4f24ae1770bed/api/unstable/crl.h#L175) that's also a bit more powerful (exposes the full cert chain and optionally allows asynchronous validation).

I'd personally expect s2n to just set this callback to always return true and then apply any validation it wants to separately from the AWS-LC X509_verify call. So maybe we should just drop it from the interface to avoid the extra complexity?

Let's get s2n thoughts before any decision, of course.

[goatgoose]

Yeah, I agree. I'm not opposed to the callback, but I think we're planning on having s2n-tls consumers just set the list of OIDs and do their custom verification in our existing cert verification callback or after the handshake, so we'd likely implement this how Mark described and always return true here. Even if we were to expose a similar callback in s2n-tls, I'd be hesitant to forward the AWS-LC callback for this purpose since I think we'd struggle to make it async compatible if we ever needed to.

[samuel40791765]

We initially decided to do a callback because we're not just going to be exposing this feature to only s2n-tls users, but to general users of AWS-LC as well. Since this feature is actually relaxing a check against unknown critical extensions, there's the caveat that we're relaying on the consumer to do additional validation against their OIDs. We do trust that s2n-tls will be doing the correct validation during/before/after the handshake.
However without the callback, there's not a particular mechanism in AWS-LC enforcing general consumers of the feature to actually do their OID validation. We want to put the fate of this check relaxation into the user's hands, so that they understand what they're doing.

I do agree that there's a redundancy on with the X509_V_FLAG_ALLOW_CUSTOM_CRIT_EXT flag vs callback though. Ideally whatever we decided upon, I'd like to just keep 1 of either. If we decided against a callback to enforce validation, there has to be explicit documentation in AWS-LC asking the consumer to be aware of what they're doing and there's the slight concern that the general consumer may overlook this.

[Mark-Simulacrum]

Perhaps we could call the callback with any unknown critical extension (taking a single OID as the argument) and then s2n-tls could implement that with its own comparison logic? That would make the API be just the callback -- arguably more complicated than necessary, but avoids some of the redundancy at least...

[samuel40791765]

True that certificate verification would still fail if encountering an unknown critical extension and the callback would force the checking mechanism into the users hands. I do have the concern where there's a more dangerous scenario where a consumer could just simply set the callback to return 1 and circumvent all checks instead.

Do let me know what you folks think about the current iteration, I've removed the flag in the current PR iteration so this is a clearer example of what we were initially envisioning. s2n may be looking apply any validation it wants to separately from the call to X509_verify_cert, but the verification call was meant to be a single function that tells the consumer the entire chain is OK. Validation logic should be consolidated within X509_verify_cert for most general consumers.

[skmcgrail]

It might be useful for the callback to be passed the subset of critical extensions that were found on the certificate in question rather then the whole list? I feel like you partially formulate what this is in check_custom_known_critical_extension but you don't store and give it to the user.

Otherwise right now the author of the callback is given the fullset of OIDs and they have to then filter and search which ones are applicable for the passed in certificate.

[smittals2]

is ctx->custom_crit_oids not the subset already? Or can ctx->custom_crit_oids have certain custom extensions that are not applicable to the given cert x (and might be to another cert in the chain), so you are recommending filtering further here?

[samuel40791765]

ctx->custom_crit_oids contains all of the custom critical oids that the user has set. I believe @skmcgrail's asking me to change some logic here to return only the custom critical oids that were found for this specific cert.

Agreed that this would be more valuable than a list the user has set themselves. I'll rework some of the logic around here to do that.

[skmcgrail]

We should clear this EXFLAG_CRITICAL flag off after this if case. It's kind of a misnomer of a name but the purpose of the flag indicates that there is unknown critical extension unhandled. If we don't exit out here at this point we should remove it cause that implies no extensions are unknown at that point on the certificate.

[smittals2]

Can we also add a comment here about this maybe, I was also confused at first. I read the if condition as "if IGNORE_CRITICAL is not set, and EXFLAG_CRITICAL is set (which to me implied any critical extensions, not just unhandled ones), AND either check_custom_known_critical_extension or ctx->verify_crit_oids fails", we are in an error state.

Since check_custom_known_critical_extension fails immediately if ctx->custom_crit_oids is NULL, if EXFLAG_CRITICAL were to mean any critical ext and not just unhandled ones, this if condition would be erroneous.

Just to confirm for my own clarity, at this point in code, are we expecting that any "known" critical oids have already been parsed/validated. So if EXFLAG_CRITICAL is still set here, we know that it's only possible because the customer has set custom oids and can therefore fail otherwise?

[samuel40791765]

I was a bit hesitant to do so, mainly because the state of EXFLAG_CRITICAL is maintained along with the x certificate. If that state is cleared, there is now an implication that critical extension is supported and the certificate can be accepted. I was initially afraid of the user not being aware that there were typically unknown critical extensions present in the cert. But if they're already running X509_verify_cert with this feature, I guess it would make sense that that state is cleared later on.

[samuel40791765]

EXFLAG_CRITICAL simply indicates an unsupported critical extension, so a critical extension that is already supported wouldn't trigger this. This flag is added during the initial parsing of the certificate.

check_custom_known_critical_extension is supposed to fail under the circumstance you mentioned. Typical OpenSSL/AWS-LC behavior is to fail when an unsupported critical extension is found (hence the flag). The user has to set up ctx->custom_crit_oids with all their "known" unsupported critical extensions they wish to work around here.

At this point in the code, we only know if there are unknown critical extensions present with the EXFLAG_CRITICAL flag. We could remove the flag after the first validation against this has succeeded, but I'd prefer to do the custom OID validation regardless of whether the flag has been set.

[smittals2]

Thank you for those details, that clears up a lot for me! It would be great if we can add a comment here explaining EXFLAG_CRITICAL/what we are checking to improve readability in this func :)

[samuel40791765]

No problem, I think it's valuable. Will do :).

[smittals2]

nit: Array, should be stack?

[smittals2]

maybe null_verify_custom_crit_oids_callback instead?

[smittals2]

General question/comment around naming convention here. I'm seeing both "custom_crit" and "_custom_known_crit_" in the code. Is there a distinction between the two that makes a given custom crit oid "known", or should this naming be standardized?

[samuel40791765]

General question/comment around naming convention here. I'm seeing both "custom_crit" and "custom_known_crit" in the code. Is there a distinction between the two that makes a given custom crit oid "known", or should this naming be standardized?

Sorry for the confusion, I was fighting myself with using custom_crit and custom_known_crit since the exported function names were getting a bit too long in the initial drafts... Both mean the same thing, I believe we'll be moving forward with custom_crit, so I'll change all places to reflect that.

[samuel40791765]

No problem, I think it's valuable. Will do :).