KMAC implementation using EVP_MAC #7597

slontis · 2018-11-09T05:34:38Z

Checklist

documentation is added or updated
tests are added or updated

crypto/kmac/kmac.c

paulidale

A few minor comments, but looking good for the most part.

crypto/evp/m_sha3.c

crypto/kmac/kmac.c

paulidale · 2018-11-09T21:39:09Z

crypto/kmac/kmac.c

+    if (kctx->custom_len == 0)
+        (void)kmac_ctrl_str(kctx, "custom", "");
+
+    return bytepad(out, &out_len, kmac_string, sizeof(kmac_string),


Do we support a sequence like:

init() init() update() final()

where the effects of the first init are ignored?

I don't see why we should and if that assumption is correct, the code is here if fine.

Hmm this should probably be made to work.. So I will cache md and not do the digestInit to the kmac_init().

crypto/kmac/kmac.c

paulidale · 2018-11-12T04:35:33Z

crypto/kmac/kmac.c

+
+    switch (cmd) {
+    case EVP_MAC_CTRL_SET_XOF:
+        kctx->xof_mode = va_arg(args, size_t);


The documentation claims the type should be unsigned long not size_t.

paulidale · 2018-11-12T04:36:20Z

doc/man7/EVP_MAC_KMAC.pod

+
+EVP_MAC_ctrl_str() type string: "xof"
+
+The value string is expected to be an unsigned long. Use 1 to enable XOF mode.


This type doesn't match the code (in two different ways). How about using an int?

levitte · 2018-11-12T06:29:49Z

crypto/kmac/kmac.c

+ *           right_encoded(requested output length).
+
+ * All other Ctrl functions should be set before init().
+ * EVP_MAC_CTRL_SET_MD must be set before EVP_MAC_CTRL_SET_KEY.


This line should be removed

levitte · 2018-11-12T06:33:56Z

crypto/kmac/kmac.c

+/*
+ * The following Ctrl functions can be set any time before final():
+ *     - EVP_MAC_CTRL_SET_SIZE: The requested output length.
+ *     - EVP_MAC_CTRL_SET_XOF_MODE: If set, this indicates that


Lose _MODE

levitte · 2018-11-12T06:40:52Z

include/openssl/evp.h

@@ -1027,6 +1030,9 @@ void EVP_MAC_do_all_sorted(void (*fn)
 # define EVP_MAC_CTRL_SET_CIPHER        0x05 /* EVP_CIPHER * */
 # define EVP_MAC_CTRL_SET_SIZE          0x06 /* size_t */
 # define EVP_MAC_CTRL_SET_IV            0x07 /* unsigned char *, size_t */
+# define EVP_MAC_CTRL_SET_CUSTOM        0x08 /* unsigned char *, size_t */
+# define EVP_MAC_CTRL_SET_XOF           0x09 /* unsigned long */
+


Lose the extra blank line

levitte · 2018-11-12T06:49:34Z

doc/man7/EVP_MAC_KMAC.pod

+
+=item B<EVP_MAC_CTRL_SET_KEY>
+
+This can only be used after B<EVP_MAC_CTRL_SET_MD> has been set.


Lose this line

Also, you should probably specify that this can't be called after EVP_MAC_init

levitte · 2018-11-12T06:50:08Z

doc/man7/EVP_MAC_KMAC.pod

+
+=back
+
+=item B<EVP_MAC_CTRL_SET_CUSTOM>


You should probably specify that this can't be called after EVP_MAC_init

levitte · 2018-11-12T06:51:07Z

doc/man7/EVP_MAC_KMAC.pod

+EVP_MAC_ctrl_str() type string: "outlen"
+
+The value string is expected to contain a decimal number. This can be used to
+override the default value that is specified by the digest.


This probably needs to be rephrased, since you don't set the MD any more.

levitte · 2018-11-12T06:54:37Z

doc/man3/EVP_MAC.pod

+
+=item B<EVP_MAC_CTRL_SET_XOF>
+
+This control expects one argument: C<unsigned long flags>


s/unsigned long/int/

levitte · 2018-11-12T06:55:53Z

include/openssl/evp.h

@@ -1027,6 +1030,9 @@ void EVP_MAC_do_all_sorted(void (*fn)
 # define EVP_MAC_CTRL_SET_CIPHER        0x05 /* EVP_CIPHER * */
 # define EVP_MAC_CTRL_SET_SIZE          0x06 /* size_t */
 # define EVP_MAC_CTRL_SET_IV            0x07 /* unsigned char *, size_t */
+# define EVP_MAC_CTRL_SET_CUSTOM        0x08 /* unsigned char *, size_t */
+# define EVP_MAC_CTRL_SET_XOF           0x09 /* unsigned long */


s/unsigned long/int/

should all be updated now.

paulidale · 2018-11-12T21:14:56Z

Could a test case with xof = 0 explicitly listed be added? This should catch the varargs type change.
Just amend an existing case with this, no need to create a new one.

paulidale

Still looks good.

paulidale · 2018-11-13T21:02:42Z

Merged to master. Thanks for the patience everyone.

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #7597)

paulidale reviewed Nov 9, 2018

View reviewed changes

crypto/kmac/kmac.c Outdated Show resolved Hide resolved

paulidale reviewed Nov 9, 2018

View reviewed changes

slontis force-pushed the new-kmac branch 2 times, most recently from a458bad to 2beca10 Compare November 12, 2018 02:21

paulidale reviewed Nov 12, 2018

View reviewed changes

crypto/kmac/kmac.c Show resolved Hide resolved

slontis force-pushed the new-kmac branch 2 times, most recently from b2ea98f to b369a18 Compare November 12, 2018 03:59

paulidale requested changes Nov 12, 2018

View reviewed changes

slontis force-pushed the new-kmac branch from b369a18 to 0def141 Compare November 12, 2018 04:50

paulidale approved these changes Nov 12, 2018

View reviewed changes

paulidale added approval: review pending This pull request needs review by a committer branch: master Merge to master branch labels Nov 12, 2018

levitte requested changes Nov 12, 2018

View reviewed changes

KMAC implementation using EVP_MAC

24a087f

slontis force-pushed the new-kmac branch from 0def141 to 24a087f Compare November 12, 2018 23:59

paulidale approved these changes Nov 13, 2018

View reviewed changes

levitte approved these changes Nov 13, 2018

View reviewed changes

levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 13, 2018

paulidale closed this Nov 13, 2018

levitte pushed a commit that referenced this pull request Nov 13, 2018

KMAC implementation using EVP_MAC

6e624a6

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #7597)

evqna mentioned this pull request Nov 14, 2018

Advanced BLAKE2 parameters support advice #7636

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KMAC implementation using EVP_MAC #7597

KMAC implementation using EVP_MAC #7597

slontis commented Nov 9, 2018 •

edited

paulidale left a comment

paulidale Nov 9, 2018

slontis Nov 12, 2018 •

edited

paulidale Nov 12, 2018

paulidale Nov 12, 2018

slontis Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

levitte Nov 12, 2018

slontis Nov 13, 2018

paulidale commented Nov 12, 2018

paulidale left a comment

paulidale commented Nov 13, 2018


		EVP_MAC_ctrl_str() type string: "xof"

		The value string is expected to be an unsigned long. Use 1 to enable XOF mode.


		=item B<EVP_MAC_CTRL_SET_KEY>

		This can only be used after B<EVP_MAC_CTRL_SET_MD> has been set.


		=item B<EVP_MAC_CTRL_SET_XOF>

		This control expects one argument: C<unsigned long flags>

KMAC implementation using EVP_MAC #7597

KMAC implementation using EVP_MAC #7597

Conversation

slontis commented Nov 9, 2018 • edited

Checklist

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis Nov 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Nov 12, 2018

paulidale left a comment

Choose a reason for hiding this comment

paulidale commented Nov 13, 2018

slontis commented Nov 9, 2018 •

edited

slontis Nov 12, 2018 •

edited