957727 - Incorrect parentheses around the key object attribute checks for CKK_NSS_JPAKE_ROUND1 in sftk_handlePrivateKeyObject

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

Nico Weber of Google reported this bug. It was found by his experimental clang warning.

In nss/lib/softoken/pkcs11.c, function sftk_handlePrivateKeyObject, we have:

    case CKK_NSS_JPAKE_ROUND1:
        if (!sftk_hasAttribute(object, CKA_PRIME ||
            !sftk_hasAttribute(object, CKA_SUBPRIME) ||
            !sftk_hasAttribute(object, CKA_BASE))) {
            return CKR_TEMPLATE_INCOMPLETE;
        }

The closing parenthesis ')' after CKA_PRIME is missing. Since CKA_PRIME is
nonzero (0x00000130), the second and third sftk_hasAttribute calls are not
evaluated, and this code is equivalent to:

    case CKK_NSS_JPAKE_ROUND1:
        if (!sftk_hasAttribute(object, 1)) {
            return CKR_TEMPLATE_INCOMPLETE;
        }

1 is the value of CKA_TOKEN. I guess |object| always has the CKA_TOKEN
attribute because sftk_handleObject calls

    crv = sftk_defaultAttribute(object,CKA_TOKEN,&ckfalse,sizeof(CK_BBOOL));

and then calls sftk_handleKeyObject, which is the only function that calls
sftk_handlePrivateKeyObject. This must be why this check hasn't failed in
practice.

Note: based on the code in jpake_Round1, I think this should also check
for the CKA_NSS_JPAKE_SIGNERID attribute.

The patch also fixes a missing break statement in nss/lib/softoken/pkcs11c.c,
function NSC_GenerateKey, which I noticed while reviewing code related to
CKK_NSS_JPAKE_ROUND1.

Comment 1

[Wan-Teh Chang]

Patch checked in: https://hg.mozilla.org/projects/nss/rev/47e537892f5a