- Download Diff

Summary:

5869 Need AES CMAC support in KCF+PKCS11

Review Request #39 — Created April 24, 2015 and discarded April 24, 2015, 10:21 p.m.

Information
Owner:	gwr
Repository:	illumos-gate
Branch:
Bugs:	5869
Depends On:
Reviewers
Groups:	general
People:

Description

5869 Need AES CMAC support in KCF+PKCS11
It turns out Matt could not easily update this one, so I had him open his own RB.
Please see: https://www.illumos.org/rb/r/40/

Sorry if I've caused you reviewers to have to add your questions and comments again.

Testing Done

We have a test framework for these changes, and we'll have that out for review too before this integrates into illumos.
However, we'd really like review comments on the KCF and PKCS11 changes now.

Issues

11
0
2
2
All Issues: 15

Description	From	Last Updated
A comment here with this new line would be nice, and is this inserted mid-enum because of PKCS#11?	danmcd	April 24, 2015, 7:50 p.m.
Is changing the value of any following item in this enumeration really safe and compatible?	richlowe	April 30, 2015, 4:04 a.m.
COMMENT!	danmcd	April 24, 2015, 7:50 p.m.
What is this constant?	gdamore	April 27, 2015, 7:20 p.m.
I'm guessing the value of this is defined by the algorithm. Is there a reference or something, or a better ...	richlowe	April 30, 2015, 4:04 a.m.
I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I ...	gdamore	April 27, 2015, 7:20 p.m.
Just make a choice.	gdamore	April 27, 2015, 7:20 p.m.
Presumably this is somethingy ou should resolve before you want to integrate	richlowe	April 30, 2015, 4:05 a.m.
Again, something to resolve before you want to integrate	richlowe	April 30, 2015, 4:05 a.m.
Is it really safe to not use the provided copy_block and such here? I feel like I must be missing ...	richlowe	April 24, 2015, 5:31 p.m.
Prefer _NOTE() in the function body to ARGSUSED wild card. (As in _NOTE(ARGUNUSED(what)).	gdamore	April 27, 2015, 7:20 p.m.
Same comment regarding copy_block. It looks like you're perhaps just making these functions not copy at all, in which case ...	richlowe	April 24, 2015, 5:31 p.m.
Comment doesn't really make it clear how this differs from cbc_remainder_len	richlowe	April 24, 2015, 5:31 p.m.
I think without the trailing slash is better.	gdamore	April 27, 2015, 7:20 p.m.
Perhaps this bit can be moved into the two cases above, with a fallthrough?	gdamore	April 27, 2015, 7:20 p.m.

Bugs:

+	5869

Fix it!

usr/src/common/crypto/aes/aes_impl.h (Diff revision 1)

Show all issues

Is changing the value of any following item in this enumeration really safe and compatible?

mbarden April 27, 2015, 8:39 p.m.

(I'll just answer these here, I suppose)

I moved it to the end just to be safe.

usr/src/common/crypto/modes/cbc.c (Diff revision 1)

Show all issues

I'm guessing the value of this is defined by the algorithm. Is there a reference or something, or a better name, or is this a name that would be well known to anyone familiar with AES-CMAC?

gwr April 24, 2015, 6:08 p.m.

I think this is from the spec. (will let Matt B. confirm).

Yes, a spec. reference might be nice...

mbarden April 27, 2015, 8:39 p.m.

It's defined in RFC 4493 ("Constants: const_Zero is 0x00000000000000000000000000000000\nconst_Rb is 0x00000000000000000000000000000087")

usr/src/common/crypto/modes/cbc.c (Diff revision 1)

Show all issues

Presumably this is somethingy ou should resolve before you want to integrate

mbarden April 27, 2015, 8:39 p.m.

Already resolved; I believe we came to the conclusion that It could technically be used for DES, etc, even if it isn't currently, so we may as well pass them in.

usr/src/common/crypto/modes/cbc.c (Diff revision 1)
Show all issues
```
Again, something to resolve before you want to integrate
```
1. mbarden April 27, 2015, 8:39 p.m.
  We decided to keep the bzero

usr/src/common/crypto/modes/ccm.c (Diff revision 1)

Show all issues

Is it really safe to not use the provided copy_block and such here? I feel like I must be missing something (but it's probably that I'm not familiar with the design of the framework).

usr/src/common/crypto/modes/gcm.c (Diff revision 1)

Show all issues

Same comment regarding copy_block.

It looks like you're perhaps just making these functions not copy at all, in which case why take the parameters?

usr/src/common/crypto/modes/modes.h (Diff revision 1)

Show all issues

Comment doesn't really make it clear how this differs from cbc_remainder_len

mbarden April 27, 2015, 8:39 p.m.

cbc_remainder_len contains the number of bytes currently stored in cbc_remainder, while max_remain is the upper limit for these bytes. (for CBC it's 15, and for CMAC it's 16, as we want to store a full unencrypted block for cmac_final.)

Description:

		5869 Need AES CMAC support in KCF+PKCS11

		The SMB3 support we're working on requires AES/CMAC from KCF and PKCS11.
		Changes to crypto code seem to scare everyone, so we're putting this out for community review.
	+
	+	BTW, this work by Matt Barden. He'll be answering review comments & questions.

Fix it!

Overall design question: Why is CMAC a special case of CBC, instead of a special case of CCM (where you don't encrypt and just take the residue)?

mbarden April 27, 2015, 8:39 p.m.

Just looking at CCM, it looks to be much more invasive to add CMAC as a special case; To me it seems more involved than "do CBC in place, then something different at the end". More along the lines of "surgically isolate the CBC-MAC step - with the changes I made to CBC - so that it can be run alone, making sure not to break other things in the process".

Requested comments had already been added :)

usr/src/common/crypto/aes/aes_impl.h (Diff revision 1)

Show all issues

A comment here with this new line would be nice, and is this inserted mid-enum because of PKCS#11?

usr/src/common/crypto/modes/cbc.c (Diff revision 1)
Show all issues
```
COMMENT!
```

Description:

~		5869 Need AES CMAC support in KCF+PKCS11
	~	5869 Need AES CMAC support in KCF+PKCS11
	+	It turns out Matt could not easily update this one, so I had him open his own RB.
	+	Please see: https://www.illumos.org/rb/r/40/

~		The SMB3 support we're working on requires AES/CMAC from KCF and PKCS11.
	~	Sorry if I've caused you reviewers to have to add your questions and comments again.
-		Changes to crypto code seem to scare everyone, so we're putting this out for community review.
-
-		BTW, this work by Matt Barden. He'll be answering review comments & questions.

Status: Discarded

Fix it!

Generally looks pretty good, but I've not verified against PKCS#11 nor have I gone through the algorithm with a super-fine toothed comb.

As you're modifying existing CBC modes, I think its really very important to have a complete test suite here, that provides known answer tests and covers the various edge cases. I think you need to supply that for both the new CMAC mode plus the other AES modes that you've affected.

mbarden April 27, 2015, 8:39 p.m.
```
That's the goal of the test suite
```

usr/src/common/crypto/modes/cbc.c (Diff revision 1)
Show all issues
```
What is this constant?
```
1. mbarden April 27, 2015, 8:39 p.m.
  part of the CMAC spec RFC 4493

usr/src/common/crypto/modes/cbc.c (Diff revision 1)

Show all issues

I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I think copy-paste-modify may actually be superior in this cae than inline modification, even though a lot of code is shared.

usr/src/common/crypto/modes/cbc.c (Diff revision 1)
Show all issues
```
Just make a choice.
```

usr/src/common/crypto/modes/ccm.c (Diff revision 1)

Show all issues

Prefer _NOTE() in the function body to ARGSUSED wild card.  (As in _NOTE(ARGUNUSED(what)).

usr/src/lib/pkcs11/pkcs11_kernel/Makefile.com (Diff revision 1)
Show all issues
```
I think without the trailing slash is better.
```
usr/src/lib/pkcs11/pkcs11_softtoken/common/softEncryptUtil.c (Diff revision 1)
Show all issues
```
Perhaps this bit can be moved into the two cases above, with a fallthrough?
```

You have a pending review.

Review Board 3.0.18

5869 Need AES CMAC support in KCF+PKCS11

Bugs:

Description:

Description:

Status: Discarded