X.509: Enhance negative testing for CertificatePolicy extension #2836
Conversation
hanno-becker
added
enhancement
mbed TLS team
needs-review
There was a problem hiding this comment.
I put all the data in https://lapo.it/asn1js/ and I am not sure the data is as described in their description.
In addition, all the data has a100a200 which is unclear what it is.
|
|
||
| X509 CRT ASN1 (TBSCertificate v3, inv CertificatePolicies, outer length missing) | ||
| depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C | ||
| x509parse_crt:"3081a8308192a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffffa100a200a30c300a30080603551d20040130300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS + MBEDTLS_ERR_ASN1_OUT_OF_DATA |
There was a problem hiding this comment.
Using https://lapo.it/asn1js/ shows that the issue here is that the policy has an invalid CertPolicyId. (30). Not an invalid outer length as in the test description
There was a problem hiding this comment.
The OCTET STRING should be a SEQUENCE of Policies which themselves are SEQUENCES of PolicyIDs, and this test exercises the 'outer' SEQUENCE TLV not being well-formed.
There was a problem hiding this comment.
I understand it's not being well formed, but I am not sure that it's a matter of outer length missing
|
|
||
| X509 CRT ASN1 (TBSCertificate v3, inv CertificatePolicies, outer length inv encoding) | ||
| depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C | ||
| x509parse_crt:"3081a9308193a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffffa100a200a30d300b30090603551d2004023085300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS + MBEDTLS_ERR_ASN1_INVALID_LENGTH |
There was a problem hiding this comment.
Using https://lapo.it/asn1js/ shows that the issue here is that the policy has an invalid encoding for CertPolicyId. (3085). Not an invalid encoding outer length as in the test description
|
|
||
| X509 CRT ASN1 (TBSCertificate v3, inv CertificatePolicies, unknown critical policy) | ||
| depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C:!MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION | ||
| x509parse_crt:"3081b130819ba0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffffa100a200a315301330110603551d20010101040730053003060100300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE |
There was a problem hiding this comment.
Is MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE the error we expect when an unknown critical extension exists?
Since the certificate contains a critical unknown extension, I think the error code should be more strict than an unknown feature error ( even both are error codes that stop the parsing).
THis is out of scope of this PR, but this comment is for tracking this issue
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.
hanno-becker
force-pushed
the
x509_crt_policies_tests
branch
from
19f4897
to
6dfa665
Compare
|
CI passed on merge result, but GitHub doesn't display any acknowledgement of this. OK to merge. |
Patater
removed
needs-ci
Summary: This PR extends negative X.509 CRT tests related to CertificatePolicies: