Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMAC to JSS #259

Merged
merged 2 commits into from
Sep 30, 2019
Merged

Add CMAC to JSS #259

merged 2 commits into from
Sep 30, 2019

Conversation

cipherboy
Copy link
Member

@cipherboy cipherboy commented Sep 17, 2019

This is WIP as it depends on #258 being merged. This series will be rebased afterwards. The commits will be squashed and rewritten.

This adds initial support for CMAC to the JSS Provider interface. This depends on a recent enough NSS that includes CMAC support in PKCS#11. Thus CI will fail until I introduce a new instance with Sandboxed NSS builds.

I've added build time support for CMAC. What this means is that JSS only needs a rebuild in order to take advantage of CMAC support in NSS. Otherwise, it'll ignore the non-existence of CMAC and not run the applicable test from the test suite. CMAC will still be defined in the JSS provider though.

You can see this via checking the output of CI: currently there are 66 tests without CMAC, 1 CMAC test. So only fedora_sandbox CI image will have 67 test cases.

@cipherboy cipherboy added enhancement New feature or request primitives Changes which affect cryptographic or NSS primitives; low-level work labels Sep 17, 2019
@cipherboy cipherboy added this to the 4.6.2 milestone Sep 17, 2019
@cipherboy cipherboy requested a review from edewata September 17, 2019 23:18
@cipherboy cipherboy self-assigned this Sep 17, 2019
@cipherboy cipherboy removed the request for review from edewata September 17, 2019 23:21
@cipherboy cipherboy force-pushed the add-cmac branch 2 times, most recently from b225726 to d46c7d9 Compare September 18, 2019 20:24
@cipherboy cipherboy force-pushed the add-cmac branch 2 times, most recently from 924eaa6 to da9b2a9 Compare September 25, 2019 16:29
@cipherboy
Copy link
Member Author

@edewata Do you think it is worthwhile to detect whether or not CMAC exists as a constant and not run the test at all, instead of always running the test and checking if it exists inside the test?

