Skip to content

Cleanup EVP_DH asn1 parsing#3047

Merged
justsmth merged 1 commit intoaws:mainfrom
justsmth:cleanup-p_dh_asn1
Mar 5, 2026
Merged

Cleanup EVP_DH asn1 parsing#3047
justsmth merged 1 commit intoaws:mainfrom
justsmth:cleanup-p_dh_asn1

Conversation

@justsmth
Copy link
Contributor

Description of changes:

Fixes several issues in crypto/evp_extra/p_dh_asn1.c:

  1. dh_pub_decode: Fixed an unreachable double-free. After ownership of pubkey is transferred to the DH object via dh->pub_key = pubkey, the error path would free it twice — once through DH_free(dh) and again via BN_free(pubkey). This is currently unreachable because EVP_PKEY_assign_DH always succeeds when dh is non-NULL, but a future refactor could easily trigger it. Fixed by nulling the local pointer after the ownership transfer.

  2. dh_pub_decode: Added a CBS_len(key) != 0 check after parsing the public key to reject trailing data, consistent with the RSA, DSA, and EC pub_decode implementations.

  3. dh_param_copy: Fixed a NULL dereference when to->pkey.dh is NULL by allocating a new DH object on demand.

  4. dh_pub_encode: Added an early NULL check for the DH object and its public key.

Call-outs:

These are defensive fixes hardening error paths and edge cases.

Testing:

Existing tests continue to pass.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.35%. Comparing base (04e7dc0) to head (c3a42d6).

Files with missing lines Patch % Lines
crypto/evp_extra/p_dh_asn1.c 50.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3047   +/-   ##
=======================================
  Coverage   78.35%   78.35%           
=======================================
  Files         689      689           
  Lines      121041   121050    +9     
  Branches    16968    16973    +5     
=======================================
+ Hits        94839    94854   +15     
+ Misses      25306    25300    -6     
  Partials      896      896           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@justsmth justsmth enabled auto-merge (squash) March 5, 2026 15:53
@justsmth justsmth merged commit de4383e into aws:main Mar 5, 2026
473 of 483 checks passed
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Mar 11, 2026
### Description of changes:
Fixes several issues in `crypto/evp_extra/p_dh_asn1.c`:

1. **`dh_pub_decode`:** Fixed an unreachable double-free. After
ownership of `pubkey` is transferred to the `DH` object via `dh->pub_key
= pubkey`, the error path would free it twice — once through
`DH_free(dh)` and again via `BN_free(pubkey)`. This is currently
unreachable because `EVP_PKEY_assign_DH` always succeeds when `dh` is
non-NULL, but a future refactor could easily trigger it. Fixed by
nulling the local pointer after the ownership transfer.

2. **`dh_pub_decode`:** Added a `CBS_len(key) != 0` check after parsing
the public key to reject trailing data, consistent with the RSA, DSA,
and EC `pub_decode` implementations.

3. **`dh_param_copy`:** Fixed a NULL dereference when `to->pkey.dh` is
NULL by allocating a new `DH` object on demand.

4. **`dh_pub_encode`:** Added an early NULL check for the `DH` object
and its public key.

### Call-outs:
These are defensive fixes hardening error paths and edge cases.

### Testing:
Existing tests continue to pass. 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants