Use memcmp_constant_time for SHA-256 hash comparisons#1004
Use memcmp_constant_time for SHA-256 hash comparisons#1004kodareef5 wants to merge 1 commit intoOpenVPN:masterfrom
Conversation
Three security-critical comparisons use memcmp() on SHA-256 digests where memcmp_constant_time() should be used: - ssl_verify.c:244: certificate fingerprint comparison - ssl_mbedtls.c:1038: TLS certificate hash comparison during renegotiation - init.c:2167-2168: configuration digest comparison The HMAC verification at crypto.c:652 already uses memcmp_constant_time (CVE-2025-13086 fix). These three sites have the same pattern but were not updated.
schwabe
left a comment
schwabe
left a comment
There was a problem hiding this comment.
Three security-critical comparisons use memcmp() on SHA-256 digests where memcmp_constant_time() should be used:
I seriously disagree with these being the same as the CVE. Just because a hash is used does not mean that comparison of it is time critical. Out of the three changes only one might qualify for being better with constant time and even in that case there is (unlike in the auth token path) no way for an attacker to even abuse timing differences.
| return memcmp(a, b, sizeof(struct sha256_digest)) | ||
| || !memcmp(a, &zero, sizeof(struct sha256_digest)); | ||
| return memcmp_constant_time(a, b, sizeof(struct sha256_digest)) | ||
| || !memcmp_constant_time(a, &zero, sizeof(struct sha256_digest)); |
There was a problem hiding this comment.
This is a check if we can keep tun device open or not. The hash is really used to avoid storing all the old options to compare them.
I do not see a reason to make this a constant time check other than blindly applying "hashes must be compared with constant time"
| } | ||
|
|
||
| if (0 != memcmp(old_sha256_hash, sha256_hash, sizeof(sha256_hash))) | ||
| if (0 != memcmp_constant_time(old_sha256_hash, sha256_hash, sizeof(sha256_hash))) |
There was a problem hiding this comment.
This also feels like applying "hashes must be compared with constant time" rather than any actual security scenario.
Also the newer mbed TLS versions do not use this anymore.
| } | ||
| else if (ch1 && ch2 | ||
| && !memcmp(ch1->sha256_hash, ch2->sha256_hash, sizeof(ch1->sha256_hash))) | ||
| && !memcmp_constant_time(ch1->sha256_hash, ch2->sha256_hash, sizeof(ch1->sha256_hash))) |
There was a problem hiding this comment.
This is only one that could maybe be constant time. But then again to have any chance of exploiting this one requires you to be able to generate arbitary certificates that are trusted by CA that is in use by OpenVPN to verify the client certificates.
If that happens you have bigger things to worry about.
|
Thanks for the thorough review, @schwabe. Your points are well taken:
I was applying the pattern too broadly without considering the actual threat model at each call site. Closing this — appreciate the detailed feedback. |
2 participants
Three security-critical comparisons use
memcmp()on SHA-256 digests wherememcmp_constant_time()should be used:ssl_verify.c:244— certificate fingerprint comparisonssl_mbedtls.c:1038— TLS certificate hash comparison during renegotiationinit.c:2167-2168— configuration digest comparisonThe HMAC verification at
crypto.c:652already usesmemcmp_constant_time(CVE-2025-13086 fix). These three sites have the same pattern but were not updated.