X.509: Fix bug in SAN parsing and enhance negative testing #2839
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hanno-becker
added
bug
enhancement
mbed TLS team
needs-review
hanno-becker
force-pushed
the
x509_san_parsing_testing-dev
branch
from
f1abe7f
to
998cbd8
Compare
hanno-becker
force-pushed
the
x509_san_parsing_testing-dev
branch
from
998cbd8
to
9ab98f0
Compare
RonEld
reviewed
Sep 16, 2019
| buf->p = *p; | ||
| buf->len = tag_len; | ||
| *p += buf->len; | ||
| cur->buf = tmp_san_buf; |
There was a problem hiding this comment.
Although technically this works, since it is assigning by value tmp_buf to cur->buf, I don't like that there is an assignment of a structure defined in the stack, without a proper memory copying.
There was a problem hiding this comment.
Yes, that's deliberate - why do you prefer a memcpy()?
There was a problem hiding this comment.
Since you are counting for the struct to be copied by value, it wouldn't do a deep-copy(neither does memcpy actually), and this is not future proof.
In addition, in a matter of style, I always prefer using to copy data from non primitive types.
added 10 commits
September 17, 2019 13:10
Judging from its name, the purpose of the test TBSCertificate v3, ext CertificatePolicies tag, bool len missing in test_suite_x509parse.data is to exercise the X.509 parsing stack's behaviour when parsing a CertificatePolicy extension which lacks the length field of the boolean 'Criticality' value. However, the test fails at an earlier stage due to a mismatch of inner and outer length of the explicit ASN.1 extensions structure. Since we already have tests exercising - mismatch of inner and outer length in the extensions structure, namely 'X509 CRT ASN1 (TBS, inv v3Ext, inner tag invalid)' - missing length of the 'Criticality' field in an extension, namely 'X509 CRT ASN1 (TBS, inv v3Ext, critical length missing)' and since for both tests there's no relevance to the use of the policy extension OID, the test 'TBSCertificate v3, ext CertificatePolicies tag, bool len missing' can be dropped.
This commit moves the X.509 negative parsing tests for the CertificatePolicy extension to the place where negative testing of other extensions happens.
This commit modifies the test X509 CRT ASN1 (TBSCertificate v3, inv CertificatePolicies, data missing) which exercises the behaviour of the X.509 CRT parser when facing a CertificatePolicy extension with empty data field. The following adaptations are made: - The subject ID and issuer ID are modified to have length 0. The previous values `aa` and `bb` are OK, but a generic ASN.1 parser will try to interpret them as ASN.1 tags and fail. For maintainability, it's therefore better to use something that can be parsed as ASN.1, and an empty ID is the easiest solution here. - The TBS part of the certificate wasn't followed by signature algorithm and signature fields, which makes the test incompatible with future changes swapping to breadth-first parsing of certificates.
This commit adds multiple test cases to the X.509 CRT parsing test suite exercising the stack's behaviour when facing CertificatePolicy extensions that are malformed for a variety of reasons. It follows the same scheme as in other negative parsing tests: For each ASN.1 component, have test cases for (a) unexpected tag, (b) missing length, (c) invalid length encoding, (d) length out of bounds.
Fixes Mbed-TLS#2838. See the issue description for more information.
- ASN.1 parsing functions check that length don't exceed buffer bounds, so checks `p + len > end` are redundant. - If `p + len == end`, this is erroneous because we expect further fields, which is automatically caught by the next ASN.1 parsing call. Hence, the two branches handling `p + len >= end` in x509_get_other_name() can be removed. Further, zeroization of the `other_name` structure isn't necessary because it's not confidential (and it's also not performed on other error conditions in this function).
hanno-becker
force-pushed
the
x509_san_parsing_testing-dev
branch
from
9ab98f0
to
f21de55
Compare
gilles-peskine-arm
removed
needs-ci
daverodgman
added
historical-reviewed
daverodgman
added
the
priority-medium
|
Continued in #6882. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: This PR fixes #2838 and adds numerous negative CRT parsing tests for the SubjectAlternativeName extension.
Dependencies: Based on #2836 to avoid conflicts at merge-time.