The former would allow someone to check the build logs / test stdout and see if it has CMAC support. The latter would require someone to check the output of the tests (which don't get shown on non-error and are written to build/Testing/Temporary/LastTest.log).

@cipherboy
Copy link
Member Author

cipherboy commented Sep 25, 2019

@edewata Never mind, I've decided to go with the build-time detection. I think it'll make life easier for all of us. This is ready for review, pending passing CI.

@cipherboy
Copy link
Member Author

Ok, this works now:

fedora_30:

100% tests passed, 0 tests failed out of 66

fedora_sandbox:

100% tests passed, 0 tests failed out of 67

@@ -21,9 +23,9 @@
class JSSMacSpi extends javax.crypto.MacSpi {

private JSSMessageDigest digest=null;
private HMACAlgorithm alg;
private DigestAlgorithm alg;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edewata This isn't quite right I now realize (I thought it wasn't, then though it was, now realize it isn't).

DigestAlgorithm defines things like SHA-1, SHA-2, &c.

HMACAlgorithm is a subclass which defines things like HMAC/SHA-1, HMAC-SHA-2-256, &c. These all require keys though, unlike digests (though, some digests are keyed, but none we define are).

CMAC (the algorithm) isn't actually based off of a digest (hash algorithm), but is instead based off a (keyed) cipher.

So technically, we should have a class Structure like:

Algorithm
  DigestAlgorithm
  MACAlgorithm
    CMACAlgorithm
    HMACAlgorithm

But currently we have the following class structure:

Algorithm
  DigestAlgorithm
    CMACAlgorithm
    HMACAlgorithm

Lastly, these are protected so end-users won't directly be constructing them. I see the following actions we could take:

  1. Be happy how it is.
  2. Add an explicit check for HMAC or CMAC subclass.
  3. (A breaking change for anyone using HMACAlgorithm) Fix the class structure.
  4. Make CMAC inherit from Algorithm or a new MACAlgorithm, not DigestAlgorithm, checking either for HMACAlgorithm or MACAlgorithm.

Note that any checking that we would do here is limited to developers adding new HMAC/CMAC/*MAC implementations and making their lives easier.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(UMAC is the only other MAC implementation I could see someone wanting to add, but NSS doesn't yet support it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with #3 for a new JSS minor version (4.7?). If we want to keep the current minor version we can use any other options to provide the new functionality without introducing incompatible changes, but consider that as a temporary solution until we fix the structure. As long as we document the changes clearly it should be OK to require people to change their code when upgrading JSS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we add that as a second patchset after this merges, since it involves changing both HMAC and CMAC, and we can do additional simplifications internally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks!

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good in general. There are some minor comments. I'm deferring to @jmagne to review from crypto perspective.

CMAC is a form of MAC that utilizes a block cipher instead of a hash
function. Support for CMAC via PKCS#11 was recently introduced to NSS
allowing us to add support for it here.

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1570501

Signed-off-by: Alexander Scheel <[email protected]>
These tests are from NIST's Examples with Intermediate Values page:
https://csrc.nist.gov/projects/cryptographic-standards-and-guidelines/example-values

Signed-off-by: Alexander Scheel <[email protected]>

return null;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

In TPS/TKS we start with a symmetric key such as the master key and then send that down into the routine that calculates the CMAC. The key will be on the hsm possibly as a SymmetricKey object variable. How do we use this from JSS from that starting point? The test examples start from byte array keys, which is what you expect in the HMAC stuff.

Would it be similar to what I found in JSS here:

// perform the digesting
JSSMessageDigest digest = token.getDigestContext(HMACAlgorithm.SHA1);
digest.initHMAC(key);
byte[] digestBytes = digest.digest(toBeMACed);
Or is there a way to use the nice provider support we have here starting with a sym key object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's two interfaces and a class in question here... and a pointer to the PKI code might be nice. But I'll assume the following is correct. :)

Classes/Interfaces in question:

Everything that JSS uses has to be an instance of SecretKeyFacade. Internally, the key member (of interface SymmetricKey) is usually of type org.mozilla.jss.pkcs11.PK11SymKey. The latter includes additional information like algorithm, and a NSS PK11SymKey pointer. So when you say you have a SymmetricKey instance, you usually actually have a PK11SymKey instance. But anyways...

To go from SymmetricKey to something usable by the JSS Provider (and, not just the underlying crypto), your easiest option is to do something like:

javax.crypto.SecretKey key = new org.mozilla.jss.crypto.SecretKeyFacade(mySymmetricKey);

That's really what JSSSecretKeyFactorySpi does under the covers.

Note that JSSSecretKeyFactorySpi implements the javax.crypto.SecretKeyFactorySpi SPI class, so it can eventually be called from the Java Provider javax.crypto.SecretKeyFactory member (when the provider is Mozilla-JSS).

That brings us full-circle: if you have bytes, you can use the SecretKeyFactory with a correct spec class to get a SecretKey instance to use, otherwise, if you already have a PKCS#11-backed key from JSS, you can just use SecretKeyFacade to wrap it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #271. I think we can support just passing SymmetricKey directly here and be better off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the info I wanted! Just double checking I can do what we need :)

/* 69 */ {CKM_SHA512_HMAC, PK11_MECH},

/* CKM_AES_CMAC is new to NSS; some implementations might not yet have it. */
/* 70 */ {CKM_AES_CMAC, PK11_MECH}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question:
I assume by adding this mechanism , which nss now implements, is enough to make existing jni code in jss do this job for us? Just curious to take a look at what's going on down there. Is PK11MessageDigest.c where the magic is happening?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! That's some lovely magic.

So my understanding of PKCS#11 is a bit... rough, and Bob is definitely the one to ask these questions to. :)

But there's a few basic operations in PKCS#11: Sign, Verify, Encrypt, Decrypt, etc. Each of these correspond to a top-level function call in NSS: PK11_Sign, PK11_Verify, etc. So, once you've set up the context, you essentially get away with calling one of those methods, and then you'll have the result. But anyways... let's start at the top.

You create a new javax.crypto.Mac instance of type CmacAES (or one of its aliases). This gets backed by a JSSMacSpi instance of subclass CmacAES. All this does is initialize the JSSMacSpi instance with the CMACAlgorithm.AES algorithm. This is an instance of org.mozilla.jss.crypto.CMACAlgorithm with the correct PKCS#11 constant (er, proxies). [*]

Anyhow, so when you call Mac.init, it calls into PK11MessageDigest.initHMAC(...), which creates the PKCS#11 mechanism, makes sure the key is enabled for signing, and then creates the PKCS#11 context.

Each call to Mac.update then calls PK11_DigestOp, and in the end PK11_DigestFinish is called. And to finish; PK11_DigestOp/PK11_DigestFinal is merely a nice wrapper around all of those different PKCS#11 operations.

[*]: This is where I disagree with the construction of org.mozilla.jss.crypto.Algorithm: rather than handling PKCS#11-backed and SecOID-backed algorithms separately, they merge it into one big table of tuples (value, type). Then, they define pretty values like Algorithm.CKM_AES_CMAC = 70, which is an index and not the actual value. They could've instead made all PKCS#11-backed algorithms use, well, the underlying PKCS11Constants values and not made a redundant mapping (that class was added by me since its counterpart in the JDK was removed in JDK9, but that's well before our time)... But that's neither here nor there, and we have org.mozilla.jss.crypto.PKCS11Algorithm to map between them for us.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent info. I will approve since it looked good to me, just wanted a couple of clarifications. I just wanted to make sure I missed some other code that calls down into your new nss cmac code. Thanks!

Copy link
Contributor

@jmagne jmagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and apparently I can use it from TMS, so approving from the crytpo perspective as endi requested.

@cipherboy cipherboy merged commit bf65e0f into dogtagpki:master Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request primitives Changes which affect cryptographic or NSS primitives; low-level work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants