1822450 - CKM_PBE_SHA1_DES2_EDE_CBC generates a 24-byte key, but validateSecretKey expects a 16-byte key

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

[joachim]

Steps to reproduce:

In nsspkcs5_FillInParam, https://github.com/nss-dev/nss/blob/master/lib/softoken/lowpbe.c#L886, pbe_param->keyLen is erroneously set to 24 for SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_2KEY_TRIPLE_DES_CBC. As a result, CKM_PBE_SHA1_DES2_EDE_CBC generates a 24-byte key. This key is then validated all the way through sftk_handleObject -> sftk_handleKeyObject -> sftk_handleSecretKeyObject -> validateSecretKey. The key type is set to CKK_DES2 (nsc_SetupPBEKeyGen, https://github.com/nss-dev/nss/blob/master/lib/softoken/pkcs11c.c#L4507), so validateSecretKey expects a key of length 16 (https://github.com/nss-dev/nss/blob/master/lib/softoken/pkcs11.c#L1399-L1403).

Actual results:

Using CKM_PBE_SHA1_DES2_EDE_CBC returns a CKR_KEY_SIZE_RANGE error.

Expected results:

CKM_PBE_SHA1_DES2_EDE_CBC correctly generates the DES2 key of length 16 and exits without failure.

Comment 1

[joachim]

Note the fallthrough in:

    switch (algorithm) {
        /* DES3 Algorithms */
        case SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_2KEY_TRIPLE_DES_CBC:
            pbe_param->is2KeyDES = PR_TRUE;
        /* fall through */
        case SEC_OID_PKCS12_V2_PBE_WITH_SHA1_AND_3KEY_TRIPLE_DES_CBC:
            pbe_param->pbeType = NSSPKCS5_PKCS12_V2;
        /* fall through */
        case SEC_OID_PKCS12_PBE_WITH_SHA1_AND_TRIPLE_DES_CBC:
            pbe_param->keyLen = 24;
            pbe_param->encAlg = SEC_OID_DES_EDE3_CBC;
            break;
    ...
}

Comment 2

[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 auto_nag documentation.

Comment 3

[joachim]

Attachment: 0001-Bug-1822450.patch

The first fall through was removed to fix this bug, but I took the liberty of removing the second one as well for consistency and clarity.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Joachim - we use phabricator to review changes to NSS now: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html (tag #nss-reviewers for reviews)
Also, can you include tests in your patches to validate your changes? Thanks!

Comment 5

[joachim]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #4)

Joachim - we use phabricator to review changes to NSS now: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html (tag #nss-reviewers for reviews)
Also, can you include tests in your patches to validate your changes? Thanks!

I'm looking at how Phabricator works now. Is it possible to submit patches to Phabricator without moz-phab?

Comment 6

[joachim]

Attachment: Bug 1822450: Fix CKM_PBE_SHA1_DES2_EDE_CBC derivation. r=#nss-reviewers,rrelyea,nkulatova

Comment 7

[joachim]

Dana, could you let me know if that looks ok? If so, I'll use Phabricator for the other patches as well.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for putting that together! I left a couple of comments.

Comment 9

[Anna Weine]

Attachment: Bug 1822450 - Fix len computation of derived buffer (pk11_pbe_unittest.cc)

Comment 10

[Anna Weine]

Attachment: Bug 1822450 - Backed out changeset dcb174139e4f

Comment 11

[Anna Weine]

Fixed by the reporter.

https://hg.mozilla.org/projects/nss/rev/1fae668cec0f046ed19738b8cce603503cab4f48