Introduce ASN.1 SEQUENCE traversal API #263

hanno-becker · 2019-09-17T12:23:04Z

Summary: This PR is a spin-out from the X.509 on demand parsing work Mbed-TLS/mbedtls#2478, one of the main drivers of which is to avoid dynamically allocated presentations of ASN.1 SEQUENCEs by traversing the raw ASN.1 in-place whenever needed. To this end, this PR introduces the ASN.1 API mbedtls_asn1_traverse_sequence_of() which can be used to traverse an ASN.1 SEQUENCE container, triggering a user-specified callback for each entry found.
The PR also introduces two helper macros MBEDTLS_ASN1_IS_STRING_TAG and MBEDTLS_OID_CMP_RAW. Those are technically unrelated to the SEQUENCE traversal API and are included here solely to keep the number of dependencies for the on demand parsing PR low.

gilles-peskine-arm · 2019-09-18T08:30:32Z

#75 fixes some edge cases in the ASN.1 library and adds tests. Can you please make sure that your X.509 code and tests are compatible with these fixes? This PR doesn't have tests; I'm willing to accept it without tests but the lack of tests needs to be filed as a separate bug.

hanno-becker · 2019-09-18T08:31:35Z

@gilles-peskine-arm It's not fully satisfactory, but because the PR reimplements mbedtls_asn1_get_sequence_of() in terms of the new traversal API, any tests for the former will also test the latter.
But of course, it'd be better to have standalone tests, too.

Patater · 2019-09-18T11:08:58Z

include/mbedtls/asn1.h

+ *                       call a callback for each entry.
+ *
+ * \warning              This function is still experimental and may change
+ *                       at any time.


Why should this be experimental? When can it be non-experimental? Could we make asn1_internal.h and place it there if it's only for internal use for now?

It isn't internal since it will be used by Mbed TLS.

@gilles-peskine-arm @Patater What's your view on this? This function must be public, as @gilles-peskine-arm said, so what remains is to decide whether we want to declare it as experimental or not. I'm open either way. If you think the API is good as-is, I'm happy to remove the qualification.

My preference is what you wrote: declare this function as experimental. We don't care about this function except as an internal detail of the implementation an X509 feature that we do care about. It has a complex API that I would prefer not to make official, but that I'm ok to include with the understanding that we're likely to modify it in the future.

The situation should be temporary, since I think we're aiming at making the whole asn1.h private in Mbed TLS 3.0 (or some point in the future at least) anyway.

Patater · 2019-09-18T11:09:56Z

include/mbedtls/asn1.h

+ *                       \p cb in case the latter returns a non-zero value.
+ */
+int mbedtls_asn1_traverse_sequence_of(
+    unsigned char **p,


Can the unsigned char be const unsigned char? I don't think we'd be modifying **p, only p or *p.

const unsigned char ** and unsigned char ** do not mix. If this function used a different type, it couldn't be used with the other asn1 parsing functions.

I agree with @Patater in that in principle the type of p should be const unsigned char ** here. However, the rest of the ASN.1 parsing library unfortunately also uses unsigned char **, which we can't change and which forces the use of unsigned char ** here, too (not just for consistency, but simply because this function calls e.g. mbedtls_asn1_get_tag() which has a unsigned char ** argument). An alternative would be to extend the documentation of all ASN.1 parsing functions to explicitly mention that, despite the missing const qualification, the **p is never written to, and that it is hence safe to use the functions in a const unsigned char ** context. If we did that, we could start improving the API by using const unsigned char ** at least for this new API function, and doing a const-cast when calling the other parsing functions.

We can tweak the API in Mbed TLS 3. Until we get there, let's keep the API consistent.

Ok, no change here, then.

Patater · 2019-09-18T11:10:21Z

include/mbedtls/asn1.h

+    uint8_t tag_must_mask, uint8_t tag_must_val,
+    uint8_t tag_may_mask, uint8_t tag_may_val,
+    int (*cb)( void *ctx, int tag,
+               unsigned char* start, size_t len ),


Can start be made const?

I'm wondering about that. In 99% of the cases it would be const: parsing a read-only string. But it may be useful occasionally to modify an input in place.

I agree that it should be const. However, if we want to call other ASN.1 parsing functions from within the callback, then we can't have const here because those parsing functions don't have const qualification, as mentioned above.

Patater · 2019-09-18T11:12:41Z

library/asn1parse.c

+/*
+ *  Parses and splits an ASN.1 "SEQUENCE OF <tag>"
+ */
+int mbedtls_asn1_get_sequence_of( unsigned char **p,


Ah, too bad this is not const. I guess we can't have our callback with p const because of this.

This way it would work, but what doesn't work is calling parsing functions from within mbedtls_asn1_traverse_sequence_of().

gilles-peskine-arm · 2019-10-30T18:59:05Z

I rebased this pull request on top of the current base branch. The previous content by Hanno is at https://github.com/gilles-peskine-arm/mbed-crypto/tree/asn1_traversal_api-1.

Compared with the previous content:

I removed most updates to existing documentation because they were largely superseded by ASN.1 tests without x509 #75.
I added tests for the new functions.
I tweaked the documentation of the new functions.

gilles-peskine-arm · 2019-11-12T17:15:10Z

New CI run now that the problems with the Cypress boards are fixed: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-263-merge/20/ → PASS

Also fix a copypasta.

hanno-becker · 2020-01-31T15:55:59Z

Rebased on top of development. The only conflict stemmed from @yanesca's introduction of default error codes and was easy to fix.

mpg · 2020-02-03T10:37:51Z

The CI found a problem, in test-ref-configs, with config-symmetric-only.h, the first error is:

/var/lib/build/include/mbedtls/asn1.h:504:5: error: unknown type name 'uint8_t'
     uint8_t tag_must_mask, uint8_t tag_must_val,
     ^

and the other errors look similar. I suggest including stdint.h directly in asn1.h - it probably happens to be included indirectly in some configurations including the default one, but apparently not all of them.

hanno-becker · 2020-02-03T10:38:54Z

Thank you @mpg, I'll fix that.

The rest of the ASN.1 API uses `unsigned char`, too.

hanno-becker · 2020-02-03T10:41:08Z

@mpg @gilles-peskine-arm Fixed the issue by replacing uint8_t by unsigned char.

mpg

LGTM

gilles-peskine-arm · 2020-02-03T12:35:01Z

CI passes except for the Mbed OS tests which are failing for unrelated reasons.

* ARMmbed#352: Parse RSA parameters DP, DQ and QP from PKCS1 private keys * ARMmbed#263: Introduce ASN.1 SEQUENCE traversal API * ARMmbed#345: Fix possible error code mangling in psa_mac_verify_finish * ARMmbed#357: Update Mbed Crypto with latest Mbed TLS changes as of 2020-02-03 * ARMmbed#350: test_suite_asn1parse: improve testing of trailing garbage in parse_prefixes * ARMmbed#346: Improve robustness and testing of mbedtls_mpi_copy

hanno-becker added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Sep 17, 2019

hanno-becker requested review from Patater and yanesca September 17, 2019 12:23

hanno-becker mentioned this pull request Sep 17, 2019

Reduce X.509 RAM usage through parsing on demand Mbed-TLS/mbedtls#2478

Draft

10 tasks

hanno-becker force-pushed the asn1_traversal_api branch from 8950dc6 to 5d972e7 Compare September 18, 2019 08:16

Patater reviewed Sep 18, 2019

View reviewed changes

Patater added the needs: work The pull request needs rework before it can be merged. label Sep 19, 2019

gilles-peskine-arm self-assigned this Oct 30, 2019

gilles-peskine-arm force-pushed the asn1_traversal_api branch from 5d972e7 to f40fccf Compare October 30, 2019 18:56

gilles-peskine-arm removed the needs: work The pull request needs rework before it can be merged. label Oct 30, 2019

gilles-peskine-arm requested a review from Patater October 30, 2019 19:01

gilles-peskine-arm added the needs: ci Needs a passing full CI run label Nov 12, 2019

gilles-peskine-arm added enhancement New feature or request and removed needs: ci Needs a passing full CI run labels Nov 12, 2019

Hanno Becker and others added 6 commits January 31, 2020 15:37

ASN.1: Add helper macro to detect string types

cba7122

ASN.1: Add helper macro to compare ASN.1 buffer to OID string

63e38fe

ASN.1: Introduce helper function to free ASN.1 sequence

12ae27d

Document one more error code for mbedtls_asn1_get_sequence_of

b5c74a5

Also fix a copypasta.

ASN.1: Add ASN.1 SEQUENCE traversal API

199b709

ASN.1: Reimplement mbedtls_asn1_get_sequence_of() via traversal API

1505f63

hanno-becker force-pushed the asn1_traversal_api branch from f40fccf to 1505f63 Compare January 31, 2020 15:55

hanno-becker requested a review from gilles-peskine-arm January 31, 2020 15:55

gilles-peskine-arm previously approved these changes Jan 31, 2020

View reviewed changes

Replace use of uint8_t by unsigned char in ASN.1 seq traversal API

34aada2

The rest of the ASN.1 API uses `unsigned char`, too.

hanno-becker dismissed gilles-peskine-arm’s stale review via 34aada2 February 3, 2020 10:41

mpg approved these changes Feb 3, 2020

View reviewed changes

gilles-peskine-arm approved these changes Feb 3, 2020

View reviewed changes

gilles-peskine-arm added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 3, 2020

gilles-peskine-arm merged commit c69c2c5 into ARMmbed:development Feb 3, 2020

mpg mentioned this pull request Feb 17, 2020

Update crypto submodule 2020-02-06 Mbed-TLS/mbedtls#3037

Merged

gilles-peskine-arm mentioned this pull request Jun 7, 2023

asn1parse: Reject some invalid tags Mbed-TLS/mbedtls#7697

Open

3 tasks

Introduce ASN.1 SEQUENCE traversal API #263

Introduce ASN.1 SEQUENCE traversal API #263

Uh oh!

Conversation

hanno-becker commented Sep 17, 2019

Uh oh!

gilles-peskine-arm commented Sep 18, 2019

Uh oh!

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

Uh oh!

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!

mpg Feb 3, 2020 • 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!

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!

gilles-peskine-arm commented Oct 30, 2019

Uh oh!

gilles-peskine-arm commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanno-becker commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpg commented Feb 3, 2020

Uh oh!

hanno-becker commented Feb 3, 2020

Uh oh!

hanno-becker commented Feb 3, 2020

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Feb 3, 2020

Uh oh!

Uh oh!

hanno-becker commented Sep 18, 2019 •

edited

Loading

mpg Feb 3, 2020 •

edited

Loading

gilles-peskine-arm commented Nov 12, 2019 •

edited

Loading

hanno-becker commented Jan 31, 2020 •

edited

Loading