Additional extension support #2530

RonEld · 2019-03-24T11:34:48Z

Description

Add support for parsing the following X509 extensions:

subject alternative name, of type otherName - HardwareModuleName , as defined in rfc 4108 section 5.
certificate policy, only of type "Any Policy", as defined in rfc 5280 section 4.2.1.4

Status

READY

Requires Backporting

NO
This is an enhancement

Migrations

NO

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

hanno-becker · 2019-03-24T19:52:58Z

@RonEld How urgent is this? It would be good to base it on the recent reworkings of the X.509 module, especially #2478, but it's not clear when we'll have time to review and merge the latter.

RonEld · 2019-03-25T07:38:38Z

@hanno-arm This is targeting 5.13.
I'll rebase, and see if there are differrnces in the behavior

RonEld · 2019-03-25T12:25:50Z

@hanno-arm I have just checked, and it seems that this is already on top of the latest x509 changes, except for the parsing on demand which is not merged yet

RonEld · 2019-03-26T13:59:18Z

This is dependent on #2531 to be merged, in order to pass CI

k-stachowiak

I have not found any critical issue. However, I am not sure whether the ABI break is feasible at this moment, and I would suggest removing redundant checks. If a rework is done I would also suggest fixing minor formatting issues.

k-stachowiak · 2019-03-25T16:12:46Z

include/mbedtls/x509_crt.h

@@ -79,6 +79,8 @@ typedef struct mbedtls_x509_crt
    mbedtls_x509_sequence subject_alt_names;    /**< Optional list of Subject Alternative Names (Only dNSName supported). */
    mbedtls_x509_name hardware_module_name;     /**< Optional OtherName subject Alternative Name. */

+    mbedtls_x509_sequence certificate_policies; /**< Optional list of certificate4 policies (Only anyPolicy supported). */


To my understanding this line breaks the ABI compatibility. Is this intentional and acceptable in this moment in terms of the development cycle?

Yes, this breaks the ABI. As well as the hardware_module_name introduced in line 80.

Do you see another way to support these new extensions?
I believe that in this development cycle, it is ok to break the ABI in development branch. @Patater please confirm

I don't know, how else this could be implemented, especially in a natural way. I only wanted to point this out, as it will matter in case of a release. As far as I understand, merging an ABI break into development, effectively makes the next release a breaking one. If this is what we plan (this is what I meant by the development cycle), then it is fine. But if we plan to have non-breaking, minor releases first, then I think this PR should be somehow tagged and the merge should be held until we are ready for an ABI breaking release.

Thanks,yes, of course.

The only way to avoid ABI breakage is to add yet another configuration option ( even two for the two additions?), but I don't like adding more configuration options; at least for this purpose

k-stachowiak · 2019-03-26T08:20:29Z

ChangeLog

@@ -12,6 +12,10 @@ Features
   * Add MBEDTLS_REMOVE_3DES_CIPHERSUITES to allow removing 3DES ciphersuites
     from the default list (enabled by default). See
     https://sweet32.info/SWEET32_CCS16.pdf.
+   * Add support for parsing subject alternative name, of type otherName,
+     HardwareModuleName , as defined in rfc 4108 section 5.


If there is a rework done, I would suggest removing an extra space before the comma here.

k-stachowiak · 2019-03-26T08:39:54Z

include/mbedtls/oid.h

+ * { iso(1) identified-organization(3) dod(6) internet(1)
+ *                      private(4) enterprise(1) WiSUN(45605) FieldAreaNetwork(1) }
+ */
+#define MBEDTLS_OID_WISUN_FAN                  MBEDTLS_OID_INTERNET "\x04\x01\x82\xe4\x25\x01"


Minor: the define's value is not aligned to the others.

k-stachowiak · 2019-03-26T08:48:43Z

library/oid.c

@@ -290,12 +294,22 @@ static const mbedtls_oid_descriptor_t oid_ext_key_usage[] =
    { ADD_LEN( MBEDTLS_OID_EMAIL_PROTECTION ), "id-kp-emailProtection", "E-mail Protection" },
    { ADD_LEN( MBEDTLS_OID_TIME_STAMPING ),    "id-kp-timeStamping",    "Time Stamping" },
    { ADD_LEN( MBEDTLS_OID_OCSP_SIGNING ),     "id-kp-OCSPSigning",     "OCSP Signing" },
+    { ADD_LEN( MBEDTLS_OID_WISUN_FAN ),        "id-kp-wisun-fan-device","Wi-Sun alliance field area network" },


Minor: all the description strings could be shifted one space to the right here, to have a space after the comma.

k-stachowiak · 2019-03-26T08:59:21Z

library/x509_crt.c

