ASN.1 tests without x509 #75

gilles-peskine-arm · 2019-03-01T22:29:37Z

Add ASN.1 unit tests. Cover all functions from the asn1 parse and write modules. Fix #76.

Fix a couple of minor bugs and improve the documentation.

This PR breaks mbedtls because two test cases in test_suite_x509_parse make assumptions about bugs in the ASN.1 module which this PR fixes. This PR must land in mbedtls only once Mbed-TLS/mbedtls#2488 is merged there.

Once this PR is merged, a commented-out test case that Mbed-TLS/mbedtls#2488 adds to test_suite_x509_parse.data can be uncommented.

Backports:

ASN.1 parsing bug fixes: I don't think so. The bugs I fixed are minor. They cause valid but uncommon data to be rejected. If it hasn't bothered anyone in all that time, it's best not to take any risks.
New tests: no. In earlier versions, ASN.1 was tested indirectly through X.509. The ASN.1 tests improve the coverage, but I don't think it's worth backporting them
Documentation improvements: yes (but check if the documentation needs to be tweaked to acknowledge the bugs and limitations in the older code).

Patater · 2019-09-10T15:42:29Z

Needs rebase to resolve conflicts

Allow test code to declare a "step number". Report the current step number when a test fails.

Document preconditions on parameters, values changed through pointers, and error codes. This commit leaves some issues regarding integers (especially negative integers) open, because we don't have a policy decision on how to handle them yet.

Add self-contained ASN.1 parsing tests, so that ASN.1 parsing is not solely tested through X.509 and TLS. The tests cover every function and almost complete line coverage in asn1parse.c. A few test cases containing negative and edge case INTEGER values are deliberately deactivated because the historical library behavior is at odds with official specifications, but changing the behavior might break interoperability. Other than that, these tests revealed a couple of minor bugs which will be fixed in subsequent commits.

Allow any number of leading zeros, not just based on sizeof(int).

Fix improper rejection of bitstrings with length less than 2.

Omit negative integers and MPIs that would result in values that look like negative INTEGERs, since the library doesn't respect the specifications there, but fixing it has a serious risk of breaking interoperability when ASN.1 is used in X.509 and other cryptography-related applications.

mbedtls_asn1_write_int had an undocumented restriction to values that fit in a single octet. Fix this. Negative integers are still not supported.

The documentation never said it explicitly, but the ASN.1 library doesn't support negative integers. Say it explicitly. Also fix a copypasta error.

Use the test-many-sizes framework for string writes as well (previously, it was only used for booleans and integers). This way, more edge cases are tested with less test code. This commit removes buffer overwrite checks. Instead of these checks, run the test suite under a memory sanitizer (which we do in our CI).

Document how mbedtls_asn1_store_named_data allocates val.p in the new or modified entry. Change the behavior to be more regular, always setting the new length to val_len. This does not affect the previous documented behavior since this aspect was not documented. This does not affect current usage in Mbed TLS's X.509 module where calls with the same OID always use the same size for the associated value.

The new macro ASSERT_ALLOC_WEAK does not fail the test case if the memory allocation fails. This is useful for tests that allocate a large amount of memory, but that aren't useful on platforms where allocating such a large amount is not possible. Ideally this macro should mark the test as skipped. We don't yet have a facility for that but we're working on it. Once we have a skip functionality, this macro should be changed to use it.

MBEDTLS_ERR_ASN1_INVALID_DATA is documented as "not used", but it has been used since the PolarSSL days.

k-stachowiak

In general the changes look good. If I'm not mistaken some test cases are applied with incorrect arguments (see comments), plus, I've managed to notice some typos. Otherwise looks good.

k-stachowiak · 2019-09-12T09:28:18Z

include/mbedtls/asn1.h

+ *              after the length, i.e. the first byte of the content.
+ *              On error, the value of \c *p is undefined.
+ * \param end   End of data.
+ * \param len   On successful completion, \c *len contains the lengtth


Typo: "lengtth"

This typo is still present and should be fixed.

Oh, I hadn't spotted it was present twice. I amended the typo-fixing commit to also fix this one.

k-stachowiak · 2019-09-12T09:29:08Z

include/mbedtls/asn1.h

