CSR parsing with critical fields fails

[mschulz-at-hilscher]

Summary

When using mbedtls_x509_csr_parse_der to parse CSRs that contain extensions that are marked as critical ("Each extension in a certificate is designated as either critical or non-critical.", Section 4.2, RFC5280) with an optional boolean field, the certificate parsing fails with error -0x2562 (MBEDTLS_ERR_X509_INVALID_EXTENSIONS | MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) as the implementation of x509_csr_parse_extensions always expects an octet string to follow the extension ID, ignoring an optionally added boolean indicating criticality of the field.

System information

Mbed TLS version (number or commit id): 3.5.0 Operating system and version: Linux but does not really matter Configuration (if not default, please attach mbedtls_config.h): default Compiler and options (if you used a pre-built binary, please indicate how you obtained it): see under "Steps to reproduce" Additional environment information:

Expected behavior

When parsing valid CRS with or without extensions marked as critical, the certificate parsing shall succeed.

Actual behavior

Certificate parsing returns -0x2562 (MBEDTLS_ERR_X509_INVALID_EXTENSIONS | MBEDTLS_ERR_ASN1_UNEXPECTED_TAG)

Steps to reproduce

Example CSRs: Without extension marked as critical: 308201203081c802010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004e1917dd36f166032917928b272e088f6aa4326eb3690eb0650d37bf97b1abfadff8304026c89564fc1e253c47edc209606de986135807ff2ab5db9135dfd7314a025302306092a864886f70d01090e311630143012060b2b0601040183890c8622020403010101300a06082a8648ce3d040302034700304402205cb201bddb922dd2987facbef22371534ef1b20243008ecae314317b3dae478d02207cd2228b117e4cc5420660eede97eb3f7e1e9541c4f26250585156e0fad6faa5

          AttributeValue [?] SEQUENCE (1 elem)
            SEQUENCE (2 elem)
              OBJECT IDENTIFIER 1.3.6.1.4.1.50316.802.2
              OCTET STRING (3 byte) 010101
                BOOLEAN true

With extension marked as critical: 308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc

          AttributeValue [?] SEQUENCE (1 elem)
            SEQUENCE (3 elem)
              OBJECT IDENTIFIER 1.3.6.1.4.1.50316.802.2
              BOOLEAN true
              OCTET STRING (3 byte) 010101
                BOOLEAN true

Test programe to be placed as csr_critical_ext.c under programs/x509:

#include "mbedtls/build_info.h"
#include "mbedtls/platform.h"
#include "mbedtls/x509_csr.h"
#include <stdio.h>
#include <stdint.h>

uint8_t csr_without_critical[] = {
  0x30, 0x82, 0x01, 0x20, 0x30, 0x81, 0xc8, 0x02, 0x01, 0x00, 0x30, 0x41,
  0x31, 0x19, 0x30, 0x17, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c, 0x10, 0x53,
  0x65, 0x6c, 0x66, 0x20, 0x73, 0x69, 0x67, 0x6e, 0x65, 0x64, 0x20, 0x74,
  0x65, 0x73, 0x74, 0x31, 0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06,
  0x13, 0x02, 0x44, 0x45, 0x31, 0x17, 0x30, 0x15, 0x06, 0x03, 0x55, 0x04,
  0x0a, 0x0c, 0x0e, 0x41, 0x75, 0x74, 0x68, 0x43, 0x72, 0x74, 0x44, 0x42,
  0x20, 0x54, 0x65, 0x73, 0x74, 0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a,
  0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce,
  0x3d, 0x03, 0x01, 0x07, 0x03, 0x42, 0x00, 0x04, 0xe1, 0x91, 0x7d, 0xd3,
  0x6f, 0x16, 0x60, 0x32, 0x91, 0x79, 0x28, 0xb2, 0x72, 0xe0, 0x88, 0xf6,
  0xaa, 0x43, 0x26, 0xeb, 0x36, 0x90, 0xeb, 0x06, 0x50, 0xd3, 0x7b, 0xf9,
  0x7b, 0x1a, 0xbf, 0xad, 0xff, 0x83, 0x04, 0x02, 0x6c, 0x89, 0x56, 0x4f,
  0xc1, 0xe2, 0x53, 0xc4, 0x7e, 0xdc, 0x20, 0x96, 0x06, 0xde, 0x98, 0x61,
  0x35, 0x80, 0x7f, 0xf2, 0xab, 0x5d, 0xb9, 0x13, 0x5d, 0xfd, 0x73, 0x14,
  0xa0, 0x25, 0x30, 0x23, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d,
  0x01, 0x09, 0x0e, 0x31, 0x16, 0x30, 0x14, 0x30, 0x12, 0x06, 0x0b, 0x2b,
  0x06, 0x01, 0x04, 0x01, 0x83, 0x89, 0x0c, 0x86, 0x22, 0x02, 0x04, 0x03,
  0x01, 0x01, 0x01, 0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d,
  0x04, 0x03, 0x02, 0x03, 0x47, 0x00, 0x30, 0x44, 0x02, 0x20, 0x5c, 0xb2,
  0x01, 0xbd, 0xdb, 0x92, 0x2d, 0xd2, 0x98, 0x7f, 0xac, 0xbe, 0xf2, 0x23,
  0x71, 0x53, 0x4e, 0xf1, 0xb2, 0x02, 0x43, 0x00, 0x8e, 0xca, 0xe3, 0x14,
  0x31, 0x7b, 0x3d, 0xae, 0x47, 0x8d, 0x02, 0x20, 0x7c, 0xd2, 0x22, 0x8b,
  0x11, 0x7e, 0x4c, 0xc5, 0x42, 0x06, 0x60, 0xee, 0xde, 0x97, 0xeb, 0x3f,
  0x7e, 0x1e, 0x95, 0x41, 0xc4, 0xf2, 0x62, 0x50, 0x58, 0x51, 0x56, 0xe0,
  0xfa, 0xd6, 0xfa, 0xa5
};

