Skip to content
Open
12 changes: 8 additions & 4 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the XFREE is 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 outer if (ssl->buffers.outputBuffer.dynamicFlag) that no longer serves a purpose - only the DTLS SendAlert() 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

XFREE(ssl->buffers.outputBuffer.buffer -

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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;
}


Expand Down
11 changes: 11 additions & 0 deletions src/ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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);
Expand Down
33 changes: 33 additions & 0 deletions src/tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -13541,6 +13541,22 @@ static int TLSX_ClientCertificateType_Parse(WOLFSSL* ssl, const byte* input,
else if (msgType == server_hello || msgType == encrypted_extensions) {
/* parse it in client side */
if (length == 1) {
/* Same offered-vs-received binding as server_cert_type: an
* unsolicited value lets the peer pick the form this client
* presents its own credential in. */
if (ssl->options.rpkState.sending_ClientCertTypeCnt == 0) {
WOLFSSL_MSG("client_cert_type received but never offered");
WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION);
return UNSUPPORTED_EXTENSION;
}
if (!IsCertTypeListed(*input,
ssl->options.rpkState.sending_ClientCertTypeCnt,
ssl->options.rpkState.sending_ClientCertTypes)) {
WOLFSSL_MSG("client_cert_type value was not offered");
WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION);
return UNSUPPORTED_EXTENSION;
}

ssl->options.rpkState.received_ClientCertTypeCnt = 1;
ssl->options.rpkState.received_ClientCertTypes[0] = *input;
}
Expand Down Expand Up @@ -13741,6 +13757,23 @@ static int TLSX_ServerCertificateType_Parse(WOLFSSL* ssl, const byte* input,
if (length != 1) /* length slould be 1 */
return BUFFER_E;

/* RFC 7250 4.1, RFC 8446 4.2: the server may only answer with a type
* the client offered. ProcessPeerCertParse() treats the stored value as
* negotiated, so an unsolicited one lets the peer select RawPublicKey
* and skip chain verification. */
if (ssl->options.rpkState.sending_ServerCertTypeCnt == 0) {
WOLFSSL_MSG("server_cert_type received but never offered");
WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION);
return UNSUPPORTED_EXTENSION;
}
if (!IsCertTypeListed(*input,
ssl->options.rpkState.sending_ServerCertTypeCnt,
ssl->options.rpkState.sending_ServerCertTypes)) {
WOLFSSL_MSG("server_cert_type value was not offered");
WOLFSSL_ERROR_VERBOSE(UNSUPPORTED_EXTENSION);
return UNSUPPORTED_EXTENSION;
}

ssl->options.rpkState.received_ServerCertTypeCnt = 1;
ssl->options.rpkState.received_ServerCertTypes[0] = *input;
}
Expand Down
52 changes: 35 additions & 17 deletions src/x509_str.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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. */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At line 1030 drop the added == 1 reference now that the variable no longer exists.

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
Expand Down Expand Up @@ -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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider comment indicating return code is not checked or have (void) or front or even consider a WOLFSSL_MSG on failure.

}
if (orig != NULL) {
ctx->current_cert = orig;
Expand Down Expand Up @@ -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--;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
39 changes: 39 additions & 0 deletions tests/api/test_dtls13.c
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,45 @@ int test_dtls13_epochs(void) {
return EXPECT_RESULT();
}

int test_dtls13_alert_with_pending_output(void)
{
EXPECT_DECLS;
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_DTLS13)
WOLFSSL_CTX *ctx_c = NULL;
WOLFSSL_CTX *ctx_s = NULL;
WOLFSSL *ssl_c = NULL;
WOLFSSL *ssl_s = NULL;
struct test_memio_ctx test_ctx;
char msg[1300];

XMEMSET(&test_ctx, 0, sizeof(test_ctx));
XMEMSET(msg, 'A', sizeof(msg));

ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfDTLSv1_3_client_method, wolfDTLSv1_3_server_method), 0);
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);

/* Stall the transport, then queue a record big enough that adding the
* alert would exceed the MTU. */
test_ctx.s_force_want_write = 1;
ExpectIntLT(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), 0);
ExpectIntGT((int)ssl_s->buffers.outputBuffer.length, 1288);

/* EndOfEarlyData is not valid in DTLS 1.3 and raises a fatal alert. */
ExpectIntEQ(Dtls13CheckEpoch(ssl_s, end_of_early_data), SANITY_MSG_E);

ExpectIntLE((int)(ssl_s->buffers.outputBuffer.idx +
ssl_s->buffers.outputBuffer.length),
(int)ssl_s->buffers.outputBuffer.bufferSize);

wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}

/*-- ack_order (test_dtls.c lines 873,951) ---*/
int test_dtls13_ack_order(void)
{
Expand Down
2 changes: 2 additions & 0 deletions tests/api/test_dtls13.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ int test_dtls13_basic_connection_id(void);
int test_dtls13_hrr_want_write(void);
int test_dtls13_every_write_want_write(void);
int test_dtls13_epochs(void);
int test_dtls13_alert_with_pending_output(void);
int test_dtls13_ack_order(void);
int test_dtls13_ack_overflow(void);
int test_dtls13_ack_dup_write_counter(void);
Expand Down Expand Up @@ -71,6 +72,7 @@ int test_dtls13_reuse_after_clear(void);
TEST_DECL_GROUP("dtls13", test_dtls13_hrr_want_write), \
TEST_DECL_GROUP("dtls13", test_dtls13_every_write_want_write), \
TEST_DECL_GROUP("dtls13", test_dtls13_epochs), \
TEST_DECL_GROUP("dtls13", test_dtls13_alert_with_pending_output), \
TEST_DECL_GROUP("dtls13", test_dtls13_ack_order), \
TEST_DECL_GROUP("dtls13", test_dtls13_ack_overflow), \
TEST_DECL_GROUP("dtls13", test_dtls13_ack_dup_write_counter), \
Expand Down
Loading
Loading