+ * \param p     On entry, \c *p points to the start of the ASN.1 element.
+ *              On successful completion, \c *p points to the first byte
+ *              after the length, i.e. the first byte of the content.
+ *              On error, the value of \c *p is undefined.


I wonder if we can at least guarantee *p to remain in the <begin, end> range. If so, the API user can e.g. dump <*p, end> for debugging purposes.

We can, but how useful is that? *p may be beyond the location of the error. I don't want to guarantee anything about where *p is in relation to the error. And “location of the error” may not be meaningful anyway.

For debugging purposes, looking around *p may be helpful, but then you can see in your debugger that *p points somewhere inside the buffer. For logging, I don't think this is useful. So I'd rather keep it simple and flexible: it's undefined (for example, one day we may want to set *p = NULL to avoid accidentally using some misparsed data).

k-stachowiak · 2019-09-12T09:35:48Z

include/mbedtls/asn1.h

-                             size_t *len );
+int mbedtls_asn1_get_bitstring_null( unsigned char **p,
+                                     const unsigned char *end,
+                                     size_t *len );

 /**
 * \brief       Parses and splits an ASN.1 "SEQUENCE OF <tag>"
 *              Updated the pointer to immediately behind the full sequence tag.


Preexisting: Shouldn't this be "Updates to the ..."? Also, there's no period at the end of the previous line.

k-stachowiak · 2019-09-20T10:24:45Z

tests/suites/test_suite_asn1parse.data

+free_named_data_list:0
+
+Free named data list (2)
+free_named_data_list:0


All the three free_named_data_list() are given an argument 0

Free named data list (1) .......................................... Segmentation fault (core dumped)

Having the intended test data revealed a bug in the test code.

k-stachowiak

Thanks for addressing my comments. I didn't manage to find further issues.

Fix copypasta in test data and fix a switcho in test code.

gilles-peskine-arm · 2019-10-04T09:36:36Z

Mbed-TLS/mbedtls#2488 has been merged, so this PR is safe to merge.

Patater

LGTM

Patater · 2019-10-04T11:29:49Z

CI failure is only TLS's check generated files test. The generated files will be fixed when TLS updates to the new version of the Crypto submodule.

******************************************************************
* check_generated_files: Check: freshness of generated source files 
* Fri Oct  4 10:27:21 UTC 2019
******************************************************************
'library/error.c' was either modified or deleted by 'scripts/generate_errors.pl'
^^^^check_generated_files: Check: freshness of generated source files: tests/scripts/check-generated-files.sh -> 1^^^^

Update crypto submodule: * ARMmbed/mbed-crypto#277: Improve speed of PBKDF2 by caching the digest state of the passphras * ARMmbed/mbed-crypto#269: Add PSA API versioning * ARMmbed/mbed-crypto#278: Fix on target test issues * ARMmbed/mbed-crypto#286: Fix defgroup syntax for API version section * ARMmbed/mbed-crypto#75: ASN.1 tests without x509

Get started on writing a change log file for Mbed Crypto. I went through pull requests merged since the tag mbedcrypto-2.0.0 and up to ARMmbed#75, i.e. commit 9ab7c07.

* ARMmbed#277: Improve speed of PBKDF2 by caching the digest state of the passphrase * ARMmbed#269: Add PSA API versioning * ARMmbed#278: Fix on target test issues * ARMmbed#286: Fix defgroup syntax for API version section * ARMmbed#75: ASN.1 tests without x509

gilles-peskine-arm added enhancement New feature or request needs: preceding PR Requires another PR to be merged first labels Mar 1, 2019

gilles-peskine-arm mentioned this pull request Mar 1, 2019

Change X.509 test case to not rely on asn1parse limitation Mbed-TLS/mbedtls#2488

Merged

gilles-peskine-arm force-pushed the asn1-tests-without-x509 branch from 9710b2d to 83ef814 Compare March 4, 2019 14:47

gilles-peskine-arm mentioned this pull request Mar 4, 2019

Add some x509 cert write tests that set certain fields twice Mbed-TLS/mbedtls#2492

Draft

gilles-peskine-arm force-pushed the asn1-tests-without-x509 branch from 83ef814 to b802a69 Compare March 4, 2019 16:49

gilles-peskine-arm added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Mar 4, 2019

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

gilles-peskine-arm added 14 commits September 11, 2019 15:46

Report step number when a test case fails

5605591

Allow test code to declare a "step number". Report the current step number when a test fails.

mbedtls_asn1_get_int: allow leading zeros properly

f7d6acd

Allow any number of leading zeros, not just based on sizeof(int).

mbedtls_asn1_get_bitstring_null: fix rejection of short inputs

e40d120

Fix improper rejection of bitstrings with length less than 2.

Improve mbedtls_asn1_write_int to support values >255

1dbab67

mbedtls_asn1_write_int had an undocumented restriction to values that fit in a single octet. Fix this. Negative integers are still not supported.

asn1_write documentation: say that integers must be non-negative

105031b

The documentation never said it explicitly, but the ASN.1 library doesn't support negative integers. Say it explicitly. Also fix a copypasta error.

Fix typo that prevented mbedtls_asn1_write_len tests from running

91d8d02

asn1write: test NULL, OID and AlgorithmIdentifier

9311cf5

Test mbedtls_asn1_store_named_data

a902303

gilles-peskine-arm force-pushed the asn1-tests-without-x509 branch from 3c435fb to f34f2db Compare September 11, 2019 15:23

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

Fix long-standing bug in error code description

aac3853

MBEDTLS_ERR_ASN1_INVALID_DATA is documented as "not used", but it has been used since the PolarSSL days.

gilles-peskine-arm force-pushed the asn1-tests-without-x509 branch from f34f2db to aac3853 Compare September 11, 2019 16:16

gilles-peskine-arm mentioned this pull request Sep 18, 2019

Introduce ASN.1 SEQUENCE traversal API #263

Merged

k-stachowiak self-requested a review September 19, 2019 14:11

k-stachowiak suggested changes Sep 20, 2019

View reviewed changes

gilles-peskine-arm requested a review from k-stachowiak September 20, 2019 19:11

k-stachowiak previously approved these changes Sep 23, 2019

View reviewed changes

gilles-peskine-arm added 2 commits October 4, 2019 11:35

Fix typos in documentation

dc2db48

Fix free_named_data_list tests

88f136f

Fix copypasta in test data and fix a switcho in test code.

gilles-peskine-arm dismissed k-stachowiak’s stale review via 88f136f October 4, 2019 09:36

gilles-peskine-arm force-pushed the asn1-tests-without-x509 branch from e304fa6 to 88f136f Compare October 4, 2019 09:36

gilles-peskine-arm removed the needs: preceding PR Requires another PR to be merged first label Oct 4, 2019

Patater approved these changes Oct 4, 2019

View reviewed changes

k-stachowiak approved these changes Oct 4, 2019

View reviewed changes

Patater merged commit 9ab7c07 into ARMmbed:development Oct 4, 2019

gilles-peskine-arm mentioned this pull request Oct 4, 2019

Update crypto 2019-10-04 Mbed-TLS/mbedtls#2882

Merged

gilles-peskine-arm mentioned this pull request Oct 10, 2019

Fix int overflow in mbedtls_asn1_get_int #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ASN.1 tests without x509 #75

ASN.1 tests without x509 #75

Uh oh!

gilles-peskine-arm commented Mar 1, 2019 •

edited

Loading

Uh oh!

Patater commented Sep 10, 2019

Uh oh!

k-stachowiak left a comment

Uh oh!

k-stachowiak Sep 12, 2019

Uh oh!

Patater Sep 30, 2019

Uh oh!

gilles-peskine-arm Oct 4, 2019

Uh oh!

k-stachowiak Sep 12, 2019

Uh oh!

gilles-peskine-arm Sep 20, 2019

Uh oh!

k-stachowiak Sep 12, 2019

Uh oh!

k-stachowiak Sep 20, 2019

Uh oh!

gilles-peskine-arm Sep 20, 2019

Uh oh!

k-stachowiak left a comment

Uh oh!

gilles-peskine-arm commented Oct 4, 2019

Uh oh!

Patater left a comment

Uh oh!

Patater commented Oct 4, 2019

Uh oh!

Uh oh!

ASN.1 tests without x509 #75

ASN.1 tests without x509 #75

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Patater commented Sep 10, 2019

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!

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!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Oct 4, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater commented Oct 4, 2019

Uh oh!

Uh oh!

gilles-peskine-arm commented Mar 1, 2019 •

edited

Loading