1570501 - Add CMAC to FreeBL and PKCS #11 libraries

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Alexander Scheel]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.100 Safari/537.36

Steps to reproduce:

(sorry if this is the wrong component, feel free to move)

I want to utilize CMAC (defined in NIST 800-38B and RFC 4493) via NSS's PKCS #11 interface. This is utilized, among other places, in Global Platform's SCP 03.

Actual results:

NSS doesn't yet implement or expose CMAC.

Expected results:

CKM_AES_CMAC and CKM_AES_CMAC_GENERAL should be implemented. I'll attach patches to do so after filing this bug. :)

I asked Bob Relyea for an informal review prior to submitting these patches. In case anyone wishes to review past conversation, the discussion is available here: https://github.com/cipherboy/nss/pull/2

There's two patches here:

1. Adds CMAC to FreeBL. The biggest design decision was using a union to represent the pointer to the context for the other ciphers.
2. Exposes CMAC from the PKCS #11 layer. I used v2.40 as reference; I'm not sure what version it was introduced in.

Remaining questions:

• I'm not too fond of implementing this for other ciphers at this point in time. I don't need DES (and PKCS #11 doesn't expose DES-based CMAC constants). If DES is desired though, I could add it to the FreeBL layer. Does the interface look extensible enough in general though?
• Do we want all of the elements to be allocated directly inside the struct instead of heap allocations? This includes k1, k2, partialBlock and lastBlock. This becomes 4*16 = 64 bytes (32 bytes additional) instead of using pointers. Thoughts? Do we care?
• What do we want to do about the return types? It is unlikely (impossible?) that the block cipher will fail during CMAC_{Update,sFinish}, but CMAC_{Update,Finish} both return SECStatus. HMAC_{Update,Finish} return void, so the softtoken code doesn't know to check the return type of hashUpdate and end calls. Should this be addressed in a later patch? Is changing the return type of HMAC_{Update,Finish} to return SECStatus within scope? Should I just ignore the error in CMAC_{Update,Finish}?

Thanks everyone!

Comment 1

[Alexander Scheel]

Attachment: 0001-Add-AES-CMAC-freebl.patch

Patch 1/2 - rev 1

Comment 2

[Alexander Scheel]

Attachment: 0002-Expose-AES-CMAC-PKCS-11.patch

Patch 2/2 - rev 1

Comment 3

[J.C. Jones [:jcj] (he/they)]

For patches of this complexity, please submit them via Phabricator. The user's guide is here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

It's a little bit of one-time setup, but the review/update cycle is so much cleaner. Thanks!

Comment 4

[Alexander Scheel]

Attachment: Bug 1570501 - Add AES-CMAC implementation to freebl

Comment 5

[Alexander Scheel]

Attachment: Bug 1570501 - Expose AES-CMAC in PKCS #11 API

Depends on D40120

Comment 6

[Alexander Scheel]

Does this look better? I think I submitted them correctly... :)

Not quite sure what happened here:

Submitting 2 commits for review:
(New) 15243:3c88c390aa26 Bug 1570501 - Expose AES-CMAC in PKCS #11 API
!! Missing reviewers
(New) 15242:986eb1640dbd Bug 1570501 - Add AES-CMAC implementation to freebl
!! Missing reviewers
snip
created new head
rebasing 15242:986eb1640dbd "Bug 1570501 - Add AES-CMAC implementation to freebl"
rebasing 15246:18668964ff35 "Bug 1570501 - Add AES-CMAC implementation to freebl" (tip)
rebasing 15243:3c88c390aa26 "Bug 1570501 - Expose AES-CMAC in PKCS #11 API" (moz-phab_3c88c390aa26)
snip

But it looks like only two got submitted so it should be good.

Thanks,

• Alex

Comment 7

[J.C. Jones [:jcj] (he/they)]

Yup, looks good. Probably we'll start review of this and your next week.

Could you give us a brief rundown of where you need to use CMAC - e.g., whether you need it in TLS, too?

Comment 8

[Martin Thomson [:mt:]]

To clarify JC's question further, I would like to know whether you have any intent to follow up with an AEAD (or AEADs). That falls short of adding TLS support (though not by far), but it makes for a far more useful primitive than the MAC alone.

Comment 9

[Alexander Scheel]

@jcj said:

Could you give us a brief rundown of where you need to use CMAC - e.g., whether you need it in TLS, too?

@mt said:

To clarify JC's question further, I would like to know whether you have any intent to follow up with an AEAD (or AEADs). That falls short of adding TLS support (though not by far), but it makes for a far more useful primitive than the MAC alone.

This was kinda hinted at in the description, but I just want the CMAC construct for the SCP 03 KDF (used for smart card authentication). Dogtag PKI until very recently shipped an SCP 03 implementation in Java. For internal compliance reasons, we'd love to move this from a Dogtag/Java implementation into a NSS implementation, wrapping it in JSS for our use. Thus no, I don't currently have plans to add this either as an AEAD cipher option or to expose it in TLS.

