1826035 - SP 800-108 KDFs incorrectly accept CKM_MD2_HMAC and CKM_MD5_HMAC

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P4)

Description

[joachim]

PKCS#11 (v3.0), Table 161 specifies the allowed PRFs for SP 800-108 KDFs. I copy the relevant table here for convenience:

CKM_SHA_1_HMAC
CKM_SHA224_HMAC
CKM_SHA256_HMAC
CKM_SHA384_HMAC
CKM_SHA512_HMAC
CKM_SHA3_224_HMAC
CKM_SHA3_256_HMAC
CKM_SHA3_384_HMAC
CKM_SHA3_512_HMAC
CKM_DES3_CMAC
CKM_AES_CMAC

NSS attempts to implement this table as follows:

    /* Start with checking the prfType as a mechanism against a list of
     * PRFs allowed by PKCS#11 v3.0. */
    if (!(/* The following types aren't defined in NSS yet. */
          /* params->prfType != CKM_3DES_CMAC && */
          params->prfType == CKM_AES_CMAC || /* allow */
          /* We allow any HMAC except MD2 and MD5. */
          params->prfType != CKM_MD2_HMAC ||                        /* disallow */
          params->prfType != CKM_MD5_HMAC ||                        /* disallow */
          sftk_HMACMechanismToHash(params->prfType) != HASH_AlgNULL /* Valid HMAC <-> HASH isn't NULL */
          )) {
        return CKR_MECHANISM_PARAM_INVALID;
    }

