Remove unnecessary uses of MD2 in X.509 parsing test suite #2430
Conversation
hanno-becker
added
enhancement
needs-review
| @@ -12,7 +12,7 @@ x509_cert_info:"data_files/test-ca.crt":"cert. version \: 3\nserial number | |||
|
|
|||
| X509 Certificate information MD2 Digest | |||
| depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_MD2_C | |||
| x509_cert_info:"data_files/cert_md2.crt":"cert. version \: 3\nserial number \: 09\nissuer name \: C=NL, O=PolarSSL, CN=PolarSSL Test CA\nsubject name \: C=NL, O=PolarSSL, CN=PolarSSL Cert MD2\nissued on \: 2009-07-12 10\:56\:59\nexpires on \: 2011-07-12 10\:56\:59\nsigned using \: RSA with MD2\nRSA key size \: 2048 bits\nbasic constraints \: CA=false\n" | |||
| x509_cert_info:"data_files/cert_md2.crt":"cert. version \: 3\nserial number \: 09\nissuer name \: C=NL, O=PolarSSL, CN=PolarSSL Test CA\nsubject name \: C=NL, O=PolarSSL, CN=PolarSSL Cert MD2\nissued on \: 2009-07-12 10\:56\:59\nexpires on \: 2011-07-12 10\:56\:59\nsigned using \: RSA with SHA-256\nRSA key size \: 2048 bits\nbasic constraints \: CA=false\n" | |||
There was a problem hiding this comment.
Do you really mean SHA-256 here? The name of the test case seems pretty explicit about testing MD2. (A few other occurrences below.)
There was a problem hiding this comment.
@mpg No, I'm sorry, that was careless.
hanno-becker
added
needs-work
and removed
needs-review
hanno-becker
force-pushed
the
x509_parse_test_fixes
branch
from
fb0f232 to
2219bed
Compare
hanno-becker
added
needs-review
|
@mpg Could you take another look? |
|
Nothing has changed since @AndrzejKurek's approval - I think it got lost when @mpg re-requested it. Was there a particular reason for that, @mpg? |
|
I don't remember doing that, so probably no particular reason ^^ |
hanno-becker
removed
the
needs-review
|
Ping @Patater for gatekeeping. |
Patater
added
the
approved
|
This fixes #821 which is labeled as a bug, so relabeling this with "bug", and hence needing backports. |
The example programs programs/x509/cert_req and programs/x509/cert_write (demonstrating the use of X.509 CSR and CRT writing functionality) previously didn't support MD2 signatures. For testing purposes, this commit adds support for MD2 to cert_req, and support for MD2 and MD4 to cert_write.
|
@gilles-peskine-arm Thanks for your review. The PR doesn't make the situation worse, because it only replaces MD2 by SHA-256 in parsing tests, and a MD2-specific parsing test remains ("X509 Certification information MD2 digest"). Nonetheless, you have a point that there's a pre-existing gap in the tests in that we don't exercise verification of an MD2 signed certificate, and I've added an analogue of "Valid Cert MD4 Digest" for MD2. Also, I noticed that these tests don't expect success because they use a profile which forbids MDx, and I've added three tests which use a test-only profile allowing all MDs, so that we now also cover successful verification of MDx-signed CRTs. @gilles-peskine-arm @AndrzejKurek Can you please re-review? |
include/mbedtls/config.h
Outdated
| @@ -2511,7 +2511,7 @@ | |||
| * it, and considering stronger message digests instead. | |||
| * | |||
| */ | |||
| //#define MBEDTLS_MD2_C | |||
| #define MBEDTLS_MD2_C | |||
There was a problem hiding this comment.
You accidentally committed a modified config.h. Feel free to rebase the offending commit as far as I'm concerned.
The X.509 parsing test suite test_suite_x509parse contains a test exercising X.509 verification for a valid MD4/MD5 certificate in a profile which doesn't allow MD4/MD5. This commit adds an analogous test for MD2.
hanno-becker
force-pushed
the
x509_parse_test_fixes
branch
from
6335f7b to
024b53a
Compare
hanno-becker
removed
the
needs-review
Patater
added
the
approved
As has been reported in #821, many test cases in the X.509 parsing test suite use MD2 in places where an arbitrary hash algorithm would be supported. This is bad for two reasons: Firstly, it shows bad practice using a weak digest, and secondly, it significantly reduces negative test coverage in the default configuration which doesn't have MD2 enabled.
This PR changes the tests to use SHA-256 in place of MD2 in those instances where a general hash algorithm is allowed (and we're not specifically testing support for MD2).