Test the AES-CCM test vectors from the NIST Known Answer Tests.
ClosedPublic
Actions

Authored by jhb on Apr 19 2019, 10:16 PM.

Details

Reviewers

cem
ngie
jmg

Commits

rS346617: Test the AES-CCM test vectors from the NIST Known Answer Tests.

Summary

The CCM test vectors use a slightly different file format in that
there are global key-value pairs as well as section key-value
pairs that need to be used in each test. In addition, the sections
can set multiple key-value pairs in the section name. The CCM KAT
parser class is an iterator that returns a dictionary once per test
where the dictionary contains all of the relevant key-value pairs
for a given test (global, section name, section, test-specific).

Note that all of the CCM decrypt tests use nonce and tag lengths
that are not supported by OCF (OCF only supports a 12 byte nonce
and 16 byte tag), so none of the decryption vectors are actually
tested.

Test Plan

ran against ccr0, cryptosoft0, and aesni0. No failures found. Did confirm via stats on ccr0 that it did do ccm encrypt operations, but no decrypts (this is what led to figuring out why none of the decryption vectors were run).

Diff Detail

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 23779
Build 22726: arc lint + arc unit

Event Timeline

jhb created this revision.Apr 19 2019, 10:16 PM

Harbormaster completed remote builds in B23779: Diff 56412.Apr 19 2019, 10:16 PM

jhb added parent revisions: D19977: Use more descriptive algorithm names in skip messages., D19853: [security/nist-kat]: Add AES-CCM and plain SHA digest test vectors..Apr 19 2019, 10:17 PM

Hm, why isn't maclen of 16 an issue for encrypt?

And can we pad tags with zeros out to 16 bytes to verify decrypt, or does the taglen get incorporated into the MAC (I forget)? (Edit: nevermind, I guess even if the tag is truncated we have no way to tell OCF to only verify so-many bytes of it, and the zeros won't match. Ugh.)

I'd encourage adding the note (that none of the NIST KAT decryption vectors are supported by OCF CCM, so runCCMDecrypt is a big no-op for now) to the code itself as well.

Requesting changes because there are a handful of non-style nits that should be resolved before this is committed to ^/head .

tests/sys/opencrypto/cryptodev.py
390	A general comment: it's better to allocate this later on to avoid leaks and make cleanup at `__del__(..)` time unnecessary (that's what you'll have to do in this case).
430	style nits: Use `line.startswith("#")` instead of line[0] == "#". Also, if you strip line, then you can condense the `if not line` and `if not line.strip()` into a single `if not line` check, e.g., line = self.fp.readline() if line.startswith("#"): continue line = line.strip() # Will catch a blank line or a line that consists only of space characters. if not line: continue
437	This could cause problems if the line has more than one `=` in the string. You might want to use `f, v = line.split(" =", 1)` to avoid the unnecessary exception.
438	Bare `except`s are generally an anti-pattern because it will catch other exceptions, like `sys.exit`, `KeyboardInterrupt`, etc. You probably should just try capturing `ValueError`, instead as that's the error that you likely care about the most: >>> f, v = "bar".split(" =") Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: not enough values to unpack (expected 2, got 1)
462	Same comment as above.
465–468	A regular expression that was pre-compiled and used for grabbing matches would likely be better, but this is potentially more performant in the right circumstances. The only thing I'd do is use `line.startswith("[")` and be a bit more defensive with the split, as it will give you false positives, as seen below: >>> "bar".split("]", 1)[0] 'bar'
tests/sys/opencrypto/cryptotest.py
260–262	It's better to format this string like so (it won't trace back if data["Count"] cannot be concatenated to "" and is a bit more legible/greppable): self.assertEqual(out, ct, "Count: %s. Actual: %r. Expected: %r on cname" % ( data["Count"], out.encode("hex"), data, cname ) )
291	Please use `EnvironmentError as e` for py3 compat <3.
297–298	This might be a bit easier to follow (and plus, you can test the exception properties afterwards): with self.assertRaises(IOError): c.decrypt(payload, nonce, aad, tag)
306–310	Same comment about `self.assertEqual` above.

This revision now requires changes to proceed.Apr 21 2019, 9:55 PM

In general, many of the style issues are present in the rest of the file. I think for those sorts of things it is better to fix all of them at once (e.g. in followup changes) rather than having the CCM tests be very different from the other tests.

tests/sys/opencrypto/cryptodev.py
390	I was just matching the existing code KATParser class in that regard.
430	The proposed version misses the test for EOF. Also, the existing changes match the existing style in KATParser (for better or worse).
437	The file format is that you have lines with a single 'key = value' pair. A line not match that should be treated as corrupt rather than trying to handle multiple pairs, etc.
438	That may very well be, this was also copied from the KATParser.
tests/sys/opencrypto/cryptotest.py
260–262	This is the current pattern used throughout the rest of this file. I do think that using a format string instead of concat probably is better though and would be fine doing a sweep to fix all of them in a followup after you reindent (since the indentation also makes these lines nearly unreadable).
297–298	I did look at using that, but chose to match the existing use of self.assertRaises in the GCM tests.

jhb added a parent revision: D20034: Skip tests with missing test vectors instead of failing..Apr 23 2019, 10:20 PM

Ok, let's proceed with this change. I'll work on fixing the style issues on my branch.

This revision is now accepted and ready to land.Apr 23 2019, 10:24 PM

ngie added inline comments.Apr 23 2019, 10:25 PM

tests/sys/opencrypto/cryptodev.py
438	Ugnnnnnnnnhhhhhh....

In D19978#429425, @cem wrote:

Hm, why isn't maclen of 16 an issue for encrypt?

For encrypt we just do the truncation in the python code of the 16 byte result from OCF.

And can we pad tags with zeros out to 16 bytes to verify decrypt, or does the taglen get incorporated into the MAC (I forget)? (Edit: nevermind, I guess even if the tag is truncated we have no way to tell OCF to only verify so-many bytes of it, and the zeros won't match. Ugh.)

Correct, since the verification of the tag on decrypt happens in the kernel, we need OCF itself to understand the tag length. In theory the 'mlen' field in the crypto session should let us do this, but I think the current /dev/crypto CIOGSESSION ioctls don't expose that, and the current GCM and CCM software bits in the kernel don't honor/support truncated tags.

I'd encourage adding the note (that none of the NIST KAT decryption vectors are supported by OCF CCM, so runCCMDecrypt is a big no-op for now) to the code itself as well.

Will do in the commit.

Closed by commit rS346617: Test the AES-CCM test vectors from the NIST Known Answer Tests. (authored by jhb). · Explain WhyApr 24 2019, 12:23 AM

This revision was automatically updated to reflect the committed changes.

Herald added a reviewer: jmg. · View Herald TranscriptApr 24 2019, 12:23 AM

Herald added a subscriber: imp. · View Herald Transcript