(source: kbkdf_ValidateParameters in https://github.com/nss-dev/nss/blob/master/lib/softoken/kbkdf.c)

However, this code is incorrect because of the CKM_MD2_HMAC and CKM_MD5_HMAC checks: any prfType will always be not equal to one of those (potentially both). Therefore, the full expression will always validate to true, and the if statement body will never be entered.

Consider, for example, params->prfType = CKM_RIPEMD128_HMAC:

• CKM_RIPEMD128_HMAC == CKM_AES_CMAC -> false
• CKM_RIPEMD128_HMAC != CKM_MD2_HMAC -> true
• CKM_RIPEMD128_HMAC != CKM_MD5_HMAC -> true
• sftk_HMACMechanismToHash(CKM_RIPEMD128_HMAC) != HASH_AlgNULL -> false
• !(false || true || true || false) -> false
  (short-circuiting omitted)

Luckily, in this case, NSS does not implement CKM_RIPEMD128_HMAC in sftk_MAC_InitRaw (called by kbkdf_RawDispatch), therefore the execution will stop there.

Now consider the example of CKM_MD2_HMAC:

• CKM_MD2_HMAC == CKM_AES_CMAC -> false
• CKM_MD2_HMAC != CKM_MD2_HMAC -> false
• CKM_MD2_HMAC != CKM_MD5_HMAC -> true
• sftk_HMACMechanismToHash(CKM_MD2_HMAC) != HASH_AlgNULL -> true
• !(false || false || true || true) -> false

However, NSS does implement CKM_MD2_HMAC in sftk_MAC_InitRaw (https://github.com/nss-dev/nss/blob/master/lib/softoken/sftkhmac.c). Execution successfully completes, contrary to the standard (and contrary to SP 800-108).

One attempt at salvaging this if statement would be:

    if (!(/* The following types aren't defined in NSS yet. */
          /* params->prfType != CKM_3DES_CMAC && */
          params->prfType == CKM_AES_CMAC || /* allow */
          /* We allow any HMAC except MD2 and MD5. */
          (params->prfType != CKM_MD2_HMAC &&                       /* disallow */
          params->prfType != CKM_MD5_HMAC &&                        /* disallow */
          sftk_HMACMechanismToHash(params->prfType) != HASH_AlgNULL /* Valid HMAC <-> HASH isn't NULL */
          ))) {
        return CKR_MECHANISM_PARAM_INVALID;
    }

Note that the mapping in sftk_HMACMechanismToHash (https://github.com/nss-dev/nss/blob/master/lib/softoken/sftkhmac.c) currently includes CKM_SSL3_MD5_MAC and CKM_SSL3_SHA1_MAC, both of which are not allowed by PKCS#11. Therefore, this fixed if statement would allow these functions:

• CKM_SSL3_MD5_MAC == CKM_AES_CMAC -> false
• CKM_SSL3_MD5_MAC != CKM_MD2_HMAC -> true
• CKM_SSL3_MD5_MAC != CKM_MD5_HMAC -> true
• sftk_HMACMechanismToHash(CKM_SSL3_MD5_MAC) != HASH_AlgNULL -> true
• !(false || (true && true && true)) -> false

However, sftk_MAC_InitRaw does not implement CKM_SSL3_MD5_MAC and CKM_SSL3_SHA1_MAC, and I do not anticipate that it will in the future. They are implemented in a very specific function, sftk_SSLv3MACConstantTime_New. Again, execution stops at kbkdf_RawDispatch.

Still, it might be prudent to enhance the if statement to avoid unintended bugs in the future:

    if (!(/* The following types aren't defined in NSS yet. */
          /* params->prfType != CKM_3DES_CMAC && */
          params->prfType == CKM_AES_CMAC || /* allow */
          /* We allow any HMAC except MD2 and MD5. */
          (params->prfType != CKM_MD2_HMAC &&                       /* disallow */
          params->prfType != CKM_MD5_HMAC &&                        /* disallow */
          params->prfType != CKM_SSL3_MD5_MAC &&                    /* disallow */
          params->prfType != CKM_SSL3_SHA1_MAC &&                   /* disallow */
          sftk_HMACMechanismToHash(params->prfType) != HASH_AlgNULL /* Valid HMAC <-> HASH isn't NULL */
          ))) {
        return CKR_MECHANISM_PARAM_INVALID;
    }

Note that this if statement still relies on consistency with sftk_HMACMechanismToHash, that is, if sftk_HMACMechanismToHash would define more MACs, and sftk_MAC_InitRaw would implement those, the execution would succeed.

Perhaps the most future-proof solution for this sanity check is to simply list each allowed PRF mechanism explicitly.

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

Comment 2

[Benjamin Beurdouche [:beurdouche]]

Bob, any opinion on this ?

Comment 3

[Robert Relyea]

This is a PKCS #11 correctness issue, and sort of a FIPS correctness issue. The latter is bigger bug can be handled with indicators.

Comment 4

[joachim]

I don't agree that this is an issue with the PKCS#11 standard, PKCS#11 specifically allows only

CKM_SHA_1_HMAC
CKM_SHA224_HMAC
CKM_SHA256_HMAC
CKM_SHA384_HMAC
CKM_SHA512_HMAC
CKM_SHA3_224_HMAC
CKM_SHA3_256_HMAC
CKM_SHA3_384_HMAC
CKM_SHA3_512_HMAC
CKM_DES3_CMAC
CKM_AES_CMAC

From https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/pkcs11-curr-v3.0.html, table 161. The only one questionable there might be CKM_DES3_CMAC, but that's a different can of worms.

The problem is that the NSS implementation does not implement this correctly. The if statement has a logic bug which will always allow any MAC mechanism. Compare

    if (!(/* The following types aren't defined in NSS yet. */
          /* params->prfType != CKM_3DES_CMAC && */
          params->prfType == CKM_AES_CMAC || /* allow */
          /* We allow any HMAC except MD2 and MD5. */
          params->prfType != CKM_MD2_HMAC ||                        /* disallow */
          params->prfType != CKM_MD5_HMAC ||                        /* disallow */
          sftk_HMACMechanismToHash(params->prfType) != HASH_AlgNULL /* Valid HMAC <-> HASH isn't NULL */
          )) {

and

    if (!(/* The following types aren't defined in NSS yet. */
          /* params->prfType != CKM_3DES_CMAC && */
          params->prfType == CKM_AES_CMAC || /* allow */
          /* We allow any HMAC except MD2 and MD5. */
          (params->prfType != CKM_MD2_HMAC &&                       /* disallow */
          params->prfType != CKM_MD5_HMAC &&                        /* disallow */
          sftk_HMACMechanismToHash(params->prfType) != HASH_AlgNULL /* Valid HMAC <-> HASH isn't NULL */
          ))) {

Comment 5

[Robert Relyea]

How is that different from what I said?
I said it was a PKCS #11 correctness issue.

I also said it could be a FIPS issue which is a bigger issue because if softoken doesn't return error, it could cause it to fail a validation (mitigated by the fact the module could use an indicator to handle the issue.

The PKCS #11 correctness isn't a high priority issue here because 1) it's far from the worst or most damaging non-compliance by softoken, and 2) from a compatibility issue, only an application written to test compliance would notice the issue.

Comment 6

[joachim]

OK, maybe I misinterpreted "PKCS #11 correctness". Yes it would be a FIPS issue though.

Comment 7

[joachim]

Attachment: 0001-Bug-1826035.patch

After thinking it over again, I think it would be more maintainable to just use an allowlist rather than mixing allows and disallows. Otherwise this code would need to be updated whenever sftk_HMACMechanismToHash is updated.

Comment 8

[joachim]

Attachment: Bug 1826035 - Use explicit allowlist for for KDF PRFS r=rrelyea,nkulatova

Comment 9

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/d9894739abfa2a456d6dd11c47f9bed6ac4efb1c