Add GCM ciphers (AES/ARIA) to providers by slontis · Pull Request #9231 · openssl/openssl

slontis · 2019-06-24T13:49:39Z

Checklist

documentation is added or updated
tests are added or updated

slontis · 2019-06-24T22:03:51Z

The s390 AES_GCM support requires the z14 model, which I currently don't have access to.

paulidale · 2019-06-25T01:47:42Z

@p-steuer are you able to help on the S390 front?

p-steuer · 2019-06-25T08:03:13Z

yes, ill look at it.

providers/common/ciphers/ciphers_locl.h

providers/common/ciphers/aes_gcm_s390x.inc

crypto/evp/evp_enc.c

providers/common/ciphers/aes_gcm_s390x.inc

p-steuer

With my non-question comments resolved, the code builds warning-free and passes openssl's test suite on z14.

slontis · 2019-06-26T05:06:20Z

Thanks @p-steuer for looking at this. I have updated the code, could you reconfirm that this updated code passes the tests & actually runs the kma code.
I have compiled and run on a z13 so it should at least compile..
NOTE: 04-test_params.t currently fails on a few platforms including s390 (a PR has been submitted for this by @paulidale) which is not related to this PR.

p-steuer · 2019-06-26T09:35:51Z

NOTE: 04-test_params.t currently fails on a few platforms including s390 (a PR has been submitted for this by @paulidale) which is not related to this PR.

Yes, i noticed it was still failing after my fixes but i already thought its unrelated.

could you reconfirm that this updated code passes the tests & actually runs the kma code.

Will do today.

providers/common/ciphers/aes_gcm_s390x.inc

p-steuer · 2019-06-26T11:29:52Z

providers/common/ciphers/aes_gcm_s390x.inc

This macro would only be used in the S390X_gcm_ivpadlen macro directly below plus padding is lcm(ivlen,16)+16 as S390X_gcm_ivpadlen does it, not just 16.
So id say keep the +16 instead of introducing a new macro (there is no predefined GHASH_BLOCK_SIZE macro).

providers/common/ciphers/aes_gcm_s390x.inc

p-steuer · 2019-06-26T11:48:35Z

Heres a diff of the proposed changes:

diff --git a/providers/common/ciphers/aes_gcm_s390x.inc b/providers/common/ciphers/aes_gcm_s390x.inc
index c3650a03e7..b8ca0d28c0 100644
--- a/providers/common/ciphers/aes_gcm_s390x.inc
+++ b/providers/common/ciphers/aes_gcm_s390x.inc
@@ -33,10 +33,8 @@
                                     (OPENSSL_s390xcap_P.kma[0] &    \
                                      S390X_CAPBIT(S390X_AES_256)))
 
-# define S390X_OUT_LEN    16
-# define S390X_IV_PAD_LEN 16
 /* iv + padding length for iv lengths != 12 */
-# define S390X_gcm_ivpadlen(i)  ((((i) + 15) >> 4 << 4) + S390X_IV_PAD_LEN)
+# define S390X_gcm_ivpadlen(i)  ((((i) + 15) >> 4 << 4) + 16)
 
 static int s390x_aes_gcm_init_key(PROV_AES_GCM_KEY *ctx,
                                   const unsigned char *key, size_t keylen)
@@ -52,7 +50,7 @@ static int s390x_aes_gcm_init_key(PROV_AES_GCM_KEY *ctx,
 /*-
  * Initialize context structure. Code is big-endian.
  */
-static void s390x_aes_gcm_setiv(PROV_AES_GCM_KEY *ctx, const unsigned char *iv,
+static int s390x_aes_gcm_setiv(PROV_AES_GCM_KEY *ctx, const unsigned char *iv,
                                 size_t ivlen)
 {
     S390X_KMA_PARAMS *kma = &ctx->plat.s390x.param.kma;
@@ -72,7 +70,7 @@ static void s390x_aes_gcm_setiv(PROV_AES_GCM_KEY *ctx, const unsigned char *iv,
     } else {
         unsigned long long ivbits = ivlen << 3;
         size_t len = S390X_gcm_ivpadlen(ivlen);
-        unsigned char iv_zero_pad[AES_GCM_IV_MAX_SIZE + S390X_IV_PAD_LEN];
+        unsigned char iv_zero_pad[S390X_gcm_ivpadlen(AES_GCM_IV_MAX_SIZE)];
         /*
          * The IV length needs to be zero padded to be a multiple of 16 bytes
          * followed by 8 bytes of zeros and 8 bytes for the IV length.
@@ -102,7 +100,7 @@ static void s390x_aes_gcm_setiv(PROV_AES_GCM_KEY *ctx, const unsigned char *iv,
 static int s390x_aes_gcm_cipher_final(PROV_AES_GCM_KEY *ctx, unsigned char *tag)
 {
     S390X_KMA_PARAMS *kma = &ctx->plat.s390x.param.kma;
-    unsigned char out[S390X_OUT_LEN];
+    unsigned char out[AES_BLOCK_SIZE];
 
     kma->taadl <<= 3;
     kma->tpcl <<= 3;
@@ -143,13 +141,21 @@ static int s390x_aes_gcm_one_shot(PROV_AES_GCM_KEY *ctx,
                                   unsigned char *tag, size_t taglen)
 {
     S390X_KMA_PARAMS *kma = &ctx->plat.s390x.param.kma;
+    int rc;
 
     kma->taadl = aad_len << 3;
     kma->tpcl = in_len << 3;
     s390x_kma(aad, aad_len, in, in_len, out,
               ctx->plat.s390x.fc | S390X_KMA_LAAD | S390X_KMA_LPC, kma);
-    memcpy(tag, kma->t.b, taglen);
-    return 1;
+
+    if (ctx->enc) {
+        memcpy(tag, kma->t.b, taglen);
+        rc = 1;
+    } else {
+        rc = CRYPTO_memcmp(tag, kma->t.b, taglen) != 0 ? 0 : 1;
+    }
+
+    return rc;
 }
 
 /*

p-steuer · 2019-06-26T11:54:20Z

With the proposed changes, test suite is passed (excuding params test) and kma instruction is used. perf tool report from openssl speed -evp aes-192-gcm -bytes 16000 -seconds 15 (similar for other key sizes):

  97.15%  openssl  libcrypto.so.3    [.] s390x_kma
   0.75%  openssl  libcrypto.so.3    [.] s390x_aes_gcm
   0.60%  openssl  libcrypto.so.3    [.] evp_EncryptDecryptUpdate
[...]

slontis · 2019-06-26T22:50:16Z

Heres a diff of the proposed changes:

Thankyou again @p-steuer. Your changes have been merged..

slontis · 2019-06-26T22:53:38Z

Now that the s390 is functional the WIP name has been removed.

slontis · 2019-07-16T12:39:53Z

ok hopefully that fixes it.. If not I will get back to it tomorrow.

p-steuer

all good now.

mattcaswell · 2019-07-16T14:58:39Z

providers/common/ciphers/aes_gcm.c

Shouldn't we confirm that ivlen == ctx->ivlen? Most importantly, if ivlen is less than ctx->ivlen then we could end up with an OOB read. I see the equivalent check does exist for the keylen, so it seems strange not to also do it for the IV.

No: The way that this works is that it passes in the default length (which comes from EVP_CIPHER_CTX_iv_length() which is always 12 bytes
the ctx->ivlen can be set by the caller to anything that fits in the iv_buffer. It checks its size in set_params.

See also the existing comment above the function

The keylength however needs to be the default size

Hmmm. Then something isn't right. The ivlen parameter comes from ivlen in aes_gcm_einit or aes_gcm_dinit. Which is intended to be the length of the IV actually passed. That at least is supposed to be the contract between libcrypto<->providers. Now, due to deficiencies with the APIs as they currently are, libcrypto has to figure out how long that IV is when it calls the provider which is I guess why its calling EVP_CIPHER_CTX_iv_length(). I imagine some future iteration of the API where an application passes an explicit length.

providers/common/ciphers/aes_gcm.c

providers/common/ciphers/aes_gcm_hw.c

providers/common/ciphers/aes_gcm_s390x.inc

slontis · 2019-07-17T13:27:35Z

ping @mattcaswell

mattcaswell · 2019-07-17T13:50:27Z

providers/common/ciphers/aes_gcm.c

Hmmm. Then something isn't right. The ivlen parameter comes from ivlen in aes_gcm_einit or aes_gcm_dinit. Which is intended to be the length of the IV actually passed. That at least is supposed to be the contract between libcrypto<->providers. Now, due to deficiencies with the APIs as they currently are, libcrypto has to figure out how long that IV is when it calls the provider which is I guess why its calling EVP_CIPHER_CTX_iv_length(). I imagine some future iteration of the API where an application passes an explicit length.

providers/common/ciphers/aes_gcm.c

p-steuer · 2019-07-17T20:48:30Z

Just noticed s390 build is broken right now due to the missing gcm defines in aes_platform.h thing due to 459b15d . I thought it was previously broke due to this PR.

So i think it would make sense to put the fix in a seperate commit and not hide it inside the other changes.

slontis · 2019-07-17T22:01:45Z

@mattcaswell

From this API:
int EVP_CIPHER_iv_length(const EVP_CIPHER *cipher)
It will get the default ivlength

And then this one really does the same thing...
int EVP_CIPHER_CTX_iv_length(const EVP_CIPHER_CTX *ctx)
{
return EVP_CIPHER_iv_length(ctx->cipher);
}
So I assume it has been broken for some time??
I am not really sure what you want me to do with it. (It looks like it wont be backwards compatible if we go changing what it does)

slontis · 2019-07-17T22:02:41Z

So i think it would make sense to put the fix in a seperate commit and not hide it inside the other changes.

I was hoping this wouldnt take so long to get merged. Will add a seperate PR.
#9403

The code has been modularized so that it can be shared by algorithms. A fixed size IV is now used instead of being allocated. The IV is not set into the low level struct now until the update (it uses an iv_state for this purpose). Hardware specific methods have been added to a PROV_GCM_HW object. The S390 code has been changed to just contain methods that can be accessed in a modular way. There are equivalent generic methods also for the other platforms.

slontis · 2019-07-25T04:59:55Z

ping
Just as was done with CCM, GCM has been reworked so that it can be shared by AES and ARIA.
Approvals are no longer valid.

providers/common/ciphers/ciphers_locl.h

mattcaswell · 2019-07-25T11:13:11Z

providers/common/ciphers/gcm.c

+
+static void gcm_deinitctx(PROV_GCM_CTX *ctx)
+{
+    OPENSSL_cleanse(ctx->iv, sizeof(ctx->iv));


Isn't the IV public anyway? So why is this needed?

I dont think it will hurt - this pattern is in a lot of places.

providers/common/ciphers/gcm.c

providers/common/ciphers/gcm_hw.c

providers/common/ciphers/gcm.c

slontis · 2019-07-30T06:54:34Z

@mattcaswell updated to address the ivlength issue + iv gen can happen in non fips mode.

mattcaswell

Approved subject to the naming nit being fixed.

providers/common/ciphers/gcm.c

The code has been modularized so that it can be shared by algorithms. A fixed size IV is now used instead of being allocated. The IV is not set into the low level struct now until the update (it uses an iv_state for this purpose). Hardware specific methods have been added to a PROV_GCM_HW object. The S390 code has been changed to just contain methods that can be accessed in a modular way. There are equivalent generic methods also for the other platforms. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Patrick Steuer <[email protected]> (Merged from #9231)

slontis · 2019-07-31T12:01:24Z

Thanks.. Merged to master.

slontis force-pushed the prov_aes_gcm branch from df7ef55 to df83f6f Compare June 24, 2019 22:01

p-steuer self-requested a review June 25, 2019 08:03

p-steuer reviewed Jun 25, 2019

View reviewed changes

providers/common/ciphers/ciphers_locl.h Outdated Show resolved Hide resolved

p-steuer reviewed Jun 25, 2019

View reviewed changes