Compat layer: fix memory leak in X509_get0_pubkey - #11467
padelsbach wants to merge 1 commit into
Conversation
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11467
Scan targets checked: wolfssl-src, wolfssl-bugs
Coverage: 2 of 6 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_ossl_x509.c, wolfssl/internal.h, wolfssl/openssl/ssl.h, wolfssl/ssl.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: Lite
| if (x509 == NULL) | ||
| return NULL; | ||
|
|
||
| if (x509->pubKeyCache == NULL) |
There was a problem hiding this comment.
pubKeyCache is not invalidated when the certificate public key changes · Logic errors
wolfSSL_X509_set_pubkey (src/x509.c:16911-16913) replaces pubKey.buffer and pubKeyOID but leaves pubKeyCache alone. After that call, X509_get0_pubkey keeps returning the old key, so code that verifies or compares with the returned key uses the wrong public key.
Suggested fix: In wolfSSL_X509_set_pubkey, free pubKeyCache and set it to NULL after a successful update, and add a test that calls get0, then set_pubkey, then get0 again.
| return NULL; | ||
|
|
||
| if (x509->pubKeyCache == NULL) | ||
| x509->pubKeyCache = wolfSSL_X509_get_pubkey(x509); |
There was a problem hiding this comment.
Unsynchronized lazy init of pubKeyCache races and leaks on shared X509 · Race conditions
The getter writes pubKeyCache with a check-then-set and no lock. If two threads call X509_get0_pubkey on the same reference-counted X509 (for example a CA cert in a shared store), both allocate a key and one overwrites the other's pointer. The overwritten EVP_PKEY (over 11 KB) is leaked, and the unsynchronized write is a data race.
Suggested fix: Fill the cache under the X509's existing ref/lock mutex, and free the losing allocation, or decode the key once when the certificate is created.
Basis: C11 5.1.2.4p25: two conflicting unsynchronized accesses to the same object, at least one a write, are a data race and undefined behavior.
|
this functionality (and more) is covered by #11428. closing |
5 participants
Description
Found that compatibility layer routine
X509_get0_pubkeyallocates a newEVP_KEYon each call, with a footprint of over 11kb. This change adds a new compat layer function to return the pointer from within the existing struct, following OpenSSL's convention.Other routines in this family do not have this issue: X509_get_subject_name, X509_get_issuer_name, X509_get_serialNumber, X509_get0_notBefore and X509_NAME_get_entry
Testing
Added unit tests
Checklist