Re: [secdir] secdir review of draft-ietf-lamps-cms-sha3-hash

[Russ Housley <[e-mail]>]

Wes:

Thanks for the careful review.

> I have reviewed this document as part of the security directorate's
> ongoing effort to review all IETF documents being processed by the
> IESG. These comments were written primarily for the benefit of the
> security area directors. Document editors and WG chairs should treat
> these comments just like any other last call comments.
> 
> The summary of the review is: ready
> 
> In general this is a well written document.  I have some minor
> thoughts/nits below.  Note that I'm not a CMS expert, but certainly have
> a decent amount of generic background (including ASN.1).
> 
> - Section 2: The first paragraph has an odd line break after the
>  first sentence.  I'm not sure what formatting has caused this, but it
>  is present in both the txt and pdf versions at least.

I figured it out.  Corrected in my edit buffer.

> - Section 2 paragraph 2: it would be helpful to less familiar readers to
>  say what the fields were inside of what CMS structure.

Paragraphs 2 and 3 are talking about the SignedData and DigestedData CMS structures.  It is listing the fields in various CMS structures that contain digest algorithm identifiers.

> - section 2, generally: by specifying exactly the list of fields where
>  this applies, it does limit future extensions to the base protocol to
>  not add new fields without updating this document as well.  This may
>  be by design though.

The idea was to say where an implementer might need to look to add support for the new message digest algorithm.  If a new CMS structure is added in the future, I would not expect this list to be updated.

> - section 3.1 and beyond: there is a discrepancy between how OIDs are
>  used in some places and the fully qualified OBJECT IDENTIFIERs are
>  used in other places.  I note the appendix uses OID everywhere, but
>  the copied versions into the sections above.

The OID convention was used only is Section 3.1 because the names that were already assigned in RFC 8017 are so long.  If you find this confusing, the impact will be two lines for each of them.

> - Why is sigAlgs and hashAlgs defined in this module but RSAPublicKey is
>  imported.  Even though sigAlgs and hashAlgs are simply references,
>  wouldn't it be better to import them from somewhere else since I don't
>  think you're actually defining them.  (my memory says you can import
>  simple OIDs from other places).  My purist had would rather import
>  them rather that duplicate the definition, but this is a choice to
>  some extent (and yes, there is no actual harm in defining it in
>  multiple places or even having two different names assigned to the
>  same OID but then you get into proper coding practice and typos and
>  fun.)

In RFC 5912, the author chose to no define these in a way that can be imported.  For example:

   id-sha512  OBJECT IDENTIFIER  ::=
       { joint-iso-itu-t(2) country(16) us(840) organization(1) gov(101)
       csor(3) nistAlgorithms(4) hashalgs(2) 3 }

So, another RFC would have to be referenced to properly IMPORT these values.  This way, all IMPORTed types come from RFC 5912.

> - section 5.2: suggest changing "If any other value is used for S" to
>  "If any value is used for S", since you just said it must be absent if
>  there was no label.  And unless "absent" is a value, then the word
>  "other" doesn't apply.

Based on an earlier comment, this was changed to:

   When the id-kmac128 or id-kmac256 is used as part of an algorithm
   identifier, the parameters field MUST be absent when there is no
   customization label S.  If any value is provided for S, then the
   parameters field MUST be present and contain the value of S, encoded
   as Customization.

      Customization ::= OCTET STRING

> - section 5.2: it would be kinda helpful in the paragraph to also state
>  that S must be encoded as an OCTET STRING even though it's in the
>  definition too.

I think the reworded paragraph makes that clear.

> - section 5.3: you state "an algorithm identifier from section 2 is
>  carried in the parameter" but don't state what parameter.  Since this
>  is sentences later, I'd suggest adding "KEMRecipientInfo" in front of
>  parameter (and if I got the parameter wrong, then you already have one
>  failed interpretation!)

I suggest:

   The key-derivation function algorithm identifier is an object
   identifier and optional parameters.  When KDF2 and KDF3 are used,
   they are identified by the id-kdf-kdf2 and id-kdf-kdf3 object
   identifiers, respectively.  The key-derivation function algorithm
   identifier parameters carry a message digest algorithm identifier,
   which indicates the hash function that is being employed.  To support
   SHA3, the key-derivation function algorithm identifier parameters
   contain an algorithm identifier from Section 2.

> - section 6: suggest "...authentication is not provided" ->
>  "...authentication is not assured".

Okay.  That works for me.

> - section 6, sentence starting with "An attacker may find"...  The first
>  time I read this I had to read it multiple times to pull it all in and
>  marked it as "awkward".  Now I seem to understand it better, but I'll
>  flag it as "could use some simplification and probably broken in 2" if
>  you want to do it to help past-me.

How about this rewording:

   ...  Instead of brute force searching the whole
   key space, an attacker may find it much easier to reproduce the PRNG
   environment that produced the keys, and then search the resulting
   small set of possibilities.  ...

> - I find it odd that the appendix doesn't have a letter and only a
>  title.  When there is only one, I suppose this makes sense but I
>  vaguely normally remember seeing "Appendix A" even when there was only
>  a single appendix.  maybe.

I will leave this one to the RFC Editor.

Russ