Allow mbed library to ignore critical extensions

[dcleblanc]

Notes:

• Pull requests cannot be accepted until the PR follows the contributing guidelines. In particular, each commit must have at least one Signed-off-by: line from the committer to certify that the contribution is made under the terms of the Developer Certificate of Origin.
• This is just a template, so feel free to use/remove the unnecessary things

Signed-off-by:` David LeBlanc

Description

Many cert authorities issue CRLs with the issuingDistributionPoint extension, which must be critical. Without this fix, this library cannot parse many commonly seen CRLs. This fixes this issue - #2605

Status

READY/IN DEVELOPMENT/HOLD

Requires Backporting

I would backport this, since it means that many consumers can't do revocation checking.

Which branch?
Don't know, up to maintainers.

Migrations

This is opt-in, so no migration needed

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

Set the new #define, and then parse a DigiCert CR, see that it works.

[gilles-peskine-arm]

I'm willing to be overridden, but I'd rather not add yet another compilation option, and I'd rather not add a compilation option that can considerably weaken security. MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION is somewhat deprecated (and we should perhaps officially deprecate it?). It's more of a quick hack than a serious feature, and we're likely to remove it in Mbed TLS 3.0.

Fortunately now we have a better mechanism: mbedtls_x509_crt_parse_der_with_ext_cb, which allows passing a callback function on a call-by-call basis and selecting which critical extensions are ignored. (See #3243 and its follow-up #3419.) I think we should add a similar interface for CRL.

I'm not sure what to do for LTS branches. We normally don't add new features in LTS branches because they add risk and increase the code size.

[dcleblanc]

Let's look at what extensions we're concerned with:

1. Authority Key Identifier - the library should be parsing this, it's more efficient than comparing issuer names. It's not critical, but a conforming issuer MUST issue it.
2. CRL number -for CRLs which have the same issuer, this tells us which to use, and that's important. The library should use this, but does not. Not doing this is a security flaw.
3. Delta CRL indicator - delta CRLs tend to be rare, but by ignoring this extension, the library could do the wrong thing.
4. issuingDistributionPoint - this is what's causing the problem. The reason that this is critical is because if the onlySomeReasons field is present, then it informs how we validate the following revocation list. It's unusual to see this field, but if it were there, and it was ignored, it would be a problem. Where it is commonly used is for the distributionPoint field, which isn't really critical, just helpful URL about where the CRL came from.

Your proposed callback mechanism would be a challenge because how are we to inform the subsequent revocation entry parsing of the implications of the onlySomeReasons field of the issuingDistributionPoint extension? It also leaves to the user the responsibility of writing parsers for well-known, very common extensions.

In terms of security implications, we're now completely unable to parse CRLs from many major CAs, and thus unable to use this library for revocation checking, which is a much larger security problem than not doing quite the right thing with some obscure fields rarely seen in the field.

This fix is also very low risk - the default is to maintain the current behavior, and if the user wants to assume responsibility for any downsides, that's up to them.

[CodeMonkeyLeet]

@gilles-peskine-arm As indicated in #2605 this is a blocker for any organization that needs to deal with DigiCert CRLs using mbedTLS. Given that mbedTLS 3.0 is targeted for 2021 and any suggested mbedtls_x509_crl_parse_der_with_ext_cb is a much more heavyweight feature addition that will not be backported, this PR seems like a reasonable compromise for the existing LTS branches: minimal code change and disabled by default, with no impact on existing usage without explicit opt-in.

[hanno-becker]

Hi @dcleblanc and @CodeMonkeyLeet,

Thanks for your detailed comments, it's very helpful and greatly appreciated.

A small note on the criticality of the issuingDistributionPoint extension from RFC 5280:

Although the extension is
critical, conforming implementations are not required to support this
extension. However, implementations that do not support this
extension MUST either treat the status of any certificate not listed
on this CRL as unknown or locate another CRL that does not contain
any unrecognized critical extensions.

To my knowledge, Mbed TLS does currently not differentiate between (a) a suitable CRL not being present for a given CA and the CRT in question thus not being considered as revoked ("unknown" status), and (b) a suitable CRL being present for a given CA, and the CRT in question not being considered revoked because it's not listed in the CRL.

Introducing this distinction would be an API change. It's something we might want to consider for Mbed TLS 3.0, but nothing we can do at the moment. Given that the current API treats CRTs with "unknown" revocation state as "valid", I think it's acceptable to effectively treat the issuingDistributionPoint extension as non-critical.

Now to the mechanics:

I don't object to the introduction of a sibling of MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION for CRLs: This latter option is already present for CRTs, and should we decide that it ought to be removed in 3.0 and replaced by an alternative mechanism, we can apply this to CRLs as well. So, as a short-term fix, I can live with this PR.

Alternatively, one could register a callback for CRL extension parsing which does nothing but ignore issuingDistributionPoint extensions. The benefit of this approach over MBEDTLS_X509_CRL_ALLOW_UNSUPPORTED_CRITICAL_EXTENSIONS is that it doesn't automatically and perhaps unwillingly apply to other critical extensions. Since the introduction of this CRL extension parsing callback would again be parallel to what we already have for CRTs, I don't object to it from a design perspective. Also, I don't think it's a "heavyweight" change - given the precedent for CRTs, it should be pretty straightforward to implement. Finally, note that using such a callback to ignore specific extensions is simpler than asking users to actually add support for specific, currently unsupported extensions. I agree that it's Mbed TLS' job to support, say, the CRL number extension.

Overall, I prefer the addition of a CRL extension parsing callback parallel to the one we have for CRTs, and I'd be happy to provide a PR for it.

@mpg I'd be interested in hearing your opinion on this as well.

Thanks again @dcleblanc and @CodeMonkeyLeet for your input and for letting us know about the criticality of this (no pun intended), I hope we can turn it around quickly in some way,

[dcleblanc]

Having a callback is not a bad thing - I have a different project that can also parse CRLs, and I fed it around 500 CRLs from the top web sites, and what I found commonly was:

authorityKeyIdentifier
cRLNumber
issuingDistributionPoint

And also a couple of Microsoft-specific extensions - a callback would be handy to manage these, if someone wanted. Though I don't think anything really uses them, even Microsoft code (though I have not looked).

Out of the above, I would recommend just implementing (in later versions) the first two - the structures are really simple. The first would allow you to get better perf when matching a cert issuer to a CRL. The second is important in case someone gives you two CRLs for the same issuer, you need to know which to use first. That's not practically managed with a callback.

I was surprised to never see any delta CRLs, so I'd prioritize support for those less highly.

I don't think it is practical to implement support for the issuingDistributionPoint in a callback, so let's take a look at the fields:

distributionPoint - just helpful information, and typically why this extension is there.

onlyContainsUserCerts
onlyContainsCACerts
onlyContainsAttributeCerts

I don't think any of these are common, they have defaults, and processing them gets into being picky about things. If I were you and triaging, I don't think I'd support them.

onlySomeReasons - this one gets a bit more complex, and implies that you'd support reason codes for revocation - don't know if you do, haven't looked. Because this affects how you'd parse the revocations, this really can't easily be supported in a callback, IMO. I have no idea how common this is.

indirectCRL - this implies support for the revocation entry extension that corresponds to this, and suspect that this would be rare.

So my recommendation would be to support these three directly, and have the callback in case anything unusual or vendor-specific shows up. But that's all going forward - for now, I just need to be able to use DigiCert CRLs to do revocation checking, even if doing so isn't as tidy as it could be.

[hanno-becker]

I don't think it is practical to implement support for the issuingDistributionPoint in a callback, so let's take a look at the fields

The proposed short-term fix wouldn't be to add support for issuingDistributionPoint, but rather to use a callback to selectively ignore this extension, albeit it being critical.

Would that work for you?

[dcleblanc]

For 2.7, we prefer the #define as in the PR. Going forward, for the 3.0 code, a callback would be OK. I'm just recommending that the library ought to just directly support a limited subset, but that's future for us, and don't have strong opinions.

[CodeMonkeyLeet]

@dcleblanc @hanno-arm Just for clarification, the OE SDK uses the 2.16 LTS, which would be the preferred backport target.

[dcleblanc]

@CodeMonkeyLeet - yes, thank you for the correction

[gilles-peskine-arm]

MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION leaves all the work to the application anyway. So we might as well do it in a clean way where the application gets to handle the extensions it needs individually, rather than make it do all the work including what the library already supports.

Separately, the library should support commonly used extensions.

The Mbed TLS team isn't likely have time to code these features any time soon, but we'd be glad to review and merge these features if you contribute them (including tests).

For LTS branches, we need to minimize risk and code size increase. MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION fulfills these requirements as far as the library is concerned, but I'm not sure it's the right thing for the ecosystem in general since it means everyone who needs these extensions has to code for them on their own.

[CodeMonkeyLeet]

@gilles-peskine-arm Regarding the LTS branches and the ecosystem, I think the proposed fix still offers an incremental improvement for the ecosystem, since the alternative is that mbedTLS simply fails outright when attempting to handle CRLs from specific authorities. In the specific use case of issuingDistributionPoint as analyzed by Hanno, ignoring the extension without explicitly dealing with it is still preferable to not handling revocation of the CRL at all.

[mpg]

In terms of security implications, we're now completely unable to parse CRLs from many major CAs, and thus unable to use this library for revocation checking, which is a much larger security problem than not doing quite the right thing with some obscure fields rarely seen in the field.

I think that's a pretty strong point, which IMO shows that we need to do something and that a solution doesn't need to be perfect to be significantly better than the status quo. Let's try to map the solution space:

1. The ideal solution would obviously be for the library to be able to parse and properly handle the issuingDistributionPoint extension (and any common extension that's critical, but this appears to be the only one so far).
2. A partial solution would be for the library to be able to parse that extension, but not fully handle it, and fail in a safe way it cases it doesn't handle (for example, fail if the onlySomeReasons is present, and I understand the other fields could be ignored).
3. Another solution, that's more partial in some ways, and more general in other ways, is to allow for a callback function to be called during parsing for each unsupported extension. Unfortunately, it doesn't really allow doing much with the extension beyond ignoring it.
4. Finally, there's MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION (this PR).

I think (4) is the only one that's acceptable for LTS branches: we normally don't backport new features to LTS branches, but considering this one is opt-in and doesn't add any new code, and can be useful for security, I think it's acceptable.

For the development branch, I think I'd prefer (3) (or 2 or 1 obviously): as Gilles says, it's less blunt, and allows users to make sure they only ignore expected extensions rather than all of them.

And obviously, if at any point you feel like contributing support for any of the common extensions mentioned in this thread, that would be most welcome!

[mpg]

Note: if we add such an option:

• it would need to be excluded from the full target in scripts/config.py (in LTS branches, that would be scripts/config.pl)
• we would need a component in tests/scripts/all.sh that builds and runs the test suites with that option enabled
• we'd probably need to add !MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION on the depends_on line of some existing tests in tests/suites/test_suite_x509parse.data
• we'd also want at least one new test in the same file that depends on this option being enabled and demonstrates that unsupported critical extensions are indeed ignored in that case.

[hanno-becker]

I think (4) is the only one that's acceptable for LTS branches: we normally don't backport new features to LTS branches, but considering this one is opt-in and doesn't add any new code, and can be useful for security, I think it's acceptable. For the development branch, I think I'd prefer (3) (or 2 or 1 obviously): as Gilles says, it's less blunt, and allows users to make sure they only ignore expected extensions rather than all of them.

If we choose approach (4), I prefer to uniformly add MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION for development, 2.16 and 2.7, and to leave the implementation of (1),(2) or (3) on development for whenever we or a contributor will have time for it.

I don't like the idea of having MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION on 2.16 and 2.7 only, because it would harden upgrade from 2.16 to more recent versions of Mbed TLS.

[hanno-becker]

I think (4) is the only one that's acceptable for LTS branches:

I disagree with that: If we justify the addition of MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION to LTS branches because we see the status-quo as a security issue, then I think the CRL callback is a more robust way to address it, because MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION itself can lead to security issues.

My overall preference is still the addition of some mechanism to development, 2.16 and 2.7 which allows to selectively ignore specific critical CRL extensions. CRL callbacks are one such mechanism, which moreover parallels the existing CRT callbacks. Another mechanism would be a compile-time configured list of OIDs for critical extensions which may be ignored.
Both would be straightforward to implement.

[mpg]

You're making a good point that it's probably better enough uniformity to allow easy migration from 2.16.x to development. Actually, now that you're mentioning it, that's actually part of our API stability promise: migrating from any 2.x version to any later 2.* version should not require modifying application code.

I think (4) is the only one that's acceptable for LTS branches:
I disagree with that: If we justify the addition of MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION to LTS branches because we see the status-quo as a security issue, then I think the CRL callback is a more robust way to address it, because MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION itself can lead to security issues.

Well the difference is, adding the callback adds new code, which we tend to avoid in LTS branches. However there is indeed a precedent for that with the fix for ecp_mul() called without a RNG (but with a way to opt out in config.h: MBEDTLS_ECP_NO_INTERNAL_RNG). So I should probably have been more nuanced: 4 is the most acceptable, and the more a solution adds new code, the harder it is to justify adding it in the LTS branches.

[hanno-becker]

Well the difference is, adding the callback adds new code, which we tend to avoid in LTS branches.

I think it should be less about not adding new code to LTS branches, but rather about avoiding any code modification unless necessary for special reasons, such as addressing bugs or security issues. And disabling a piece of code, as MBEDTLS_X509_ALLOW_UNSUPPORTED_CRL_CRITICAL_EXTENSION does, is already such a code modification.

[mpg]

While I agree that we want to be careful about any modification in the LTS branches, I maintain my point that adding code is a particular issue for LTS branches, because want the code footprint to remain a stable as possible there. That's one of the goals of LTS branches: you have a devices deployed that have barely more flash than needed to fit the current firmware, and you want to be able to deploy security updates.

But I agree that, contrary to what I said earlier, that doesn't fully prevent us from adding a callback in the LTS branches is we think that's the best solution to this problem. It's just what we don't want to get into the habit of adding new code in the LTS branches too lightly.

[hanno-becker]

@dcleblanc After short internal discussion, our view is that best short-term solution is the introduction of a CRL callback, analogous to the existing CRT extension callback introduced in #3243. This mechanism can then be instantiated with a callback silently ignoring the issuingDistributionPoint extension. This is more robust than MBEDTLS_X509_CRL_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION since it doesn't come at the danger of sillently ignoring other critical extensions.

Would you be able to contribute a PR + Backports introducing this mechanism?

[dcleblanc]

Sorry, my management won't allow me to do more work on this right now. If I get a chance in off hours, I may be able to do something more thorough.