20251114-WOLFSSL_BLIND_PRIVATE_KEY-thread-safety #9436

douzzer · 2025-11-14T19:01:43Z

fix races around WOLFSSL_CTX.{privateKey,privateKeyMask,altPrivateKey,altPrivateKeyMask} in WOLFSSL_BLIND_PRIVATE_KEY code paths:

rename wolfssl_priv_der_unblind() to wolfssl_priv_der_blind_toggle(),
add wolfssl_priv_der_unblind() that allocates a temp copy,
add wolfssl_priv_der_unblind_free(),
in wolfssl_priv_der_blind_toggle(), make mask a const arg;

restore const attribute to ctx arg to wolfSSL_CTX_get0_privatekey(), and add explanatory comment.

tested with wolfssl-multi-test.sh ... check-source-text clang-tidy-all-sp-all

Also ran clang-tidy-all-sp-all with LIBWOLFSSL_CONFIGURE_ARGS_OVERRIDE=CFLAGS=-DWOLFSSL_BLIND_PRIVATE_KEY, which is analytically clean, but crashes, unrelated to this PR:

Thread 1 "unit.test" received signal SIGSEGV, Segmentation fault.
0x00007ffff7c7a925 in DoTls13ClientHello (ssl=ssl@entry=0x5555563bcfe0, 
    input=input@entry=0x5555562d2160 "\001", inOutIdx=inOutIdx@entry=0x5555563bd2d4, 
    helloSz=helloSz@entry=518) at src/tls13.c:6937
6937        ssl->session->sessionIDSz = sessIdSz;
(gdb) bt
#0  0x00007ffff7c7a925 in DoTls13ClientHello (ssl=ssl@entry=0x5555563bcfe0, 
    input=input@entry=0x5555562d2160 "\001", inOutIdx=inOutIdx@entry=0x5555563bd2d4, 
    helloSz=helloSz@entry=518) at src/tls13.c:6937
#1  0x00007ffff7c7e405 in DoTls13HandShakeMsgType (ssl=ssl@entry=0x5555563bcfe0, 
    input=input@entry=0x5555562d2160 "\001", inOutIdx=inOutIdx@entry=0x5555563bd2d4, type=1 '\001', 
    size=518, totalSz=<optimized out>) at src/tls13.c:12821
#2  0x00007ffff7c80c79 in DoTls13HandShakeMsg (ssl=ssl@entry=0x5555563bcfe0, input=0x5555562d2160 "\001", 
    inOutIdx=inOutIdx@entry=0x5555563bd2d4, totalSz=totalSz@entry=522) at src/tls13.c:13166
#3  0x00007ffff7bef6ef in DoProcessReplyEx (ssl=ssl@entry=0x5555563bcfe0, 
    allowSocketErr=allowSocketErr@entry=0) at src/internal.c:22932
#4  0x00007ffff7bf0273 in ProcessReplyEx (ssl=0x5555563bcfe0, allowSocketErr=0) at src/internal.c:23305
#5  ProcessReply (ssl=ssl@entry=0x5555563bcfe0) at src/internal.c:23298
#6  0x00007ffff7c25a98 in wolfSSL_accept (ssl=0x5555563bcfe0) at src/ssl.c:11027
#7  0x00005555558b4dad in test_wolfSSL_BIO_accept () at tests/api/test_ossl_bio.c:855
#8  0x0000555555733a97 in ApiTest () at tests/api.c:51996
#9  0x00005555555b8a45 in unit_test (argc=1, argv=0x7fffffffcf18) at tests/unit.c:284
#10 0x00007ffff770b288 in __libc_start_call_main (main=main@entry=0x5555555b85a0 <main>, 
    argc=argc@entry=1, argv=argv@entry=0x7fffffffcf18) at ../sysdeps/nptl/libc_start_call_main.h:58
#11 0x00007ffff770b34b in __libc_start_main_impl (main=0x5555555b85a0 <main>, argc=1, argv=0x7fffffffcf18, 
    init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffcf08)
    at ../csu/libc-start.c:360
#12 0x00005555555b85d5 in _start () at ../sysdeps/x86_64/start.S:115

dgarske · 2025-11-14T21:05:21Z

Jenkins retest this please: PRB-generic-config-parser aborted

julek-wolfssl · 2025-11-14T21:03:01Z

src/internal.c

+void wolfssl_priv_der_blind_toggle(DerBuffer* key, const DerBuffer* mask)
 {
    if (key != NULL) {
        xorbuf(key->buffer, mask->buffer, mask->length);
    }
 }


If key is null checked then mask should be too.

it's incumbent code. not bothering to bikeshed it for present purposes.

While this area is being touched, we can add a null check. It shouldn't have any affect (if it does we're already in trouble).

(fixed it anyway)

julek-wolfssl · 2025-11-14T21:09:02Z

src/internal.c

+    ret = (DerBuffer *)XMALLOC(sizeof(*key) + key->length, key->heap,
+                               DYNAMIC_TYPE_TMP_BUFFER);
+    if (ret == NULL)
+        return NULL;
+    XMEMCPY(ret, key, sizeof(*key));
+    ret->buffer = (byte *)ret + sizeof(*key);


This kind of extra allocation at the end of DerBuffer is an anti-pattern. There should be an explicit field at the end of the struct to let us know that the structure stores data at the end.

Ok I guess that is what AllocDer does. We should consider just moving buffer to the end and changing it to buffer[] (save the space from the pointer).

that sounds like the right idea. I thought of checking for an existing allocator but never got around to it -- will fix.

julek-wolfssl · 2025-11-14T21:11:57Z

src/internal.c

+DerBuffer *wolfssl_priv_der_unblind(const DerBuffer* key, const DerBuffer* mask)
+{
+    DerBuffer *ret;
+    if (key == NULL)
+        return NULL;
+    if (mask->length > key->length)
+        return NULL;
+    ret = (DerBuffer *)XMALLOC(sizeof(*key) + key->length, key->heap,
+                               DYNAMIC_TYPE_TMP_BUFFER);
+    if (ret == NULL)
+        return NULL;
+    XMEMCPY(ret, key, sizeof(*key));
+    ret->buffer = (byte *)ret + sizeof(*key);
+    xorbufout(ret->buffer, key->buffer, mask->buffer, mask->length);
+    return ret;
+}
+
+void wolfssl_priv_der_unblind_free(DerBuffer* key)
+{
+    if (key != NULL) {
+        void *heap = key->heap;
+        ForceZero(key->buffer, key->length);
+        ForceZero(key, sizeof(*key));
+        XFREE(key, heap, DYNAMIC_TYPE_TMP_BUFFER);
+    }
+}


Also you should be using AllocDer and FreeDer.

julek-wolfssl · 2025-11-14T21:17:37Z

src/internal.c

 }

-void wolfssl_priv_der_unblind(DerBuffer* key, DerBuffer* mask)
+void wolfssl_priv_der_blind_toggle(DerBuffer* key, const DerBuffer* mask)


I think I'd prefer wolfssl_der_key_xor_mask.

xor isn't the point here (blinding could be implemented with barrel shifts or other ops), and we're not gonna bikeshed the terminology for present purposes.

I don't like the "toggle" part of the name. Maybe "apply" would be better but I won't block the pr over this.

…,altPrivateKeyMask} in WOLFSSL_BLIND_PRIVATE_KEY code paths: * rename wolfssl_priv_der_unblind() to wolfssl_priv_der_blind_toggle(), * add wolfssl_priv_der_unblind() that allocates a temp copy, * add wolfssl_priv_der_unblind_free(), * in wolfssl_priv_der_blind_toggle(), make mask a const arg; restore const attribute to ctx arg to wolfSSL_CTX_get0_privatekey(), and add explanatory comment.

…wolfssl_priv_der_unblind_free() to use AllocDer() and FreeDer().

douzzer requested review from SparkiDev, julek-wolfssl and wolfSSL-Bot November 14, 2025 19:01

douzzer assigned wolfSSL-Bot and SparkiDev Nov 14, 2025

douzzer added the For This Release Release version 5.8.4 label Nov 14, 2025

This comment was marked as spam.

Sign in to view

dgarske previously approved these changes Nov 14, 2025

View reviewed changes

julek-wolfssl requested changes Nov 14, 2025

View reviewed changes

julek-wolfssl assigned douzzer and unassigned wolfSSL-Bot and SparkiDev Nov 14, 2025

douzzer added 2 commits November 14, 2025 18:13

src/internal.c: peer review: refactor wolfssl_priv_der_unblind() and …

c29abcc

…wolfssl_priv_der_unblind_free() to use AllocDer() and FreeDer().

douzzer dismissed dgarske’s stale review via c29abcc November 15, 2025 00:14

douzzer force-pushed the 20251114-WOLFSSL_BLIND_PRIVATE_KEY-thread-safety branch from e82110b to c29abcc Compare November 15, 2025 00:14

douzzer requested review from dgarske and julek-wolfssl November 15, 2025 00:15

douzzer assigned wolfSSL-Bot and SparkiDev and unassigned douzzer Nov 15, 2025

20251114-WOLFSSL_BLIND_PRIVATE_KEY-thread-safety #9436

Are you sure you want to change the base?

20251114-WOLFSSL_BLIND_PRIVATE_KEY-thread-safety #9436

Conversation

douzzer commented Nov 14, 2025

Uh oh!

This comment was marked as spam.

dgarske commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants