Re: [Gen-art] Gen-ART Last Call review of draft-josefsson-scrypt-kdf-03

[Paul Kyzivat <[e-mail]>]

I have had no replies on this, and just want to double check that it has 
been received.

	Thanks,
	Paul

On 8/25/15 2:14 PM, Paul Kyzivat wrote:
> I am the assigned Gen-ART reviewer for this draft. For background on
> Gen-ART, please see the FAQ at <​
> http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
> Please resolve these comments along with any other comments you may
> receive.
>
> Summary:
>
> This draft is on the right track but has open issues, described in the
> review.
>
> Disclaimers:
>
> I am not schooled about crypto algorithms and techniques. I have
> reviewed this draft from the perspective of a software engineer, trying
> to understand if I could implement from this specification.
>
> I also know nothing about using ASN.1 to configure KDFs. So I can't
> comment on section 6. This draft ought to be reviewed by somebody
> capable of checking that.
>
> I have not attempted to verify the test vectors in sections 7-12.
>
> -------------
> Major issues:
>
> (Issue-1): Intended status
>
> The intended status of this document is Informational. I question why it
> is not a normative document. As best I can tell, this is the formal
> specification of the algorithm. Those that use it would presumably want
> to claim conformance to it. The introduction describes this as an
> alternative to other KDF functions, including only one with an RFC
> reference - RFC2898. That one is also informational, but it is a
> restatement of an algorithm specified elsewhere, so that RFC can be
> viewed as an informational supplement to the actual definition. The same
> is not true of this document.
>
> (Of course changing this to a normative document would require
> significant changes, including adding 2119 language. And it probably
> could then not be handled as an AD-sponsored document.)
>
> (Issue-2): Type mismatch on call to scryptROMix
>
> The scryptROMix function calls scryptBlockMix with an octet vector of
> length 128*r octets. But the definition of scryptBlockMix specifies that
> the input argument should be a vector of 2*r 64-octet blocks.
>
> Clearly these don't match. One way to make them match would be to divide
> the single 128*r octet vector into two 64-octet vectors, and then to
> treat r as 2 inside of scryptBlockMix. I don't know if that is the intent.
>
> (Issue-3): Definition of Integerify
>
> The Integerify function is not clearly defined. The following is given
> in the document:
>
>    j = Integerify (X) mod N
>        where Integerify (B[0] ... B[2 * r - 1]) is defined
>        as the result of interpreting B[2 * r - 1] as a
>        little-endian integer.
>
> I can make no sense of this definition of Integerify. The description
> implies that B must be an array containing elements up to index 2*r-1.
> But the definition of B is "Input octet vector of length 128 * r
> octets". Taking the definition literally, B[2*r-1] must be an octet, and
> 2*r must be less than 128. That seems like nonsense to me.
>
> I found the following in the [SCRYPT] paper:
>
> "We expect that for reasons of performance and simplicity,
> implementors will restrict N to being a power of 2, in which case
> the function Integerify can be replaced by reading the first (or
> last) machine-length word from a k-bit block."
>
> Simply reading a machine-length word ignores the differences between
> little-endian and big-endian machines, and machines with different word
> sizes. Conveniently, [SALSA20SPEC] defines a littleendian function that
> yields a 32-bit integer from four bytes. That should be sufficient bits
> for computing "j". So Integerify(X) could be defined as:
>
>     littleendian(X[0],X[1],X[2],X[3])
>
> or
>
>     littleendian(X[128*r-4],X[128*r-3],X[128*r-2],X[128*r-1])
>
> (I don't think it matters which, as long as everyone does it the same way.)
>
> In any case, the language is ambiguous and needs to be clarified.
>
> -------------
> Minor issues:
>
> (Issue-4): Identifiers reused for different meanings
>
> In scrypt, "B" is an array of "p" vectors, each of which is 128*r
> octets. In scryptROMix, "B" is a single vector of 128*r octets. In
> scryptBlockMix, "B" is a vector of 2*r 64-octet blocks.
>
> In both scrypt and scryptROMix "r" is the same block size parameter. But
> in scryptROMix it is only used in the (broken) definition of Integerify.
> In scryptBlockMix "r" is (apparently, if I have figured things out)
> always 2, and unrelated to the other "r".
>
> The document would be clearer if distinct identifiers were used for each
> unique concept.
>
> For those identifiers whose value is intended to be constant and common
> across all the functions (such as "N"), it would be better to define
> them once, globally.
>
> (Issue-5): Confusing/misleading names/definitions of identifiers
>
> The "Block size" parameter ("r") does not denote the size of a block. It
> is a factor in the size of blocks, varying from function to function.
> Exactly what concept it denotes, and how one would choose it, isn't
> clear to me.
>
> The definition of the "N" parameter (CPU/Memory cost parameter) isn't
> especially clear. It appears that increasing N increases the cost both
> of CPU and memory. But the "p" (parallelization) parameter acts as a
> multiplier on N, also increasing the cost. It is far from clear how one
> would choose appropriate values for N and p. For a given value of N*p,
> is it better for N to be large, or p to be large?
>
> I suggest that more thought be given to what these things mean in the
> context of this application, and then choose identifier names and
> descriptions accordingly. It may be better to refactor these some other
> way.
>
> The ASN.1 in section 6 assigns names to several of these identifiers. It
> would be helpful to readers if the names used in defining the algorithms
> were also the names used here.
>
> (Issue-6): Dubious stability of references
>
> I looked for prior discussion of this draft, and found some on the saag
> mailing list regarding the references.
>
> The definition of the Salsa20 hash function in
> http://cr.yp.to/snuffle/spec.pdf seems clear enough, but is the document
> reference stable? It might be safer to replicate the definition in this
> document (in an appendix) with attribution. It doesn't appear that there
> is any copyright in the referenced document. I'll also note that call to
> this hash function in scryptBlockMix is to "Salsa", not Salso20. It
> ought to be consistent with the definition.
>
> ------------------------
> Nits/editorial comments:
>
> (Issue-7): IdNits reported errors
>
> IdNits reports:
>
> -- Obsolete informational reference (is this intentional?): RFC 5208
>       (Obsoleted by RFC 5958)
>
> This is triggered by the following in section 6:
>
> "To be usable in PKCS#8 [RFC5208] and Asymmetric Key Packages [RFC5958]
> the following extension of the PBES2-KDFs type is needed."
>
> Since RFC5958 is referenced, and it obsoletes RFC5208, what is the
> reason for referencing both?
>
> (There is also an error about 2119, but it is bogus, triggered by the
> use of OPTIONAL in the ASN.1.)
>
> This completes my review.
>
>      Thanks,
>      Paul Kyzivat
>
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art