Potential use of an uninitialized pointer "registered_mt"

[popovec]

Problem Description

When creating RSA mechanisms, I found a potential use of an uninitialized pointer (registered_mt).

src/pkcs11/framework-pkcs15.c function register_mechanisms():

               sc_pkcs11_mechanism_type_t *mt, *registered_mt;
.
.
.
                mt = sc_pkcs11_new_fw_mechanism(CKM_RSA_PKCS, &mech_info, CKK_RSA, NULL, NULL, NULL);
                rc = sc_pkcs11_register_mechanism(p11card, mt, &registered_mt);
                sc_pkcs11_free_mechanism(&mt);
                if (rc != CKR_OK)
                        return rc;
#ifdef ENABLE_OPENSSL
                /* sc_pkcs11_register_sign_and_hash_mechanism expects software hash */
                /* All hashes are in OpenSSL
                 * Either the card set the hashes or we helped it above */

                if (rsa_flags & SC_ALGORITHM_RSA_HASH_SHA1) {
                        rc = sc_pkcs11_register_sign_and_hash_mechanism(p11card,
                                CKM_SHA1_RSA_PKCS, CKM_SHA_1, registered_mt);

We rely on the function sc_pkcs11_register_mechanism() to always set the pointer registered_mt. However, the sc_pkcs11_register_mechanism() function returns CKR_OK even in cases where the mechanism was not copied and the next initialization of the pointer registered_mt does not occur.

part of sc_pkcs11_register_mechanism()

                for (i = 0; i < MAX_KEY_TYPES; i++) {
                        if (existing_mt->key_types[i] == mt->key_types[0]) {
                                /* Mechanism already registered with the same key_type,
                                 * just update it's info */
                                _update_mech_info(&existing_mt->mech_info, &mt->mech_info);
                                return CKR_OK;
                        }

Fortunately, when this code is actually executed, registered_mt initialization will always occur.

If the pointer registered_mt is set (by calling sc_pkcs11_register_mechanism()), it points to a copy of the information pointed to by the pointer mt . I suggest changing this part of the initialization and not using registered_mt at all.
It is enough if after calling sc_pkcs11_register_mechanism() we do not call sc_pkcs11_free_mechanism(&mt); and instead of registered_mt we use mt to initialize SHA* using the function sc_pkcs11_register_sign_and_hash_mechanism().

Suggested code:

                rc = sc_pkcs11_register_mechanism(p11card, mt, NULL);
#ifdef ENABLE_OPENSSL
                if(rc == CKR_OK){
                     if (rsa_flags & SC_ALGORITHM_RSA_HASH_SHA1) {
                        rc = sc_pkcs11_register_sign_and_hash_mechanism(p11card,
                                CKM_SHA1_RSA_PKCS, CKM_SHA_1, mt);
                        if (rc != CKR_OK)
                                goto err;
                    }
                     .... SHA 256
                     .... SHA 384
                     .... SHA 512
                }
             err:
#endif /* ENABLE_OPENSSL */
                sc_pkcs11_free_mechanism(&mt);
                if (rc != CKR_OK)
                        return rc;

Will someone look at it and check if I missed something?

Similar code is in the initialization of RSA_*_PSS and EC mechanisms...
Thanks.

[Jakuje]

This was introduced as part of #2516. I think there were some memory issues before as it was storing several copies of the mechanism in some internal structures, making it impossible to free afterward. Certainly we should not use uninitialized values and if the memory management can be somehow simplified, I am all for it.