Add AES-SIV support #3540

tmshort · 2017-05-24T15:57:11Z

Checklist

documentation is added or updated
tests are added or updated

FdaSilvaYY · 2017-05-25T21:58:04Z

INSTALL

@@ -510,8 +510,8 @@
                   Build without support for the specified algorithm, where
                   <alg> is one of: bf, blake2, camellia, cast, chacha, cmac,
                   des, dh, dsa, ecdh, ecdsa, idea, md4, mdc2, ocb, poly1305,
-                   rc2, rc4, rmd160, scrypt, seed, siphash or whirlpool. The
-                   "ripemd" algorithm is deprecated and if used is synonymous
+                   rc2, rc4, rmd160, scrypt, seed, siphash aes-siv or whirlpool.


siphash, aes-siv

test/evp_test.c

dot-asm · 2017-06-14T10:08:18Z

Is there specific reason for why is it "hard-wired" to AES? SIV is a mode that can operate with any block cipher. Why does it get wired unconditionally to AES? In other words, wouldn't it be better if it was cipher-agnostic crypto/modes/siv128.c?

[And btw, red cross from appveyor is totally relevant.]

tmshort · 2017-06-14T12:42:31Z

Ah, Windows build failure, thanks @dot-asm.
As far as being wired to AES, I have only seen AES implementations and references, specifically RFC5297. Not to say, it couldn't be done.

dot-asm · 2017-06-14T13:14:12Z

RFC5297

Which opens with "This memo describes SIV, a block cipher mode of operation." Original paper reads "SIV can be based on an arbitrary blockcipher, such as AES or TDEA". The only additional and relevant reference to AES in the paper is "SIV never uses the inverse of the blockcipher, which is convenient for a blockcipher like AES." Well, admittedly RFC is more explicit asserting that E(K,X) indicates AES encryption of string X using key K, but this still doesn't preclude possibility of using it with alternative cipher. But there is even another reason. I'd argue that cipher-agnostic module would be more manageable and easier to maintain. Because it allows you to concentrate harder on one thing at a time, namely on mode specifics, and then on EVP wiring...

tmshort · 2017-06-14T13:17:13Z

@dot-asm Fair enough; the title specifically mentions AES, and the implementation that this was based on was originally (and only) AES. That being said, it'll take some time! :)

tmshort · 2017-06-14T13:19:54Z

(Don't get me wrong, when I first started this, I had thought of adding a mode, but I lacked familiarity in that code.)

dot-asm · 2017-06-14T13:45:18Z

Well, there is certain ambiguity in suggestion. Modules in crypto/modes aim for EVP-neutrality in sense that they don't make calls to EVP or depend on EVP structures. They are rather providers to EVP. But this is not necessarily the only way. Well, one of original rationales behind making them EVP-neutral was that it would allow to make better decisions when it comes to performance and re-usability. But still it doesn't mean that every mode has to be that way. What I'm trying to say it that I'd [let's say] insist on taking mode specific code out of e_aes.c. For better maintainability. But whether or not it will be crypto/modes-style EVP-neutral thing or one that still relies heavily on EVP could be open for discussion. Personally I'd prefer to see crypto/modes-style, but maybe it would make it unnecessarily complicated. Maybe you can start by taking current code out of evp/e_aes.c, put it into say evp/e_mode_siv.c... EVP_aes_*_siv would go to the latter. Idea would be if we want to add another-cipher-siv, we'd add few lines to evp/e_mode_siv.c...

If I also may make another suggestion. Would it be sensible to arrange objects.txt update as separate commit?

dot-asm · 2017-06-14T13:46:24Z

doc/man3/EVP_EncryptInit.pod

@@ -30,6 +30,7 @@ EVP_aes_192_cbc, EVP_aes_192_ecb, EVP_aes_192_cfb, EVP_aes_192_ofb,
 EVP_aes_256_cbc, EVP_aes_256_ecb, EVP_aes_256_cfb, EVP_aes_256_ofb,
 EVP_aes_128_gcm, EVP_aes_192_gcm, EVP_aes_256_gcm,
 EVP_aes_128_ccm, EVP_aes_192_ccm, EVP_aes_256_ccm,
+EVP_aes_128_civ, EVP_aes_192_siv, EVP_aes_256_siv,


Typo: EVP_aes_128_civ.

tmshort · 2017-06-15T15:12:14Z

We have a new mode! siv128

tmshort · 2017-06-15T18:49:58Z

Travis error is:
The command "$make update; git diff --quiet" exited with 1.

My make update is clean, so this is unrelated?

mattcaswell · 2017-06-15T18:52:16Z

Rebase. Should be fixed by #3688.

richsalz

Gee, sorry for missing this. Some notes, nothing major.

richsalz · 2017-09-07T23:59:35Z

crypto/modes/siv128.c

+
+#if !defined(OPENSSL_NO_SIV) && !defined(OPENSSL_NO_CMAC)
+
+static const union {


get rid of the run-time endian tests and just do the portable code.

You'll have to pull in some ifdef'd code from Daniel's original work.

This is based on other code in openssl that does the same thing.

richsalz · 2017-09-08T00:00:39Z

crypto/modes/siv128.c

+#include <openssl/cmac.h>
+#include "modes_lcl.h"
+
+#if !defined(OPENSSL_NO_SIV) && !defined(OPENSSL_NO_CMAC)


you can make no-siv imply no-cmac or otherwise tie them together in Configure; why test for both?

It's the opposite; SIV depends on CMAC, so no-cmac implies no-siv. It's already in Configure, so I can remove the CMAC checks.

richsalz · 2017-09-08T00:03:27Z

crypto/modes/modes_lcl.h

+
+#define SIV_LEN 16
+
+typedef union {


Put a name siv_block_u

richsalz · 2017-09-08T00:03:39Z

crypto/modes/siv128.c

+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ *
+ * Copyright 2017, Akamai Technologies. All Rights Reserved.


Remove the akamai copyright; put Daniel's name in the commit message.

I included a reference to his github; but the code has diverged considerably.

richsalz · 2017-09-08T00:06:16Z

crypto/modes/siv128.c

+}
+
+/*
+ * Copy an SIV128_CONTEXT object -- TODO


Do we need a copy operation? If not, remove this.

Comment out of date.

Copy is referenced in the CTRL.

richsalz · 2017-09-08T00:09:36Z

crypto/modes/siv128.c

+
+int CRYPTO_siv128_speed(SIV128_CONTEXT *ctx, int arg)
+{
+    if (arg == 1)


Use the ?: operator

richsalz · 2017-09-08T00:10:09Z

crypto/objects/objects.txt

@@ -1532,3 +1532,7 @@ id-pkinit 5                     : pkInitKDC             : Signing KDC Response
                            : Poly1305     : poly1305
 # NID for SipHash
                            : SipHash      : siphash
+# NID for AES-SIV
+                            : AES-128-SIV  : aes-128-siv


Where were these defined BTW?

In the crypto/objects/obj_dat.h

richsalz · 2017-09-08T00:11:22Z

test/evp_test.c

@@ -485,8 +490,13 @@ static int cipher_test_parse(EVP_TEST *t, const char *keyword,
    if (strcmp(keyword, "Ciphertext") == 0)
        return parse_bin(value, &cdat->ciphertext, &cdat->ciphertext_len);
    if (cdat->aead) {
-        if (strcmp(keyword, "AAD") == 0)
-            return parse_bin(value, &cdat->aad, &cdat->aad_len);
+        if (strcmp(keyword, "AAD") == 0) {


hm, should we have AAD1 AAD2 etc tags instead?

That limits the number of AADs, I think it's easier to just allow multiple entries.

richsalz · 2017-09-08T00:11:41Z

test/evp_test.c

-            if (!EVP_CipherUpdate(ctx, NULL, &chunklen, expected->aad,
-                                  expected->aad_len))
-                goto err;
+            for (i = 0; expected->aad[i] != NULL; i++){


space before the curly.

richsalz · 2017-09-08T00:12:17Z

test/evp_test.c

@@ -611,32 +621,36 @@ static int cipher_test_enc(EVP_TEST *t, int enc,
            goto err;
        }
    }
-    if (expected->aad) {
+    if (expected->aad[0]) {


Add NULL explicitly

richsalz · 2017-09-26T18:31:48Z

so is this all set and ready for final(!) review, @tmshort?

tmshort · 2017-09-26T19:44:05Z

Yes @richsalz, I just rebased

bernd-edlinger · 2017-09-27T16:00:20Z

travis says:
if [ -s doc-nits ] ; then cat doc-nits; rm doc-nits ; exit 1; fi
*** ERROR: unresolved internal link 'SIV Modes' at line 404 in file doc/man3/EVP_EncryptInit.pod
make: *** [doc-nits] Error 1

tmshort · 2017-09-27T16:01:37Z

I'll look at doc-nits.

tmshort · 2017-09-28T12:45:43Z

Fixed doc-nits, but now there's a libcrypto conflict.

changes made

richsalz · 2018-02-27T21:44:07Z

@dot-asm where are we with this? what needs to be done for your approval?

tmshort · 2018-09-19T13:28:04Z

Rebased on top of master

tmshort · 2018-10-24T19:44:01Z

Rebased again...

InfoHunter · 2018-10-25T10:29:50Z

This is really old...

tmshort · 2018-10-25T12:47:18Z

Yes, it was delayed (along with a lot of other enhancements) due to the TLSv1.3 effort that was the primary focus of 1.1.1.

tmshort · 2018-12-05T20:49:41Z

Rebased.

InfoHunter · 2018-12-06T09:34:34Z

Still pending for an OMC review, @openssl/omc

paulidale

Looks good apart from the EVP_MAC suggestion which can be a new PR.

paulidale · 2018-12-06T21:49:12Z

crypto/modes/siv128.c

+
+    if (key == NULL || cbc == NULL || ctr == NULL
+            || (ctx->cipher_ctx = EVP_CIPHER_CTX_new()) == NULL
+            || (ctx->cmac_ctx_init = CMAC_CTX_new()) == NULL


Thinking that all of the CMAC calls should use the new EVP_MAC API rather than going directly. Probably as a new PR.

paulidale · 2018-12-06T22:01:48Z

Need a rebase too.

tmshort · 2018-12-06T22:10:19Z

And I had just done that, too!

INSTALL

Based originally on github.com/dfoxfranke/libaes_siv This creates an SIV128 mode that uses EVP interfaces for the CBC, CTR and CMAC code to reduce complexity at the cost of perfomance. The expected use is for short inputs, not TLS-sized records. Add multiple AAD input capacity in the EVP tests.

paulidale · 2018-12-11T22:17:15Z

Merged to master, thanks to all involved.

Based originally on github.com/dfoxfranke/libaes_siv This creates an SIV128 mode that uses EVP interfaces for the CBC, CTR and CMAC code to reduce complexity at the cost of perfomance. The expected use is for short inputs, not TLS-sized records. Add multiple AAD input capacity in the EVP tests. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3540)

tmshort force-pushed the master-aes-siv branch from ba54642 to 2c2578d Compare May 24, 2017 19:18

FdaSilvaYY reviewed May 25, 2017

View reviewed changes

tmshort force-pushed the master-aes-siv branch from ed643c4 to a271540 Compare June 6, 2017 20:22

tmshort changed the title ~~WIP: Add AES-SIV support~~ Add AES-SIV support Jun 7, 2017

tmshort force-pushed the master-aes-siv branch from a271540 to b46faff Compare June 13, 2017 13:47

bernd-edlinger reviewed Jun 14, 2017

View reviewed changes

test/evp_test.c Outdated Show resolved Hide resolved

dot-asm reviewed Jun 14, 2017

View reviewed changes

tmshort force-pushed the master-aes-siv branch 2 times, most recently from 583ed82 to 2a15741 Compare June 15, 2017 14:30

tmshort force-pushed the master-aes-siv branch from 45dd4bc to aa0446d Compare June 16, 2017 18:38

shigeki mentioned this pull request Jun 20, 2017

crypto.createCipher should not work with AES-CTR nodejs/node#13801

Closed

kaduk mentioned this pull request Sep 7, 2017

WIP: Add AES-SIV #4348

Closed

richsalz previously requested changes Sep 8, 2017

View reviewed changes

tmshort force-pushed the master-aes-siv branch from aa0446d to d996767 Compare September 13, 2017 19:03

tmshort force-pushed the master-aes-siv branch from d996767 to eb15f0c Compare September 26, 2017 19:43

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

tmshort force-pushed the master-aes-siv branch from 4d6414e to fd05b0d Compare May 8, 2018 15:25

tmshort force-pushed the master-aes-siv branch from fd05b0d to d092900 Compare September 19, 2018 13:27

tmshort force-pushed the master-aes-siv branch from d092900 to b8136b7 Compare October 24, 2018 19:43

tmshort force-pushed the master-aes-siv branch from b8136b7 to 7bca5f3 Compare December 5, 2018 20:49

InfoHunter added the branch: master Merge to master branch label Dec 6, 2018

paulidale approved these changes Dec 6, 2018

View reviewed changes

paulidale 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 after 1.1.1 labels Dec 6, 2018

tmshort force-pushed the master-aes-siv branch from 7bca5f3 to 89b4d5d Compare December 7, 2018 16:53

tmshort commented Dec 7, 2018

View reviewed changes

INSTALL Outdated Show resolved Hide resolved

tmshort force-pushed the master-aes-siv branch from 89b4d5d to 59ae51d Compare December 7, 2018 16:55

paulidale closed this Dec 11, 2018

paulidale mentioned this pull request Dec 11, 2018

AES-SIV to use EVP_MAC APIs #7879

Closed

richsalz mentioned this pull request Oct 20, 2020

Clean up exported EVP_aes_n_siv symbols that shouldn't be. #13195

Closed

tmshort deleted the master-aes-siv branch June 9, 2021 18:43


		#if !defined(OPENSSL_NO_SIV) && !defined(OPENSSL_NO_CMAC)

		static const union {


		#define SIV_LEN 16

		typedef union {

Add AES-SIV support #3540

Add AES-SIV support #3540

Conversation

tmshort commented May 24, 2017

Checklist

Choose a reason for hiding this comment

dot-asm commented Jun 14, 2017

tmshort commented Jun 14, 2017 • edited Loading

dot-asm commented Jun 14, 2017

tmshort commented Jun 14, 2017 • edited Loading

tmshort commented Jun 14, 2017

dot-asm commented Jun 14, 2017

Choose a reason for hiding this comment

tmshort commented Jun 15, 2017

tmshort commented Jun 15, 2017

mattcaswell commented Jun 15, 2017

richsalz left a 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

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

richsalz commented Sep 26, 2017

tmshort commented Sep 26, 2017

bernd-edlinger commented Sep 27, 2017

tmshort commented Sep 27, 2017

tmshort commented Sep 28, 2017

richsalz commented Feb 27, 2018

tmshort commented Sep 19, 2018

tmshort commented Oct 24, 2018

InfoHunter commented Oct 25, 2018

tmshort commented Oct 25, 2018

tmshort commented Dec 5, 2018

InfoHunter commented Dec 6, 2018

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Dec 6, 2018

tmshort commented Dec 6, 2018

paulidale commented Dec 11, 2018

tmshort commented Jun 14, 2017 •

edited

Loading

tmshort commented Jun 14, 2017 •

edited

Loading