If adding e.g., CCM mode off of this would make this more likely to be adopted upstream, I'm happy to add such, but would prefer it be a separate bug as I don't have that ready and won't have time to work on it for a couple of weeks (I'm moving :).

I'll reply to the review comments, thanks all!

Comment 10

[Alexander Scheel]

Sorry, KDF is the wrong word. Its actually used "as a weird MAC thing".

Section 6.2.4 "APDU Command C-MAC Generation and Verification" if you happen to have a copy of the spec. You essentially share 8 bytes of the CMAC output as the MAC over the command you send. Each command is prefixed by the 16 bytes of full CMAC output from the previous command (initially: 0^128), but only for the purposes of computing the CMAC; it is removed before sending. So you're doing a form of chaining mode for MACs over commands / responses, leaking half of the IV that's used in the next message at a time (with the other 8 bytes kept secret).

Comment 11

[Alexander Scheel]

Attachment: Bug 1570501 - Add AES-CMAC implementation to freebl

Comment 12

[Alexander Scheel]

Ok, I'm not sure what happened with my moz-phab submit. If someone knows how to close https://phabricator.services.mozilla.com/D40853, I'd appreciate it. The older version (https://phabricator.services.mozilla.com/D40120) has been updated with the diff from D40853 so comments are preserved.

Comment 13

[Alexander Scheel]

@mt -- gentle poke. Any chance I could get another look at my patches and a decision on the question I asked? Thanks!

Comment 14

[Martin Thomson [:mt:]]

Sorry, it's been quite busy. This tends to drop to the bottom of the queue. I need to find some time to think about the tests some more. I've reviewed the PKCS#11 patches, which look generally good, but I need some more time on the bigger one.

Comment 15

[Alexander Scheel]

Hey, thanks Martin!

Busy here as well. I've finally gotten around to figuring out how to update my patches again. git is too ingrained in my workflows... :)

In addition to my (previous) question on D40120, I've asked two more on D40121.

First is in reply to your comment "The other test you should run is..." -- it wasn't clear to me what you were expecting there.

The other was about "Maybe the right answer here is to build the switch statement...." -- there I thought it wasn't the right answer and provided reasoning why, but if you still think it is I'll do it.

I didn't see any more feedback on D40120 but if I missed something, do let me know.

Comment 16

[Alexander Scheel]

Ok, thanks Martin. I've addressed all feedback and realized clang wasn't happy so I've updated both patches to fix that.

I think they're in better shape. One quirk of the ASSERT_* and EXPECT_* macros is that they apparently return, which means they don't work well inside of ImportKey and work better in void functions. Perhaps there's a variant of them that I'm missing. I used plain assert instead.

Comment 17

[Martin Thomson [:mt:]]

EXPECT_* just adds a failure to the list of failures that gtest maintains. ASSERT_* does the same with an extra return;, so you can use EXPECT_* anywhere, but ASSERT_* only in a void function.

Does that help?

Comment 18

[Alexander Scheel]

Sure :) Do you want to fix this in post-processing or do you want me to revise it?

Comment 19

[Martin Thomson [:mt:]]

Attachment: Bug 1570501 - Postprocessing

This does some cleanup for landing

clang-format (lots of changes here)

fixes a CI bug with modular builds. Why we have that hack in there is something
that bothers me, but I can't justify the time it would take to find out.

removes an unused 'wrap' function

removes the assert() use in favour of an early return if we can't get a slot

renames a bunch of test bits to fit with convention

makes the constants in tests 'static const'

adds a static assertion that the test arrays are the right size

Comment 20

[Martin Thomson [:mt:]]

I've made a few changes and put those up for your approval. Nothing major, just some dancing around to get code formatted consistently and that sort of thing.

Comment 21

[Alexander Scheel]

ACK patch looks good.

Weird hack; how would I have executed this to catch it? I typically ran something like:

rm out ../dist output.txt -rf
./build.sh -g --enable-fips
bash tests/gtests/gtests.sh > output.txt 2>&1 
[ -e output.txt ] && vi output.txt

But that didn't catch it in a way I could find.

Also, is there a way for me to submit to phab while run linting and unit coverage (or, can I run linting offline prior to submission)? I guess your phab submission didn't either:

Lint: No Linters Available	
Unit: No Unit Test Coverage

Thanks again for your review!

Comment 22

[Martin Thomson [:mt:]]

The hack arises from the way we test modular builds on our CI system. I don't know all the details of that build, I just cargo-culted a fix.

The phabricator integration for linting and testing isn't used here. We're still building out our infrastructure based on the stuff gecko uses. I don't think we will ever use the test setup and the linters seem to run once the review is uploaded. We manually trigger tests using our try servers.

Comment 23

[Alexander Scheel]

Thanks for taking the time to reply and review! :) Makes sense.

Comment 24

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/a42c6882ba1b5df9f02de19ca25163a817fac864
https://hg.mozilla.org/projects/nss/rev/cf0df88aa807fe80a4b319f50bedc014fb7fdcb0