Allow unsupported extensions in runtime

[lstipakov]

Hello,

I work for OpenVPN, Inc and recently we got customer complaint that he is not able to connect with our client. He has CA which includes X.509 v3 extension which mbedTLS doesn't support - "Name Constraints".

Since mbedTLS indeed doesn't support "Name Constrains" (https://tls.mbed.org/discussions/feature-request/supported-for-x-509-name-constraints-extension) and has no plans to support it, we came up with a workaround which enables customer to use our product without implementing Name Constraints support. This workaround includes certain changes to mbedTLS, which I submit in this PR.

At the moment mbedTLS just fails certificate verification if it finds unsupported critical extension (https://github.com/ARMmbed/mbedtls/blob/development/library/x509_crt.c#L600). This behavior can be adjusted in compile-time by setting flag MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION. When this flag is set, mbedTLS allows any unsupported critical extensions and it becomes responsibility of a user to validate those and fail on ones which he cannot / do not want to validate.

I would like to present a different approach and enable user to allow specific extensions in runtime.

Here are extensions to an API:

• new mbedtls_ssl_conf_allow_unsupported_critical_exts accepts bit flags which represent unsupported critical extensions we want to enable in runtime - has an effect on remote certificate validation during handshake.

• new allowed_unsupported_critical_exts field to mbedtls_x509_crt structure, same meaning as above -
  has an effect on a certificate parsing outside of ssl context.

• new mbedtls_oid_get_x509_ext_type_supported method which translates both supported and unsupported extensions OID to a local value. Existing version mbedtls_oid_get_x509_ext_type does not work for unsupported extensions.

With this new API a user could enable certain extension in runtime (mbedTLS will still fail on other unsupported extensions) and parse this extension in verification callback.

I have included unit tests which verifies that:

• CA verification fails if it contains unsupported critical extension
• Verification doesn't fail if extension if enabled in runtime via new API

Please let me know what do you think.

I have signed CLA as a private person ([email protected]).

Status

IN DEVELOPMENT

Requires Backporting

NO

Todos

• [] Tests
• [] Documentation
• [] Changelog updated
• [] Backported

[simonbutcher]

Hi @lstipakov!

Thanks for the contribution, and thanks for signing the CLA! Unfortunately I can't find any reference to it in our registry. Can you tell me how you submitted it? And do you know if it was processed?

Thanks again!

[lstipakov]

Hi @sbutcher-arm ,

Here https://os.mbed.com/users/lstipakov/ it says "Lev Stipakov has accepted the Contributor Agreement." - I think I pressed a button on a website to accept it.

[simonbutcher]

Thanks for confirming the CLA! I was looking for a signed contract, not an Mbed account. My mistake.

[lstipakov]

Bump.

[dsommers]

Can we please get a feedback on this pull request? Is this something which can be considered for a future release or should we just drop the ball and look for a different solution?

--
kind regards,

David Sommerseth
Team lead, OpenVPN Core development

[simonbutcher]

Hi @lstipakov and @dsommers,

We're doing quite a lot of work on the PR backlog at the moment, so I'm hoping that we can get to this one very soon.

[dsommers]

Hi @lstipakov and @dsommers,

We're doing quite a lot of work on the PR backlog at the moment, so I'm hoping that we can get to this one very soon.

Hi again,

Just wondering, @sbutcher-arm, how far away "very soon" is nowadays ...

I see that this pull request now has merge conflicts, and we're ready to fix that - but it would be good to at least establish if the concept of this patch is acceptable or if we need to do this differently - before we start spending time on fixing a merge conflict if it would be rejected anyhow.

[simonbutcher]

Hi @dsommers. When I said "very soon" we were doing a big push on getting 3rd party PRs into the project, but unfortunately for whatever reason this one didn't get picked up as part of that.

This looks like a good PR, and something that it should be straightforward to adopt, so I propose I review this for you myself, provide feedback, and if you do the rework, I'll find a second reviewer to get it approved and merged. To that end I'll assign it to myself now.

If you confirm you'll do the rework, I'll review it now.

[lstipakov]

Hi @sbutcher-arm, we'll definitely do the rework.

[lstipakov]

@sbutcher-arm I have moved oid.[ch] changes to crypto submodule, fixed @RonEld 's comment and rebased on top of development branch,

[lstipakov]

PR to crypto: ARMmbed/mbed-crypto#372

[dsommers]

Is there anything else we need to do to get this moving forward, @sbutcher-arm?

[simonbutcher]

Well, I started a review, there was some activity on the PR, but I hadn't actually finished my review. I'll try and do that in the next few days.

[lstipakov]

@sbutcher-arm I got "PR-1425-merge TLS Testing — Test failure" but I cannot see what went wrong there.

[gilles-peskine-arm]

This is a good addition in principle, but I think there are several aspects of the current design which don't fit well. Feel free to discuss them if you disagree with my objections.

[gilles-peskine-arm]

This increases the size of oid.o significantly by the standards of small embedded platforms. In a Cortex-M0 build in the default configuration, I get 5892 bytes before, 6673 bytes after.

How about changing the interface so that the extra OIDs are only defined if MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION is enabled, and this allows all unsupported extensions by default (for backward compatibility) but the set of permitted extensions can be changed at runtime?

[lstipakov]

Sorry, I am confused.

In the current model, MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION doesn't require any extra OIDs. Extra OIDs are needed to get extension type from asn1_buf and supported/unsupported flag.

If one wants to permit certain unsupported extension in runtime and obviously doesn't define MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION. how do we recognize extension type without extra OIDs?

[gilles-peskine-arm]

You can parse the ASN.1 sequence crt->v3_ext. But that's obviously not nice at all. Ideally I'd like to come up with a solution that's reasonably easy to use, doesn't add a new compile-time option (or maybe even better: makes the existing one moot), and doesn't increase the code size for embedded builds. Maybe the library could provide a pre-broken-down sequence for the extensions, in such a way that the application can easily look up the extension with a given OID? Or the application could provide a list of OID to accept even if critical?

Since we now have a mailing list, I've started a design discussion there: https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-April/000009.html . Feel free to reply here or on the mailing list.

[ndilieto]

Ideally I'd like to come up with a solution that's reasonably easy to use, doesn't add a new compile-time option (or maybe even better: makes the existing one moot), and doesn't increase the code size for embedded builds.

I think I might have come up with such a solution, please see #3243

[gilles-peskine-arm]

Hi @lstipakov ,

@ndilieto has kindly proposed a different way to handle unsupported extensions which I think is more generic than your proposal and has a smaller footprint, so we're likely to integrate that and reject your proposal. Can you please check if #3243 works for you?

[lstipakov]

#3243 is fine for me, I'll close this one.