Skip to content

openssl: use non-deprecated API for MD5/MD4 - #22221

Closed
vszakats wants to merge 8 commits into
curl:masterfrom
vszakats:openssl-md5-nodeprec
Closed

vszakats wants to merge 8 commits into
curl:masterfrom
vszakats:openssl-md5-nodeprec

Conversation

@vszakats

@vszakats vszakats commented Jun 29, 2026 •

Copy link
Copy Markdown
Member

Rework MD4 and MD5 code to use the modern EVP_Digest_*() API,
replacing the deprecated MD4_*() and MD5_*() ones. It allows to use
these OpenSSL crypto primitives with OpenSSL built with the
no-deprecated option.


Also:


  • split the original goal from a) leak fixes / result verification b) naming sync into separate PRs. [KEPT THE ORIGINAL GOAL IN THIS, MOVING THE REST TO FUTURE PRs]
  • also cross-sync with curl_sha512_256 → [SEPARATE PR]
  • add more low-level error handling to hash update/final functions. → [SEPARATE PR]
  • replace null digest solution with finishing anyway? How would that work [NOT A FAN, SKIP]
  • or add null digest support to sha512_256

@vszakats
vszakats marked this pull request as draft June 29, 2026 20:21
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch 2 times, most recently from 2b51a82 to 21b2f3f Compare June 29, 2026 21:32
@vszakats
vszakats marked this pull request as ready for review June 29, 2026 21:38
@vszakats
vszakats requested a review from Copilot June 29, 2026 21:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates curl’s OpenSSL-backed MD5/MD4 implementations to use the EVP digest API instead of the deprecated low-level MD5_* / MD4_* functions, keeping curl compatible with newer OpenSSL deprecation policies.

Changes:

  • Switch MD5 OpenSSL backend from <openssl/md5.h> + MD5_* to <openssl/evp.h> + EVP_Digest*.
  • Switch MD4 OpenSSL backend from <openssl/md4.h> + MD4_* to <openssl/evp.h> + EVP_Digest*.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
lib/md5.c Replaces OpenSSL MD5 low-level API usage with EVP-based MD5 context management.
lib/md4.c Replaces OpenSSL MD4 low-level API usage with EVP-based MD4 context management.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/md5.c Outdated
Comment thread lib/md5.c Outdated
Comment thread lib/md4.c Outdated
Comment thread lib/md4.c

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread lib/md5.c Outdated
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch from ec8a23c to 9ced360 Compare June 30, 2026 00:12
@vszakats vszakats changed the title openssl: use non-deprecated API for MD5/MD4 openssl: use non-deprecated API for MD5/MD4, sync up SHA256 code Jun 30, 2026
@vszakats vszakats changed the title openssl: use non-deprecated API for MD5/MD4, sync up SHA256 code openssl: use non-deprecated API for MD5/MD4 Jun 30, 2026
@vszakats
vszakats requested a review from Copilot June 30, 2026 00:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread lib/sha256.c Outdated
Comment thread lib/md5.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread lib/hmac.c
@vszakats
vszakats marked this pull request as draft June 30, 2026 21:43
Copilot AI review requested due to automatic review settings July 27, 2026 09:44
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch from 01b8413 to 029d4cc Compare July 27, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/curl_sha512_256.c:176

  • The new digest == NULL cleanup pattern (used from Curl_HMAC_init()'s fail: path) is only implemented for the OpenSSL SHA-512/256 backend here. The wolfSSL and GnuTLS/Netttle Curl_sha512_256_finish() implementations in this same file still unconditionally write to digest, which will crash when hfinal(NULL, ...) is used during cleanup. Please add the same if(digest) handling (and any required abort/reset) to the other backend branches to keep the HMAC cleanup contract consistent across TLS backends.
  if(digest) {
#ifdef NEED_NETBSD_SHA512_256_WORKAROUND
    /* Use a larger buffer to work around a bug in NetBSD:
       https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58039 */
    unsigned char tmp_digest[CURL_SHA512_256_DIGEST_SIZE * 2];
    result = EVP_DigestFinal_ex(*ctx, tmp_digest, NULL) ?
      CURLE_OK : CURLE_BAD_FUNCTION_ARGUMENT;
    if(result == CURLE_OK)
      memcpy(digest, tmp_digest, CURL_SHA512_256_DIGEST_SIZE);
    curlx_memzero(tmp_digest, sizeof(tmp_digest));
#else /* !NEED_NETBSD_SHA512_256_WORKAROUND */
    result = EVP_DigestFinal_ex(*ctx, digest, NULL) ?
      CURLE_OK : CURLE_BAD_FUNCTION_ARGUMENT;
#endif /* NEED_NETBSD_SHA512_256_WORKAROUND */
  }
  EVP_MD_CTX_free(*ctx);

Comment thread lib/curl_sha512_256.c
static CURLcode Curl_sha512_256_finish(unsigned char *digest, void *context)
static CURLcode Curl_sha512_256_finish(unsigned char *digest, void *in)
{
CURLcode result;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the most annoying thing in LLM review being enabled by default, is
that it's trying to do the job of compilers. It's redundant, wasteful and annoying.

The second one is that it makes at least me half-insane with the barrage of
reports, unread notifications (those are also coming for PRs actively monitored
and reacted-to), especially when working on a PR that's not ready yet. Maybe
something to get used to, or something that'll eventually make me disable this.

The 3rd one is when doing experimental PRs and LLM going crazy with mostly
useless reports.

(I did not enable automatic reviews, still don't know how/why it happened, and
where's the toggle to control it.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I did not enable automatic reviews, still don't know how/why it happened, and
where's the toggle to control it.)

It was some magic GitHub did themselves for untold reasons. I don't know how to switch it back to on-demand either, but I have asked someone.

@vszakats vszakats Jul 27, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @bagder!

My guess so far is that it's a personal setting. It's unset on my profile, an
omission (or dark pattern) where neither disabled/enabled is selected,
and the actual setting sourced maybe from a GitHub global default. It's
irreversible to the current state once one selects something, so did not
touch it yet. (ref "Automatic Copilot code review")

Copilot AI review requested due to automatic review settings July 27, 2026 11:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch from dec4035 to 45f26b4 Compare August 17, 2026 21:49
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch from 45f26b4 to 9ffd481 Compare September 4, 2026 07:44
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch 4 times, most recently from c00528d to 114cbb1 Compare September 5, 2026 10:21
@vszakats
vszakats force-pushed the openssl-md5-nodeprec branch from 179bc1c to b1e375d Compare September 5, 2026 10:43
@vszakats
vszakats marked this pull request as ready for review September 5, 2026 11:06
@vszakats

vszakats commented Sep 5, 2026 •

Copy link
Copy Markdown
Member Author

The non-flaky CI failure is unrelated and happening in master since 9be5c68.
https://github.com/curl/curl/actions/runs/33891130575/job/101082778404#step:43:159

edit: fix PR → #22834

@vszakats vszakats closed this in 68ed69a Sep 5, 2026
@vszakats
vszakats deleted the openssl-md5-nodeprec branch September 5, 2026 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants