-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix multiple reported issues. #11009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
65230fc
688cca5
d6009a6
1d3ebc0
26c55a6
90f3ecc
2f74a6a
9449c68
5fd6966
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11691,17 +11691,21 @@ static int wolfSSLReceive(WOLFSSL* ssl, byte* buf, word32 sz) | |
| } | ||
|
|
||
|
|
||
| /* Switch dynamic output buffer back to static, buffer is assumed clear */ | ||
| /* Switch dynamic output buffer back to static, discarding any pending output */ | ||
| void ShrinkOutputBuffer(WOLFSSL* ssl) | ||
| { | ||
| WOLFSSL_MSG("Shrinking output buffer"); | ||
| XFREE(ssl->buffers.outputBuffer.buffer - ssl->buffers.outputBuffer.offset, | ||
| ssl->heap, DYNAMIC_TYPE_OUT_BUFFER); | ||
| if (ssl->buffers.outputBuffer.dynamicFlag) { | ||
| XFREE(ssl->buffers.outputBuffer.buffer - | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need ForceZero()? |
||
| ssl->buffers.outputBuffer.offset, | ||
| ssl->heap, DYNAMIC_TYPE_OUT_BUFFER); | ||
| } | ||
| ssl->buffers.outputBuffer.buffer = ssl->buffers.outputBuffer.staticBuffer; | ||
| ssl->buffers.outputBuffer.bufferSize = STATIC_BUFFER_LEN; | ||
| ssl->buffers.outputBuffer.dynamicFlag = 0; | ||
| ssl->buffers.outputBuffer.offset = 0; | ||
| /* idx and length are assumed to be 0. */ | ||
| ssl->buffers.outputBuffer.idx = 0; | ||
| ssl->buffers.outputBuffer.length = 0; | ||
| } | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5659,6 +5659,17 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, | |
| ssl->earlyData = no_early_data; | ||
| ssl->earlyDataSz = 0; | ||
| #endif | ||
| #ifdef HAVE_RPK | ||
| { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not use empty brace. Thanks |
||
| /* Drop the negotiated cert types so one peer's choice cannot carry | ||
| * into the next handshake. isRPKLoaded describes the local | ||
| * certificate, not the negotiation, so it survives. */ | ||
| int rpkLoaded = ssl->options.rpkState.isRPKLoaded; | ||
| XMEMSET(&ssl->options.rpkState, 0, | ||
| sizeof(ssl->options.rpkState)); | ||
| ssl->options.rpkState.isRPKLoaded = rpkLoaded; | ||
| } | ||
| #endif | ||
|
|
||
| #if defined(HAVE_TLS_EXTENSIONS) && !defined(NO_TLS) | ||
| TLSX_FreeAll(ssl->extensions, ssl->heap); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -858,7 +858,6 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| { | ||
| int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE); | ||
| int done = 0; | ||
| int added = 0; | ||
| int i = 0; | ||
| int numFailedCerts = 0; | ||
| int depth = 0; | ||
|
|
@@ -997,11 +996,8 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| &depth, origDepth); | ||
| continue; | ||
| } | ||
| added = 1; | ||
| ret = X509StoreVerifyCert(ctx); | ||
| if (ret != WOLFSSL_SUCCESS) { | ||
| if ((origDepth - depth) <= 1) | ||
| added = 0; | ||
| X509VerifyCertSetupRetry(ctx, certs, failedCerts, | ||
| &depth, origDepth); | ||
| continue; | ||
|
|
@@ -1023,13 +1019,10 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| != WOLFSSL_SUCCESS) { | ||
| /* Could not guarantee the temporary intermediates were | ||
| * dropped; fail closed rather than risk verifying the current | ||
| * certificate against one. Leave `added` set: they are still | ||
| * loaded, so the exit cleanup makes a final attempt to drop | ||
| * them. */ | ||
| * certificate against one. */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At line 1030 drop the |
||
| ret = WOLFSSL_FATAL_ERROR; | ||
| goto exit; | ||
| } | ||
| added = 0; | ||
| ret = X509StoreVerifyCert(ctx); | ||
| if (ret != WOLFSSL_SUCCESS) { | ||
| /* WOLFSSL_PARTIAL_CHAIN may only terminate the chain at a | ||
|
|
@@ -1149,12 +1142,11 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| } | ||
| } | ||
| } | ||
| /* Remove intermediates that were added to CM */ | ||
| /* Remove intermediates that were added to CM. Unconditional: anything left | ||
| * resident anchors verification for every other user of this CM. */ | ||
| if (ctx != NULL) { | ||
| if (ctx->store != NULL) { | ||
| if (added == 1) { | ||
| wolfSSL_CertManagerUnloadTempIntermediateCerts(ctx->store->cm); | ||
| } | ||
| wolfSSL_CertManagerUnloadTempIntermediateCerts(ctx->store->cm); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider comment indicating return code is not checked or have |
||
| } | ||
| if (orig != NULL) { | ||
| ctx->current_cert = orig; | ||
|
|
@@ -1777,13 +1769,15 @@ static void X509StoreFreeObjList(WOLFSSL_X509_STORE* store, | |
| * the numAdded to the store >= is used when comparing to 0. */ | ||
| i = wolfSSL_sk_X509_OBJECT_num(objs) - 1; | ||
| while (cnt > 0 && i >= 0) { | ||
| /* The inner X509 is owned by somebody else, NULL out the reference */ | ||
| obj = (WOLFSSL_X509_OBJECT *)wolfSSL_sk_X509_OBJECT_value(objs, i); | ||
| if (obj != NULL) { | ||
| /* Only certificates are borrowed, so only they consume numAdded. The | ||
| * CRL object appended after them must not shift this window. */ | ||
| if (obj != NULL && obj->type == WOLFSSL_X509_LU_X509) { | ||
| /* The inner X509 is owned by somebody else, NULL out the ref */ | ||
| obj->type = (WOLFSSL_X509_LOOKUP_TYPE)0; | ||
| obj->data.ptr = NULL; | ||
| cnt--; | ||
| } | ||
| cnt--; | ||
| i--; | ||
| } | ||
|
|
||
|
|
@@ -2522,6 +2516,9 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( | |
| { | ||
| WOLFSSL_STACK* ret = NULL; | ||
| WOLFSSL_STACK* cert_stack = NULL; | ||
| /* Set once the certificates have been handed over to "ret". Until then | ||
| * cert_stack still owns them and the error path must not free them. */ | ||
| byte certsOwned = 0; | ||
| #if ((defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM)) || \ | ||
| (defined(HAVE_CRL))) | ||
| WOLFSSL_X509_OBJECT* obj = NULL; | ||
|
|
@@ -2595,6 +2592,7 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( | |
| while (wolfSSL_sk_X509_num(cert_stack) > 0) { | ||
| wolfSSL_sk_X509_pop(cert_stack); | ||
| } | ||
| certsOwned = 1; | ||
| #endif | ||
|
|
||
| #ifdef HAVE_CRL | ||
|
|
@@ -2625,8 +2623,28 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( | |
| store->objs = ret; | ||
| return ret; | ||
| err_cleanup: | ||
| if (ret != NULL) | ||
| X509StoreFreeObjList(store, ret); | ||
| if (ret != NULL) { | ||
| if (certsOwned) { | ||
| X509StoreFreeObjList(store, ret); | ||
| } | ||
| else { | ||
| /* cert_stack still owns these certificates. pop_free() below runs | ||
| * wolfSSL_X509_OBJECT_free() on each entry, which frees the inner | ||
| * X509, so clear the references or they are freed twice. */ | ||
| int j; | ||
| WOLFSSL_X509_OBJECT* cur; | ||
|
|
||
| for (j = 0; j < wolfSSL_sk_X509_OBJECT_num(ret); j++) { | ||
| cur = (WOLFSSL_X509_OBJECT*)wolfSSL_sk_X509_OBJECT_value(ret, | ||
| j); | ||
| if (cur != NULL) { | ||
| cur->type = (WOLFSSL_X509_LOOKUP_TYPE)0; | ||
| cur->data.ptr = NULL; | ||
| } | ||
| } | ||
| wolfSSL_sk_X509_OBJECT_pop_free(ret, NULL); | ||
| } | ||
| } | ||
| if (cert_stack != NULL) { | ||
| while (store->numAdded > 0) { | ||
| wolfSSL_sk_X509_pop(cert_stack); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the
XFREEis guarded internally, the callers at src/internal.c:9326 (FreeSSL), src/internal.c:11876 (SendBuffered) and src/sniffer.c:6638 all wrap the call in an outerif (ssl->buffers.outputBuffer.dynamicFlag)that no longer serves a purpose - only the DTLSSendAlert()site (src/internal.c:28569) actually needed the new guard. Leaving both makes it ambiguous which layer owns the precondition. This is cosmetic and does not affect behavior; flagged only so the PR can decide where the check should live.Recommendation: Either remove the now-redundant caller-side guards or document that the function is safe to call unconditionally