uint8_t csr_with_critical[] = {
  0x30, 0x82, 0x01, 0x23, 0x30, 0x81, 0xcb, 0x02, 0x01, 0x00, 0x30, 0x41,
  0x31, 0x19, 0x30, 0x17, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c, 0x10, 0x53,
  0x65, 0x6c, 0x66, 0x20, 0x73, 0x69, 0x67, 0x6e, 0x65, 0x64, 0x20, 0x74,
  0x65, 0x73, 0x74, 0x31, 0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06,
  0x13, 0x02, 0x44, 0x45, 0x31, 0x17, 0x30, 0x15, 0x06, 0x03, 0x55, 0x04,
  0x0a, 0x0c, 0x0e, 0x41, 0x75, 0x74, 0x68, 0x43, 0x72, 0x74, 0x44, 0x42,
  0x20, 0x54, 0x65, 0x73, 0x74, 0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a,
  0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce,
  0x3d, 0x03, 0x01, 0x07, 0x03, 0x42, 0x00, 0x04, 0xc1, 0x1e, 0xbb, 0x99,
  0x51, 0x84, 0x8a, 0x43, 0x6c, 0xa2, 0xc8, 0xa7, 0x33, 0x82, 0xf2, 0x4b,
  0xbb, 0x6c, 0x28, 0xa9, 0x2e, 0x40, 0x1d, 0x48, 0x89, 0xb0, 0xc3, 0x61,
  0xf3, 0x77, 0xb9, 0x2a, 0x8b, 0x04, 0x97, 0xff, 0x2f, 0x5a, 0x5f, 0x60,
  0x57, 0xae, 0x85, 0xf7, 0x04, 0xab, 0x18, 0x50, 0xbe, 0xf0, 0x75, 0x91,
  0x4f, 0x68, 0xed, 0x3a, 0xeb, 0x15, 0xa1, 0xff, 0x1e, 0xbc, 0x0d, 0xc6,
  0xa0, 0x28, 0x30, 0x26, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d,
  0x01, 0x09, 0x0e, 0x31, 0x19, 0x30, 0x17, 0x30, 0x15, 0x06, 0x0b, 0x2b,
  0x06, 0x01, 0x04, 0x01, 0x83, 0x89, 0x0c, 0x86, 0x22, 0x02, 0x01, 0x01,
  0xff, 0x04, 0x03, 0x01, 0x01, 0x01, 0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86,
  0x48, 0xce, 0x3d, 0x04, 0x03, 0x02, 0x03, 0x47, 0x00, 0x30, 0x44, 0x02,
  0x20, 0x0c, 0x41, 0x08, 0xfd, 0x09, 0x85, 0x25, 0x99, 0x3d, 0x3f, 0xd5,
  0xb1, 0x13, 0xf0, 0xa1, 0xea, 0xd8, 0x75, 0x08, 0x52, 0xba, 0xf5, 0x5a,
  0x2f, 0x8e, 0x67, 0x0a, 0x22, 0xca, 0xbc, 0x0b, 0xa1, 0x02, 0x20, 0x34,
  0xdb, 0x93, 0xa0, 0xfc, 0xb9, 0x93, 0x91, 0x2a, 0xdc, 0xf2, 0xea, 0x8c,
  0xb4, 0xb6, 0x63, 0x89, 0xaf, 0x30, 0xe2, 0x64, 0xd4, 0x3c, 0x0d, 0xae,
  0xa0, 0x32, 0x55, 0xe4, 0x5d, 0x2c, 0xcc
};

int main(void)
{
    mbedtls_x509_csr csr;
    int ret;

    ret = mbedtls_x509_csr_parse_der(&csr, csr_without_critical, sizeof(csr_without_critical));
    printf("without critical: -0x%04x\n", -ret);

    ret = mbedtls_x509_csr_parse_der(&csr, csr_with_critical, sizeof(csr_with_critical));
    printf("with critical: -0x%04x\n", -ret);
}

Command to build:

cc -Wall -Wextra -Wformat=2 -Wno-format-nonliteral -I../tests/include -I../include -D_FILE_OFFSET_BITS=64 -I../3rdparty/everest/include -I../3rdparty/everest/include/everest -I../3rdparty/everest/include/everest/kremlib -I../3rdparty/p256-m/p256-m/include -I../3rdparty/p256-m/p256-m/include/p256-m -I../3rdparty/p256-m/p256-m_driver_interface -O2 x509/csr_critical_ext.c    ../tests/src/asn1_helpers.o ../tests/src/bignum_helpers.o ../tests/src/certs.o ../tests/src/fake_external_rng_for_test.o ../tests/src/helpers.o ../tests/src/psa_crypto_helpers.o ../tests/src/psa_exercise_key.o ../tests/src/random.o ../tests/src/threading_helpers.o ../tests/src/drivers/hash.o ../tests/src/drivers/platform_builtin_keys.o ../tests/src/drivers/test_driver_aead.o ../tests/src/drivers/test_driver_asymmetric_encryption.o ../tests/src/drivers/test_driver_cipher.o ../tests/src/drivers/test_driver_key_agreement.o ../tests/src/drivers/test_driver_key_management.o ../tests/src/drivers/test_driver_mac.o ../tests/src/drivers/test_driver_pake.o ../tests/src/drivers/test_driver_signature.o -L../library -lmbedtls -lmbedx509 -lmbedcrypto  -o x509/csr_critical_ext

Execution and output:

# x509/csr_critical_ext
without critical: -0x0000
with critical: -0x2562

Additional information

The following patch fixes the problem:

From 6eb197f1de355b5313bdc600af2b41db34083b39 Mon Sep 17 00:00:00 2001
From: Matthias Schulz <mschulz@hilscher.com>
Date: Tue, 17 Oct 2023 11:37:47 +0200
Subject: [PATCH] Fix "CSR parsing with critical fields fail" bug.

---
 library/x509_csr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/library/x509_csr.c b/library/x509_csr.c
index 0b2bb6f3b..bd45c5665 100644
--- a/library/x509_csr.c
+++ b/library/x509_csr.c
@@ -78,6 +78,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr,
     int ret;
     size_t len;
     unsigned char *end_ext_data;
+    int critical;
     while (*p < end) {
         mbedtls_x509_buf extn_oid = { 0, 0, NULL };
         int ext_type = 0;
@@ -100,6 +101,9 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr,
         extn_oid.p = *p;
         *p += extn_oid.len;

+        /* Get and ignore optional critical flag */
+        (void)mbedtls_asn1_get_bool(p, end_ext_data, &critical);
+
         /* Data should be octet string type */
         if ((ret = mbedtls_asn1_get_tag(p, end_ext_data, &len,
                                         MBEDTLS_ASN1_OCTET_STRING)) != 0) {
-- 
2.39.1.windows.1

Execution and output:

# x509/csr_critical_ext
without critical: -0x0000
with critical: -0x0000

[tom-cosgrove-arm]

When parsing valid CSRs with or without extensions marked as critical, the certificate parsing shall succeed

I don't think that's quite true.

If an extension is critical, and we don't understand it, we shouldn't blindly accept it.

(As otherwise we're pretty much signing random data given to us. The standard says

Conforming CAs MUST support key identifiers (Sections 4.2.1.1 and 4.2.1.2), basic constraints (Section 4.2.1.9), key usage (Section 4.2.1.3), and certificate policies (Section 4.2.1.4) extensions. If the CA issues certificates with an empty sequence for the subject field, the CA MUST support the subject alternative name extension (Section 4.2.1.6).

and this implies that CAs shouldn’t sign things they don’t understand - otherwise CA support for extensions wouldn’t be needed at all, they would just sign whatever they get.)

[mschulz-at-hilscher]

When parsing valid CSRs with or without extensions marked as critical, the certificate parsing shall succeed

I don't think that's quite true.

If an extension is critical, and we don't understand it, we shouldn't blindly accept it

The alternative would be to implement a mechanism similar to the mbedtls_x509_crt_parse_der_with_ext_cb functions that call a callback function to handle unknown extensions.

However, in the current implementation, no critical extensions are supported at all during CSR parsing. Not even the ones that could be understood by the library. That is a blocker for our applications where we have requirements for marking extensions as critical.

There are even situations where marking extensions as critical are mandatory according to the RFC: "If subject naming information is present only in the subjectAltName extension (e.g., a key bound only to an email address or URI), then the subject name MUST be an empty sequence and the subjectAltName extension MUST be critical."

[mschulz-at-hilscher]

When parsing valid CSRs with or without extensions marked as critical, the certificate parsing shall succeed

I don't think that's quite true.

If an extension is critical, and we don't understand it, we shouldn't blindly accept it.

(As otherwise we're pretty much signing random data given to us. The standard says

Conforming CAs MUST support key identifiers (Sections 4.2.1.1 and 4.2.1.2), basic constraints (Section 4.2.1.9), key usage (Section 4.2.1.3), and certificate policies (Section 4.2.1.4) extensions. If the CA issues certificates with an empty sequence for the subject field, the CA MUST support the subject alternative name extension (Section 4.2.1.6).

and this implies that CAs shouldn’t sign things they don’t understand - otherwise CA support for extensions wouldn’t be needed at all, they would just sign whatever they get.)

The updated code in #8378 now reports an error in case an unknown critical extension is part of a CSR.