+    unsigned char tag;
+    mbedtls_x509_buf cur_oid = { 0, 0, NULL };
+
+     if( ( ret = mbedtls_asn1_get_tag( p, end, &len,


The function is mis-indented starting from this line until the next return statement.

k-stachowiak · 2019-03-26T10:07:01Z

library/x509_crt.c

+    while( *p < end )
+    {
+        mbedtls_x509_buf policy_oid = { 0, 0, NULL };
+        if( ( end - *p ) < 1 )


It seems to me that this test is not necessary, as it is repeated inside the ...get_tag function below.

k-stachowiak · 2019-03-26T10:09:47Z

library/x509_crt.c

+
+        policy_oid.tag = **p;
+
+        if( policy_oid.tag != MBEDTLS_ASN1_OID )


As above, this test repeats part of the algorithm executed in ...get_tag below.

library/x509_crt.c

RonEld · 2019-03-27T14:34:41Z

@k-stachowiak Thank you for your comments. Yes, this PR will need re work, as the crypto submodule should be updated, once #2531 will be merged into it.

hanno-becker · 2019-03-27T15:45:26Z

library/x509_crt.c

+         return( 0 );
+     }
+     *p += len;
+    if( ( ret = mbedtls_asn1_get_tag( p, end, &len,


Minor: The indentation is off here.

yes, the indentation error described in #2530 (comment) is continued until this line

hanno-becker · 2019-03-27T15:47:30Z

library/x509_crt.c

+    hardware_module_name->oid.len = len;
+
+    *p += len;
+    tag = **p;


Blocker: This looks like it reads **p before validating that there's space available.

Suggestion (we've had this in other places before, too): Remove the assignment entirely and set hardware_module_name->val.tag to MBEDTLS_ASN1_OCTET_STRING, because a successful call to mbedtls_asn1_get_tag() determines the ASN.1 tag uniquely.

hanno-becker · 2019-03-27T15:48:30Z

library/x509_crt.c

        {
+        case( MBEDTLS_ASN1_CONTEXT_SPECIFIC | 0 ):


The indentation is off here.

This is same indentation as in the switch case in x509_get_crt_ext().
I took the example from that function. Am I missing something?

hanno-becker

I noticed a missing bounds check and some style issues.

In general, I am concerned that this is in severe conflict with #2478, and would appreciate if we could rebase it on top of the latter in case #2478 is targeted for the next release, too. Ping @Patater.

RonEld · 2019-03-27T16:12:08Z

@hanno-arm Thank you for your comments.
As mentioned, if #2478 will be merged prior to this PR, then I will rebase this PR on top of it, and make the necessary adjustments.

RonEld · 2019-04-01T15:27:52Z

This PR is pending #2535 and #2536 to be merged to the crypto submodule

hanno-becker · 2019-04-07T08:39:32Z

include/mbedtls/x509_crt.h

@@ -77,6 +77,9 @@ typedef struct mbedtls_x509_crt
    mbedtls_x509_buf subject_id;        /**< Optional X.509 v2/v3 subject unique identifier. */
    mbedtls_x509_buf v3_ext;            /**< Optional X.509 v3 extensions.  */
    mbedtls_x509_sequence subject_alt_names;    /**< Optional list of Subject Alternative Names (Only dNSName supported). */
+    mbedtls_x509_name hardware_module_name;     /**< Optional OtherName subject Alternative Name. */


Note: The storage of names and sequences in mbedtls_x509_name and mbedtls_x509_sequence is very RAM heavy, and I am removing them in #2478, which will also affect these new fields.

hanno-becker · 2019-04-07T08:41:05Z

library/x509_crt.c

+ */
+static int x509_get_other_name( unsigned char **p,
+                                const unsigned char *end,
+                                mbedtls_x509_name *hardware_module_name )


I think there is mismatch between the generic name of the function and the specific name of its hardware_module_name argument. However, given that it's static and can change at any time, it's not a blocker.

Yes, I am aware. My intention was this function can be enhanced to support other "other names" in the future, but currently only hardware module name supported.
In addition, this function parses the other name SAN section, and returns only the hardware module name

hanno-becker · 2019-04-07T08:47:28Z

library/x509_crt.c

 */
 static int x509_get_subject_alt_name( unsigned char **p,
                                      const unsigned char *end,
-                                      mbedtls_x509_sequence *subject_alt_name )
+                                      mbedtls_x509_sequence *subject_alt_name,


Similar to above, I think the naming is not optimal here. Now that we distinguish more than one kind of subject alternative name, with subject_alt_name only holding the dNSName entries, and hardware_module_name the HwModuleName entries, the parameter names should reflect that.

However, again this is not a blocker because the function is static, and is also largely rewritten in #2478 anyway.

The subject_alt_name should hold more than just dnsName in the future, such as ipAddress.
However, the HardwareModuleName is a different structure, as it is actually a sequence of an oid and an Octet String, while dnsName and iPAddress are strings. We could think of a way to combine the two

1. Typo fix. 2. Change byte by byte coipy to `memcpy`. 3. Remove parenthesis in switch cases.

RonEld · 2019-05-16T13:21:32Z

@k-stachowiak Thank you for your comments!
I believe I have addressed them

k-stachowiak

@RonEld Thanks for addressing my comments. The issues I raised have been resolved.

Add some invalid certificate tests for certifiate policies extension.

RonEld · 2019-05-16T15:19:39Z

@hanno-arm I have added some negative tests, however I am not sure about how it is correct

yanesca · 2019-05-17T13:53:43Z

@RonEld Can you please share how you generated the test data. Being a negative test, my generic ASN1 parser chokes on it and I can't confirm that they indeed test what they should. For example osting the correct data that you modified to get them would be very helpful. Best would be to put it in a comment or at least in the commit message.

@andresag01 I am on leave next week, can you please work with @RonEld and double check the test vectors, before you renew your approval?

RonEld · 2019-05-19T06:22:46Z

@yanesca @andresag01 I have generated the flawd data according to #2530 (comment)
I took the data from the "basic constraints" tests and modified the OID to certificate policies OID, and and manually changed relevant bytes. This is why I am not sure about the data too much, except that it's invalid.. Using the debugger, I saw that the error is returned from the x509_get_crt_ext function, and not the x509_get_certificate_policies function.

Update the test data for the negative certificate policies extension tests with correct lengths, to test the correct behaviour. Add another test.

andresag01

I looked over the PR once again very briefly. Found a small style issue that is not a blocker.

IMHO we could always use more tests, specially negative testing. But these test vectors are extremely hard to create and generally that is better done using something like fuzzing. This PR adds about 6 negative tests (+/- 1). This is not a lot and I would recommend at least a couple more, but I do not think this is a blocker.

andresag01 · 2019-05-19T16:55:02Z

library/x509_crt.c

        MBEDTLS_X509_SAFE_SNPRINTF;

        if( ( ret = x509_info_subject_alt_name( &p, &n,
-                                            &crt->subject_alt_names ) ) != 0 )
+                                                &crt->subject_alt_names,
+                                                prefix) ) != 0 )


Minor: Please add space before )

RonEld · 2019-05-20T07:10:32Z

@andresag01 Thank you for your review!
I think that we can always have more tests.
My main concern is whether the test data itself tests what it claims it does, and that it checks the correct error code. For example. the test "(TBSCertificate v3, ext CertificatePolicies tag, data not oid)" I would expect the error to be :MBEDTLS_ERR_X509_INVALID_EXTENSIONS + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG instead of MBEDTLS_ERR_X509_INVALID_EXTENSIONS + MBEDTLS_ERR_ASN1_OUT_OF_DATA which is currently returned. I am investigating if this is an issue with the data or the code

k-stachowiak · 2019-05-20T08:16:10Z

@RonEld I used a windows' utility program to analyze the certificates that you provided, and it fails for all of them with a following error:

C:\Users\krzsta01\Documents\temp>certutil cert1
OCSP Request:
Cannot decode object: ASN1 bad tag value met. 0x8009310b (ASN: 267 CRYPT_E_ASN1_BADTAG)
CertUtil: -dump command FAILED: 0x8009310b (ASN: 267 CRYPT_E_ASN1_BADTAG)
CertUtil: ASN1 bad tag value met.

I tried generating the binary file from the test string in two ways: with and xxd -r -p command in cygwin, and using an online converter. I have md5 compared them to make sure I'm getting a right certificate file.

I am willing to approve the PR, but I'm not entirely sure that the tests should be added if, as you mentioned, they may not test what they are supposed to. Once this question is resolved, I'll gladly tick it green.

Fix test data to test what it actually intends to test.

RonEld · 2019-05-20T08:59:49Z

@andresag01 THank you for your review!
I think that we can always have more tests.
My main concern is whether the test data itself tests what it claims it does, and that it checks the correct error code.

Add whitespace before parenthesis.

RonEld · 2019-05-20T09:04:07Z

@andresag01 @k-stachowiak I believe I have fixed the test data to test what it actually intends to test, and fixed the error codes accordingly.

I have also fixed the style fix mentioned in #2530 (comment)

Frankly I think some of the commits require some squashing, but that's up to you to decide

Considering the comments were addressed, dismissing the review, to be re-reviewed if have the capacity

k-stachowiak

I checked the new test data files in practice to see, where the parsing fails, and it looked good as far as I could tell.

Regarding the commit history, I would normally advise against the rewriting, but I have noticed there is a conflict on the ChangeLog. If that cannot be resolved without a rebase, the history cleanup may be a reasonable thing to do.

On that account, please note that "style fix" is an incorrect commit message (on the last commit). A minimal change I'd suggest is to make it "Fix style", to put it in the required form.

Patater · 2019-05-20T15:29:30Z

Thanks all for your reviews. We'll consider @andresag01's review as still valid, since only whitespace changes have been made since. With two approvals, this is ready for merge.

RonEld added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-x509 labels Mar 24, 2019

RonEld requested review from k-stachowiak and andresag01 March 24, 2019 11:34

RonEld force-pushed the additional_extension_support branch 2 times, most recently from 7808db6 to 77e0172 Compare March 24, 2019 15:51

RonEld force-pushed the additional_extension_support branch from 77e0172 to f714116 Compare March 25, 2019 12:25

RonEld force-pushed the additional_extension_support branch from f714116 to c2bf47d Compare March 25, 2019 12:58

RonEld added the needs-preceding-pr Requires another PR to be merged first label Mar 26, 2019

k-stachowiak reviewed Mar 27, 2019

View reviewed changes

RonEld added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 27, 2019

hanno-becker reviewed Mar 27, 2019

View reviewed changes

hanno-becker suggested changes Mar 27, 2019

View reviewed changes

Patater mentioned this pull request Apr 1, 2019

Additional extension support #2534

Closed

RonEld mentioned this pull request Apr 7, 2019

Add certificate policy oid x509 extension #2536

Merged

4 tasks

hanno-becker reviewed Apr 7, 2019

View reviewed changes

Fix minor issues

a291391

1. Typo fix. 2. Change byte by byte coipy to `memcpy`. 3. Remove parenthesis in switch cases.

k-stachowiak previously approved these changes May 16, 2019

View reviewed changes

Add some negative test cases

11818f2

Add some invalid certificate tests for certifiate policies extension.

RonEld dismissed k-stachowiak’s stale review via 11818f2 May 16, 2019 15:18

Update test data

8a59d6b

Update the test data for the negative certificate policies extension tests with correct lengths, to test the correct behaviour. Add another test.

RonEld force-pushed the additional_extension_support branch from 30c4584 to 8a59d6b Compare May 19, 2019 11:12

andresag01 previously approved these changes May 19, 2019

View reviewed changes

Fix test data

76a9c4a

Fix test data to test what it actually intends to test.

RonEld dismissed andresag01’s stale review via 76a9c4a May 20, 2019 08:59

Style fix

6aeae9e

Add whitespace before parenthesis.

k-stachowiak approved these changes May 20, 2019

View reviewed changes

Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 20, 2019

Patater merged commit 6aeae9e into Mbed-TLS:development May 21, 2019

RonEld mentioned this pull request May 22, 2019

Set next sequence of subject_alt_names to NULL #2655

Merged

4 tasks

hanno-becker mentioned this pull request Jun 5, 2019

Enhance X.509 CRT negative parsing tests #2443

Merged

AndrzejKurek mentioned this pull request Jan 16, 2023

General support for subject alternative name subtypes #731

Closed

This was referenced Apr 30, 2023

OPC UA: Support for HardwareModuleName in cert_req and cert_write #7526

Open

OPC UA: Support HardwareModuleName in CSR/Cert generation #7527

Open

davidhorstmann-arm mentioned this pull request Jun 23, 2023

Add support for certificatePolicies extension #1415

Closed


		policy_oid.tag = **p;

		if( policy_oid.tag != MBEDTLS_ASN1_OID )

Additional extension support #2530

Additional extension support #2530

Uh oh!

Conversation

RonEld commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Requires Backporting

Migrations

Todos

Steps to test or reproduce

Uh oh!

hanno-becker commented Mar 24, 2019

Uh oh!

RonEld commented Mar 25, 2019

Uh oh!

RonEld commented Mar 25, 2019

Uh oh!

RonEld commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-stachowiak Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RonEld commented Mar 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

RonEld commented Mar 27, 2019

Uh oh!

RonEld commented Apr 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

RonEld commented Mar 24, 2019 •

edited

Loading

RonEld commented Mar 26, 2019 •

edited

Loading

RonEld Mar 28, 2019 •

edited

Loading

k-stachowiak Mar 26, 2019 •

edited

Loading

hanno-becker Mar 27, 2019 •

edited

Loading

RonEld commented May 19, 2019 •

edited

Loading