Re: [pkcs11] Groups - Message Based processsing uploaded

[Wan-Teh Chang <wtc@google.com>]

On Wed, Apr 16, 2014 at 1:29 PM, Michael StJohns <msj@nthpermutation.com> wrote:
> Submitter's message
> Got tired of trying to tweak what Wan Teh was doing so I wrote the the whole
> thing from scratch. This benefits from a number of ideas I got from Wan
> Teh's approach and from discussions during the calls. I believe its close to
> 95% - will need a review pass and a pass to complete discussion of error
> codes.
>
> This adds three API calls, and 4 IV generators and should cover the bulk of
> AEAD and existing legacy uses.

Hi Mike,

I reviewed your proposal v1 dated 16 Apr 2014. I ordered my review
comments from high level to low level. Most people can skip the
editorial comments and typo fixes at the end.

Overall comments:

This is a well-thought-out proposal based on good research.

In this proposal the API evolved into a universal C_ProcessMessage
function that can perform any phase of any cryptographic operation.
Our two proposals made diametrically opposite design choices in this
aspect. I suspect the best way forward is for the technical committee
to select one proposal rather than trying to merge them.

API design comments:

1. In the description of C_ProcessMessage, we have:

    Note: while it is unlikely that an IV generator will be used on
the received side for to-be-defined mechanisms, the API does not
prohibit this.

We discussed this issue in email before but I forgot to follow up. I
think the decrypter doesn't need to use an IV generator. The scenario
you described in that email thread seems to be IV recovery rather than
IV generation.

2. In the "Initialization Vector Generators" section, the paragraph
that begins with "Some IV generators MAY take an input value": it
seems that if an IV generator needs to take an input from outside the
crypto token, then that defeats the purpose of having the crypto token
generate the IVs because the IV generation process cannot be fully
validated by a FIPS testing lab. The IV generator will need to
validate the external inputs, for example, verifying that the sequence
numbers are monotonically increasing.

3. The "CK_MECHANISM_TYPE randomMechanism" field of
CK_IVGEN_RANDOM_PARAMS and CK_IVGEN_PSEUDORAND_PARAMS seems
unnecessary because the C_GenerateRandom function doesn't take a
mechanism input.

Editorial comments:

1. In the description of C_ProcessMessage, we have:

    For output of signatures (i.e., when bReceiver is CK_FALSE), the
processing of pulIntegrityTagLen follows the conventions of section
11.2

We should also describe, for input of signatures (i.e., when bReceiver
is CK_TRUE), how pulIntegrityTagLen should be processed.

2. In the CKM_IVGEN_CCM_COUNTER section: "Note that if pNonce is not
null, then the rightmost ulFixedFieldLength bytes are the initial
value of that counter": this sentence should ideally be combined with
"pNonce points to the location that contains the fixed part of the
nonce or is NULL_PTR", so that we say something like:

    pNonce points to the location that contains the fixed part of the
nonce and the initial value of the counter, or is NULL_PTR.

Typos and nits:

1. "That handle for that object": perhaps "The handle for that object"
or just "That handle"?

2. "the HSM MAY relax that restriction to permit one encrypt/decrypt
and one hash/sign/verify per association rather than per session": it
seems that "one encrypt/decrypt and one hash/sign/verify" should be
"one encrypt/decrypt or one hash/sign/verify" because the |pMechanism|
input of C_NewAssociation can only specify one encrypt/decrypt or one
hash/sign/verify, but not both.

3. CK_BOOLEAN => CK_BBOOL. CK_BBOOL is the boolean type in PKCS #11.

4. All occurrences of "CK_NULL" and "NULL_PTR" should be changed to
"CK_NULL_PTR".

5. In the function prototype of C_ProcessMessage, the use of "Len" and
"Length" in function parameter names is inconsistent. In the text,
"ulAssociatedDataLength" should be changed to "ulAssociatedDataLen" to
match the name used in the function prototype.

6. All occurrences of "uPhase" should be changed to "ulPhase".

6. "The API specified in the mechanism parameters of the cryptographic
mechanism is ignored": change "API" to "IV".

7. "every initial call to C_ProcessMessage (i.e. where uPhase is
either one of the PHASE_SINGLE values or is PHASE_INITIAL provides an
IV for that specific message": add a closing parenthesis ')' after
"PHASE_INITIAL".

8. "I.e. if the length of the variable part is 4, then the value of
the variable part will be ...": "I.e." probably should be changed to
"E.g."

Wan-Teh Chang