Repository navigation
Conversation
|
e069aa8 to
142c6f6
Compare
142c6f6 to
8e11d4f
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12062
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-port-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/asn_public.h
Coverage: 10 of 12 in-scope changed file(s) opened by the reviewer; not opened: wolfssl/internal.h, wolfssl/wolfcrypt/asn.h
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
| ERROR_OUT(BAD_MUTEX_E, exit_scv); | ||
| } | ||
| FreeDer(&ssl->buffers.keyMask); | ||
| ssl->buffers.keyMask = ssl->buffers.altKeyMask; |
There was a problem hiding this comment.
BLIND alt-key swap leaves altKeyMask stale after the exit re-blind · Logic errors
After the swap, key/altKey share one buffer. On success, wolfssl_priv_der_blind() at exit_scv re-blinds that buffer under a new keyMask, but altKeyMask keeps the old mask. A later ALTERNATIVE CertificateVerify (e.g. TLS 1.3 post-handshake auth) then unblinds with the wrong mask and signs with a corrupted key.
Suggested fix: In WOLFSSL_BLIND_PRIVATE_KEY builds, give ssl->buffers.key its own unblinded copy of the alt key instead of aliasing altKey and altKeyMask.
Basis: wolfssl_priv_der_blind() (src/internal.c:333) frees and regenerates only the mask pointer passed to it and XORs the new mask into the key buffer, so any other pointer to the previous mask is left stale.
Related known findings (similar but distinct; listed for context, not part of this finding)
- F-13391: that one is about the CTX key ID/label setters keeping a stale mask; this one is the TLS 1.3 alt-key aliasing in SendTls13CertificateVerify, where the mask goes out of sync after the exit re-blind.
| * expected. Inspect the request sent to the responder instead. */ | ||
| (void)test_memio_do_handshake(ssl_c, ssl_s, 10, NULL); | ||
| ExpectIntEQ(test_ocsp_ctx_request_cache_cb_cnt, 4); | ||
| ExpectIntEQ(test_ocsp_ctx_request_cache_differs, 1); |
There was a problem hiding this comment.
Reload OCSP test cannot detect reuse of the stale cached request · Weak or missing assertions
differs is only compared with the first server1 request. The cached request's serial was already XOR-corrupted at i == 1, so reusing that stale cached request also gives differs == 1. The new reload block passes whether or not the CTX cache is bypassed after the certificate reload.
Suggested fix: Undo the serial corruption before the reload step, or capture the post-reload request and assert it carries server2's serial.
Description
Fixes Fenrir #16791
When certificate or key copying is disabled, a
WOLFSSLobject can share DER buffers with itsWOLFSSL_CTX. Replacing a certificate chain or private key on the context afterwolfSSL_new()can release a buffer that the existing connection still needs and cause use-after-free.This change adds reference counting to
DerBufferso that a context and its connections can retain the same certificate, chain, or private key buffer and make the buffer freed only when its last reference is released.Background
This revisits the reference-counting approach proposed in #11109, which superseded #10905 and was closed without being merged. The approach in #11191 instead blocks certain context changes during callbacks regarding handshake.
The case addressed here does not require a context change inside a callback: the context can be always reloaded after
wolfSSL_new()and before the connection uses its certificate or key and that is the condition of the case.