1596450 - softoken: unified MAC implementation

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Alexander Scheel]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

As I hinted in a previous bug I'd like to submit changes necessary for SCP 03 to be implemented via NSS. When I started that bug, I didn't have a copy of the spec, hence some confusion. Now I do!

There's two parts to working SCP03 support in NSS:

• CMAC (done) -- necessary for MACing as hinted, but also for the KDF.
• the SCP03 KDF, a NIST SP800-108 KBKDF variant.

I've been working on the latter in my fork on GitHub. That's not ready yet, but in the process I realized there's a bit of code that needs "a" MAC and could support HMAC, CMAC, or XCBC MAC.

That's mostly:

• The HMACs/CMACs themselves
• Softoken's IKE implementation
• The new KBKDF implementation (sans XCBC).
• I'm sure others I've forgotten about.

So, I'd like to get this started with a cleanup attempt: unify HMAC+CMAC into a mechanism-switched interface. Call it with the mechanism type of the MAC (not the hash!) and get something usable, with common semantics, out.

This opens the doors for other improvements in the future:

• Return code could be checked when updating the MAC, if desired. This requires updating sftk_doMACInit to condition around whether or not the function could be called and its return code checked. I'm not sure if it is worth it. I've left it alone for now.
• Bringing XCBC into the fold and updating softoken IKE to use the common interface.

I don't have much interest now in working on that (I'd prefer to get the KBKDF cleaned up so I can submit it), but I'm mentioning it in case someone else wishes to do that. I'd consider that out of scope currently.

Actual results:

More work to support both HMAC+CMAC.

Expected results:

Unified MAC interface.

Comment 1

[Alexander Scheel]

Attachment: Bug 1596450 - softoken: unified MAC implementation

Simplify HMAC and CMAC construction and usage behind a common PKCS#11
interface. This ensures that return values can be checked by the caller
for both MACs if desired and simplifies the implementation of KDFs and
PRFs capable of using both.

Comment 2

[J.C. Jones [:jcj] (he/him) [taking a break]]

https://hg.mozilla.org/projects/nss/rev/3147585149f0e907b39bda15e804ec8fb33a0138

Comment 3

[J.C. Jones [:jcj] (he/him) [taking a break]]

Coverity has found a defect in https://hg.mozilla.org/projects/nss/rev/3147585149f0e907b39bda15e804ec8fb33a0138

*** CID 1455952:  Null pointer dereferences  (FORWARD_NULL)
/lib/softoken/sftkhmac.c: 235 in sftk_MAC_Init()
229     
230         ret = sftk_MAC_InitRaw(ctx, mech,
231                                (const unsigned char *)keyval->attrib.pValue,
232                                keyval->attrib.ulValueLen, isFIPS);
233     
234     done:
>>>     CID 1455952:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "keyval" to "sftk_FreeAttribute", which dereferences it.
235         sftk_FreeAttribute(keyval);
236         return ret;
237     }
238     
239     CK_RV
240     sftk_MAC_InitRaw(sftk_MACCtx *ctx, CK_MECHANISM_TYPE mech, const unsigned char *key, unsigned int key_len, PRBool isFIPS)

It appears to be a correct analysis. We'll need to fix this this cycle. Can you make a fixup, Alexander? Just guarding with a if (keyval) should be fine.

Comment 4

[J.C. Jones [:jcj] (he/him) [taking a break]]

Nevermind, attaching a patch, marking you as a reviewer.

Comment 5

[J.C. Jones [:jcj] (he/him) [taking a break]]

Attachment: Bug 1596450 - Fixup, coverity CID 1455952 r?cipherboy,kjacobs

Comment 6

[J.C. Jones [:jcj] (he/him) [taking a break]]

https://hg.mozilla.org/projects/nss/rev/46b1355d87656704008d1a6da0d7be08bc984ebb