[interop] ASN1 NULL parameter for AlgorithmIdentifier MUST be omit for Elliptic Curve

[Fassino]

Description

• Type: Bug
• Priority: Major

---

Bug

OS
Linux

mbed TLS build:
Version: Version: 2.16.3

Expected behavior

To be conform with RFC, mbedtls_asn1_write_algorithm_identifier MUSTN'T add a NULL parameter for Elliptic Curve Cryptography key pair.

RFC 5280 says NULL parameter is optional (see 4.1.1.2)

But, RFC 5754 (ECDSA for X509) says encoding MUST omit the parameters field (see 3.2).

Please note, that the RFC for RSA x509, specify that the NULL MUST be present… Cryptography is beautiful ;-)

Actual behavior

Implementation is not conform to RFC. So when using mbedtls to issue certificate based on ECC key pairs, the certificate is rejected by some cryptography implementations.

[gilles-peskine-arm]

Oh joy, a bug where the sensible fix is to change the API :( Thanks for the report!

Memo: when fixing this, add tests to make sure that Mbed TLS can read back both the correct encoding and the encoding it used to write.

[ciarmcom]

ARM Internal Ref: IOTSSL-2979

[mpg]

Note: this was reported again on the mailing list: https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-December/000273.html

[mpg]

Note: OpenSSL and GnuTLS (certtool) both generate the correct encoding, so clearly we can read the proper encoding, and we have test certificates generated using Mbed TLS, so we can also read the current incorrect encoding, but I agree that when fixing we need to have an explicit test for that.

Also, I'm removing the label "component: crypto" as I believe this only affects X.509. The only place in the crypto library that calls mbedtls_asn1_write_algorithm_identifier() is from mbedtls_pk_write_pubkey_der(), where parameters are used (to identify the curve), so there's no extra NULL added. The only other places mbedtls_asn1_write_algorithm_identifier() is called are both in X.509, specifically in x509_create.c and x509write_crt.c.

[mpg]

The person who reported this by email confirmed that this bug is not just about compliance, but is causing interoperability issues in practice: for example, it causes Chrome to reject all ECDSA-signed certificates created with Mbed TLS.

[marekjansta]

I tried to fix this problem (ran into it when trying to connect to a device with a self-signed certificate using ECC using Chrome after it has switched to its own Certificate Verifier).

Could someone check if that looks ok? Thanks

https://github.com/marekjansta/mbedtls/tree/fix-asn1-algorithm-identifier

[gilles-peskine-arm]

@marekjansta At first glance, the code looks correct.

However the patch would not be acceptable in Mbed TLS because it changes the API. We can't change the signature of an existing function that's declared and documented in a public header.

I think the best solution is to deprecate mbedtls_asn1_write_algorithm_identifier and create a new function that doesn't add the NULL automatically. Keep mbedtls_asn1_write_algorithm_identifier as a MBEDTLS_DEPRECATED function whose implementation is a call to the new function, plus what it takes to add NULL automatically. Both functions need to have unit tests.

[Fassino]

Hi All, good to see that the fix of the issue is ongoing. It will solve many interoperability issue. Do you know when it will be released? And in which version? Thanks

[gilles-peskine-arm]

@Fassino I'm afraid that so far we do not have a pending fix and I'm not aware of anybody working on it (unless @marekjansta plans to make a backward-compatible version of their fix?).

[marekjansta]

I updated my solution so that it does not break the rules.
Could someone verify if this changeset would be acceptable?
https://github.com/marekjansta/mbedtls/tree/fix-asn1-algorithm-identifier

[gilles-peskine-arm]

@marekjansta Yes, that change looks fine in terms of not breaking the API. I haven't reviewed the implementation. And we would need unit tests, both for the new function and for the bug fix.

Please make a pull request. Preferably with the tests, but if you don't have time for those, maybe someone else can contribute the tests.

[daverodgman]

Fixed by #7788