opened
hanno-arm
username_0
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).
Remove unnecessary uses of MD2 in X.509 parsing test suite
issue
created
hanno-arm
username_0
@username_1 @username_2 I need to rework this PR, my approach to replacing MD2 by SHA-256 was too careless. Please don't review this for now.
comment
created
hanno-arm
username_0
@username_1 Could you take another look?
comment
created
hanno-arm
username_0
Nothing has changed since @username_2's approval - I think it got lost when @username_1 re-requested it. Was there a particular reason for that, @username_1?
comment
created
mpg
username_1
I don't remember doing that, so probably no particular reason ^^
comment
created
hanno-arm
username_0
Ping @Patater for gatekeeping.
comment
created
mpg
username_1
This fixes #821 which is labeled as a bug, so relabeling this with "bug", and hence needing backports.
comment
created
mpg
username_1
(Demoting from "ready for merge" as it needs backports.)
comment
created
hanno-arm
username_0
@k-stachowiak @Patater @username_2 I had to force-push this to resolve a ChangeLog conflict with `development`. Can you please re-review?
comment
created
hanno-arm
username_0
@Patater @username_2 I fixed the ChangeLog issue Jaeden spotted, and also rebased on top of `development`. Could you please re-review?
comment
created
AndrzejKurek
username_2
Please ping me when the remaining issues are resolved @username_0.
comment
created
hanno-arm
username_0
Thanks @Patater for spotting the inaccuracies, I believe to have addressed them. Could you please re-review?

Ping @username_2 for re-review, too!
comment
created
hanno-arm
username_0
@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 @username_2 Can you please re-review?
comment