1851528 - C_CreateObject for RSA private keys with CRT parameters set signals an assertion failure (SIGABRT)

Status: UNCONFIRMED

(NSS :: Libraries, defect, P4)

Description

[joachim]

RSA_PopulatePrivateKey at https://github.com/nss-dev/nss/blob/master/lib/freebl/rsa.c#L760 takes an RSAPrivateKey as input, which is passed by the PKCS#11 layer. This struct can have many parameters set, such as N, e, d, or the CRT parameters (d mod p - 1, d mod q - 1, q^-1 mod p). (Also called CKA_EXPONENT_1, CKA_EXPONENT_2, and CKA_COEFFICIENT in PKCS#11).

Then, rsa_build_from_primes is called at https://github.com/nss-dev/nss/blob/master/lib/freebl/rsa.c#L882, which computes the CRT parameters again and attempts to set them in the RSAPrivateKey struct: https://github.com/nss-dev/nss/blob/master/lib/freebl/rsa.c#L168. However, it does not delete the old data first. Compare:

    CHECK_MPI_OK(mp_mod(d, &psub1, &tmp));
    MPINT_TO_SECITEM(&tmp, &key->exponent1, key->arena);
    /* 5.  Compute exponent2 = d mod (q-1) */
    CHECK_MPI_OK(mp_mod(d, &qsub1, &tmp));
    MPINT_TO_SECITEM(&tmp, &key->exponent2, key->arena);
    /* 6.  Compute coefficient = q**-1 mod p */
    CHECK_MPI_OK(mp_invmod(q, p, &tmp));
    MPINT_TO_SECITEM(&tmp, &key->coefficient, key->arena);

with

    key->modulus.data = NULL;
    MPINT_TO_SECITEM(&n, &key->modulus, key->arena);
    key->privateExponent.data = NULL;
    MPINT_TO_SECITEM(d, &key->privateExponent, key->arena);
    key->publicExponent.data = NULL;
    MPINT_TO_SECITEM(e, &key->publicExponent, key->arena);
    key->prime1.data = NULL;
    MPINT_TO_SECITEM(p, &key->prime1, key->arena);
    key->prime2.data = NULL;
    MPINT_TO_SECITEM(q, &key->prime2, key->arena);

MPINT_TO_SECITEM will attempt to allocate new memory using SECITEM_AllocItem, which contains an assertion PORT_Assert(item->data == NULL); here: https://github.com/nss-dev/nss/blob/master/lib/util/secitem.c#L34

I suspect the solution is as simple as setting the data to NULL first:

    CHECK_MPI_OK(mp_mod(d, &psub1, &tmp));
    key->exponent1.data = NULL;
    MPINT_TO_SECITEM(&tmp, &key->exponent1, key->arena);
    /* 5.  Compute exponent2 = d mod (q-1) */
    CHECK_MPI_OK(mp_mod(d, &qsub1, &tmp));
    key->exponent2.data = NULL;
    MPINT_TO_SECITEM(&tmp, &key->exponent2, key->arena);
    /* 6.  Compute coefficient = q**-1 mod p */
    CHECK_MPI_OK(mp_invmod(q, p, &tmp));
    key->coefficient.data = NULL;
    MPINT_TO_SECITEM(&tmp, &key->coefficient, key->arena);

Note that this assertion might not trigger in release builds.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:beurdouche, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Benjamin Beurdouche [:beurdouche]]

Anna, can you please have a quick look at this and check priority and a severity ? : ) Thanks !

Comment 3

[joachim]

Attachment: 0001-Bug-1851528.patch