Skip to content
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

[Bug]: PKCS11 issue with wc_ecc_init_ex() #7482

Closed
mrdeep1 opened this issue Apr 29, 2024 · 4 comments · Fixed by #7485
Closed

[Bug]: PKCS11 issue with wc_ecc_init_ex() #7482

mrdeep1 opened this issue Apr 29, 2024 · 4 comments · Fixed by #7485
Assignees
Labels

Comments

@mrdeep1
Copy link
Contributor

mrdeep1 commented Apr 29, 2024

Contact Details

No response

Version

Latest master

Description

$ ./configure --enable-all --enable-dtls13 --enable-pkcs11

Code in wc_ecc_init_ex() is testing for isPkcs11 in passed in key - which in just about every case is uninitialized.

/* Setup dynamic pointers if using normal math for proper freeing */
WOLFSSL_ABI
int wc_ecc_init_ex(ecc_key* key, void* heap, int devId)
{
    int ret      = 0;
#if defined(HAVE_PKCS11)
    int isPkcs11 = 0;
#endif

    if (key == NULL) {
        return BAD_FUNC_ARG;
    }

#if defined(HAVE_PKCS11)
    if (key->isPkcs11) {
        isPkcs11 = 1;
    }
#endif

This (randomly set) isPkcs11 is tested later in this function


#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_ECC)
    #if defined(HAVE_PKCS11)
        if (!isPkcs11)
    #endif
        {
            /* handle as async */
            ret = wolfAsync_DevCtxInit(&key->asyncDev, WOLFSSL_ASYNC_MARKER_ECC,
                                                            key->heap, devId);
        }
#elif defined(HAVE_PKCS11)
    (void)isPkcs11;
#endif

Not sure how this should be fixed.

Reproduction steps

$ ./configure --enable-all --enable-dtls13 --enable-pkcs11

Relevant log output

From libcoap test environment when valgrind is being used to test for issues

+==PID== Conditional jump or move depends on uninitialised value(s)
+==PID==    func wc_ecc_init_ex (ecc.c:6106)
+==PID==    func TLSX_KeyShare_GenEccKey (tls.c:7459)
+==PID==    func TLSX_KeyShare_GenKey (tls.c:7783)
+==PID==    func TLSX_KeyShare_Use (tls.c:9120)
+==PID==    func TLSX_PopulateExtensions (tls.c:13185)
+==PID==    func SendTls13ClientHello (tls13.c:4377)
+==PID==    func SendClientHello (internal.c:28897)
+==PID==    func wolfSSL_connect (ssl.c:9267)

+==PID==  Uninitialised value was created by a heap allocation
+==PID==    func malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
+==PID==    func wolfSSL_Malloc (memory.c:352)
+==PID==    func TLSX_KeyShare_GenEccKey (tls.c:7450)
+==PID==    func TLSX_KeyShare_GenKey (tls.c:7783)
+==PID==    func TLSX_KeyShare_Use (tls.c:9120)
+==PID==    func TLSX_PopulateExtensions (tls.c:13185)
+==PID==    func SendTls13ClientHello (tls13.c:4377)
+==PID==    func SendClientHello (internal.c:28897)
+==PID==    func wolfSSL_connect (ssl.c:9267)
@mrdeep1 mrdeep1 added the bug label Apr 29, 2024
@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Apr 29, 2024

Same is true for int wc_InitRsaKey_ex(RsaKey* key, void* heap, int devId)

@dgarske dgarske self-assigned this Apr 29, 2024
@dgarske
Copy link
Contributor

dgarske commented Apr 29, 2024

Thanks @mrdeep1 for the report. We will take a look.

dgarske added a commit to dgarske/wolfssl that referenced this issue Apr 29, 2024
…and RSA. Resolves possible use of uninitialized value on ECC/RSA key when PKCS11 is enabled. See wolfSSL#7482
@dgarske
Copy link
Contributor

dgarske commented Apr 29, 2024

See #7485 for fix. Thank you for the report.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Apr 29, 2024

@dgarske Changes in #7485 make sense to me. I am not yet in a position to test out the wolfSSL PKCS11 code, but will regress check the rest of the libcoap usage of wolfSSL with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants