[Cfrg] Review of draft-irtf-cfrg-argon2-02

Russ Housley <housley@vigilsec.com> Fri, 23 June 2017 21:19 UTC

Return-Path: <housley@vigilsec.com>
X-Original-To: cfrg@ietfa.amsl.com
Delivered-To: cfrg@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 61BCD129ADD for <cfrg@ietfa.amsl.com>; Fri, 23 Jun 2017 14:19:11 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bVoojlhKMWmX for <cfrg@ietfa.amsl.com>; Fri, 23 Jun 2017 14:19:08 -0700 (PDT)
Received: from mail.smeinc.net (mail.smeinc.net [209.135.209.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7003B1294F8 for <cfrg@ietf.org>; Fri, 23 Jun 2017 14:19:08 -0700 (PDT)
Received: from localhost (localhost [127.0.0.1]) by mail.smeinc.net (Postfix) with ESMTP id DAA23300483 for <cfrg@ietf.org>; Fri, 23 Jun 2017 17:19:07 -0400 (EDT)
X-Virus-Scanned: amavisd-new at mail.smeinc.net
Received: from mail.smeinc.net ([127.0.0.1]) by localhost (mail.smeinc.net [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id vLJkiOmDxzM5 for <cfrg@ietf.org>; Fri, 23 Jun 2017 17:19:06 -0400 (EDT)
Received: from a860b60074bd.home (pool-108-45-101-150.washdc.fios.verizon.net [108.45.101.150]) by mail.smeinc.net (Postfix) with ESMTPSA id DA78330029C for <cfrg@ietf.org>; Fri, 23 Jun 2017 17:19:06 -0400 (EDT)
From: Russ Housley <housley@vigilsec.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Message-Id: <67210408-F27E-4AE9-9ED6-6777C70530DA@vigilsec.com>
Date: Fri, 23 Jun 2017 17:19:06 -0400
To: cfrg@ietf.org
X-Mailer: Apple Mail (2.3273)
Archived-At: <https://mailarchive.ietf.org/arch/msg/cfrg/a8x8zS5mcrti7p3gJJCF8zoOzxM>
Subject: [Cfrg] Review of draft-irtf-cfrg-argon2-02
X-BeenThere: cfrg@irtf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Crypto Forum Research Group <cfrg.irtf.org>
List-Unsubscribe: <https://www.irtf.org/mailman/options/cfrg>, <mailto:cfrg-request@irtf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/cfrg/>
List-Post: <mailto:cfrg@irtf.org>
List-Help: <mailto:cfrg-request@irtf.org?subject=help>
List-Subscribe: <https://www.irtf.org/mailman/listinfo/cfrg>, <mailto:cfrg-request@irtf.org?subject=subscribe>
X-List-Received-Date: Fri, 23 Jun 2017 21:19:11 -0000

Document: draft-irtf-cfrg-argon2-02
Reviewer: Russ Housley
Review Date: 2017-06-23

Summary: Almost Ready

Once the comments below are resolved, I think that the ISE should
publish the document.


Major Concerns:

Section 3.3: The pseudo-code includes this line:

               V_{r+1} = H_{T-32*r}(V_{r})

Up to this point, I thought that the Argon2 construction could work with
many different hash functions, but at this point I realized that Argon2
depends the ability of Blake2b to produce an output between 1 and 64
bytes.  Please add a sentence or two to the introduction so that others
can avoid this misunderstanding.

Section 3.5: This section defines the compression function G.  It makes
use of the BLAKE2b round function P, which is explained in Section 3.6.
Section 3.6 defines another function G, which is different than the 
compression function.  I realize that both G functions are discussed in
other documents ([I-D.saarinen-blake2] and [ARGON2]), but there ought to
be a way to avoid this name collision.

Section 5: I tried to compile the code, and it does not work with gcc.
Is there a missing include file?


Minor Concerns:


Section 2 defines "E_f" as a variable E with subscript index f; however,
in Section 3.3, defines "H_x" as a hash function with x-byte output.
Please use a different notation for variables and functions.

Section 5: A main function would make it easier to use this reference
code.  Can you include something simple?  Perhaps something that prints
the tags associated with the test vectors in Section 6.


Nits:

Section 1: s/implementer oriented/implementer-oriented/

Section 1: s/data-depending memory access/data-dependent memory access/

Section 2: Please add a descriptions for the floor and ceil functions.

Section 2: Please add a description for length(P), which always returns
32 bits.

Section 2 defines "a ^ b", but Sections 3.2 and 3.5 use "a XOR b".
Pick one.

Most of the document uses "=" for assignment, but Section 3.6 uses "<-".
Please use "=" throughout.

Several places, the document tells us that "integers are padded to 4
bytes and encoded in little endian".  Maybe a "uint32" function should
be specified to avoid repeating this phrase.

Section 3.2: s/described in later section./described in later sections./

Section 6.3: Please correct the formatting.

Section 9.1: s/calls to Blake2b is needed/calls to Blake2b are needed/

Section 9.2: s/number t of passes/the number of passes, t,/

Section 9.3: s/for Argon2i 3 passes/for Argon2i, 3 passes/

Section 9.3: s/for Argon2d and Argon2id 1 pass/for Argon2d and Argon2id, 1 pass/


Personal preference:

Section 3.5: s/applied rowwise, and then columnwise/
              /applied to each row, and then to each column/