Skip to content

x509asn1: fix DH public key parameter extraction#21595

Closed
sergio-correia wants to merge 1 commit into
curl:masterfrom
sergio-correia:x509asn1
Closed

x509asn1: fix DH public key parameter extraction#21595
sergio-correia wants to merge 1 commit into
curl:masterfrom
sergio-correia:x509asn1

Conversation

@sergio-correia

Copy link
Copy Markdown
Contributor

The dh(g) parameter was read from param->beg instead of from the cursor p returned by parsing dh(p). This caused dh(g) to always report the same value as dh(p) when inspecting DH certificates via CURLOPT_CERTINFO on non-OpenSSL backends.

The DSA branch correctly advances the cursor; the DH branch lost this during what appears to be a copy-paste.

Add unit1676 to verify that dh(p) and dh(g) report distinct values using a hand-crafted minimal DER certificate.

Comment thread tests/unit/unit1676.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

This PR fixes DH parameter extraction in the ASN.1 certificate info parser so that dh(g) is read from the correct cursor position (instead of re-reading dh(p)), restoring correct CURLOPT_CERTINFO output on non-OpenSSL backends that use lib/vtls/x509asn1.c.

Changes:

  • Fix cursor usage when extracting DH g parameter in do_pubkey() for dhpublicnumber.
  • Add unit1676 with a minimal DER certificate to ensure dh(p), dh(g), and dh(pub_key) are extracted distinctly.
  • Register the new unit test in the unit and test-data makefiles.

Reviewed changes

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

Show a summary per file
File Description
lib/vtls/x509asn1.c Fixes DH g element parsing to use the advanced cursor returned after parsing p.
tests/unit/unit1676.c Adds a unit test exercising the DH public key parsing path via Curl_extract_certinfo().
tests/unit/Makefile.inc Includes unit1676.c in the unit test build list.
tests/data/test1676 Adds the test case definition for the new unit test.
tests/data/Makefile.am Registers test1676 in the test data makefile list.

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

Comment thread tests/unit/unit1676.c
The dh(g) parameter was read from param->beg instead of from the
cursor p returned by parsing dh(p). This caused dh(g) to always
report the same value as dh(p) when inspecting DH certificates
via CURLOPT_CERTINFO on non-OpenSSL backends.

The DSA branch correctly advances the cursor; the DH branch lost
this during what appears to be a copy-paste.

Add unit1676 to verify that dh(p) and dh(g) report distinct values
using a hand-crafted minimal DER certificate.

Assisted-by: Claude Opus 4.6
Signed-off-by: Sergio Correia <scorreia@redhat.com>
@bagder bagder closed this in 61d59c9 May 15, 2026
@bagder

bagder commented May 15, 2026

Copy link
Copy Markdown
Member

Thanks!

outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
The dh(g) parameter was read from param->beg instead of from the
cursor p returned by parsing dh(p). This caused dh(g) to always
report the same value as dh(p) when inspecting DH certificates
via CURLOPT_CERTINFO on non-OpenSSL backends.

The DSA branch correctly advances the cursor; the DH branch lost
this during what appears to be a copy-paste.

Add unit1676 to verify that dh(p) and dh(g) report distinct values
using a hand-crafted minimal DER certificate.

Assisted by: Claude Opus 4.6
Signed-off-by: Sergio Correia <scorreia@redhat.com>
Closes curl#21595
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