Skip to content

Fixes for PKCS12_set_mac#3079

Merged
justsmth merged 1 commit intoaws:mainfrom
justsmth:PKCS12_set_mac
Mar 11, 2026
Merged

Fixes for PKCS12_set_mac#3079
justsmth merged 1 commit intoaws:mainfrom
justsmth:PKCS12_set_mac

Conversation

@justsmth
Copy link
Contributor

@justsmth justsmth commented Mar 6, 2026

Description of changes:

  • Use-after-free on BER input: CBS_asn1_ber_to_der may allocate a DER conversion buffer (storage) that backs all subsequent CBS parsing. The function freed it immediately after the call, but the CBS values derived from it were still in use. Moved the free to the cleanup label, matching PKCS12_get_key_and_certs and PKCS12_handle_sequence.

  • Dangling ber_bytes on CBB_finish failure: The old p12->ber_bytes was freed before CBB_finish, so a failure left p12 with a dangling pointer. Now we null out the pointer and zero the length immediately after freeing, so p12 stays in a safe (empty) state on failure.

  • Negative salt_len / password_len not rejected: Both are signed int but flow into size_t parameters downstream. Negative values now return early. password_len == -1 is normalized via strlen to match the convention in PKCS12_verify_mac.

Call-outs:

The password_len == -1 (use strlen) convention is already supported by PKCS12_verify_mac, which this function calls at the end. Without the normalization, the MAC generation and verification would use different password lengths and the function would always fail for that input.

Testing:

All 16 existing PKCS12Test.* tests pass, including PKCS12Test.SetMac.

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.

@justsmth justsmth requested a review from a team as a code owner March 6, 2026 17:23
@justsmth justsmth changed the title Fixes for `PKCS12_set_mac Fixes for PKCS12_set_mac Mar 6, 2026
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.38%. Comparing base (092453f) to head (33a4723).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crypto/pkcs8/pkcs8_x509.c 69.23% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3079      +/-   ##
==========================================
+ Coverage   78.36%   78.38%   +0.01%     
==========================================
  Files         689      689              
  Lines      121144   121154      +10     
  Branches    16973    16974       +1     
==========================================
+ Hits        94935    94961      +26     
+ Misses      25314    25298      -16     
  Partials      895      895              

☔ 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 requested review from nebeid and removed request for dkostic March 9, 2026 13:29
@justsmth justsmth enabled auto-merge (squash) March 11, 2026 00:51
@justsmth justsmth merged commit 1b40140 into aws:main Mar 11, 2026
628 of 635 checks passed
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Mar 11, 2026
### Description of changes:
- **Use-after-free on BER input:** `CBS_asn1_ber_to_der` may allocate a
DER conversion buffer (`storage`) that backs all subsequent CBS parsing.
The function freed it immediately after the call, but the CBS values
derived from it were still in use. Moved the free to the cleanup label,
matching `PKCS12_get_key_and_certs` and `PKCS12_handle_sequence`.

- **Dangling `ber_bytes` on `CBB_finish` failure:** The old
`p12->ber_bytes` was freed before `CBB_finish`, so a failure left `p12`
with a dangling pointer. Now we null out the pointer and zero the length
immediately after freeing, so `p12` stays in a safe (empty) state on
failure.

- **Negative `salt_len` / `password_len` not rejected:** Both are signed
`int` but flow into `size_t` parameters downstream. Negative values now
return early. `password_len == -1` is normalized via `strlen` to match
the convention in `PKCS12_verify_mac`.

### Call-outs:
The `password_len == -1` (use strlen) convention is already supported by
`PKCS12_verify_mac`, which this function calls at the end. Without the
normalization, the MAC generation and verification would use different
password lengths and the function would always fail for that input.

### Testing:
All 16 existing `PKCS12Test.*` tests pass, including
`PKCS12Test.SetMac`.

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