GOST key agreement cofactor fix (#265)

* GOST key agreement cofactor fix

(cherry picked from commit dbc8f4780fa78d66a68174f78f9ae9aa9cdad53c)

diff --git a/gost_ec_keyx.c b/gost_ec_keyx.c
index de52dec2c2a703ebb4dfa50535e8b5c83cf1e154..85a69988d5f3d64f579d9c6c5d8d47f7aee465bb 100644 (file)
--- a/gost_ec_keyx.c
+++ b/gost_ec_keyx.c
@@ -24,16 +24,16 @@ int VKO_compute_key(unsigned char *shared_key,
                     const int vko_dgst_nid)
 {
     unsigned char *databuf = NULL;
-    BIGNUM *UKM = NULL, *p = NULL, *order = NULL, *X = NULL, *Y = NULL, *cofactor = NULL;
-    const BIGNUM *key = EC_KEY_get0_private_key(priv_key);
-    EC_POINT *pnt = EC_POINT_new(EC_KEY_get0_group(priv_key));
-    BN_CTX *ctx = BN_CTX_secure_new();
+    BIGNUM *scalar = NULL, *order = NULL, *X = NULL, *Y = NULL;
+    const EC_GROUP *grp = NULL;
+    EC_POINT *pnt = NULL;
+    BN_CTX *ctx = NULL;
     EVP_MD_CTX *mdctx = NULL;
     const EVP_MD *md = NULL;
     int buf_len, half_len;
     int ret = 0;
 
-    if (!ctx) {
+    if ((ctx = BN_CTX_secure_new()) == NULL) {
         GOSTerr(GOST_F_VKO_COMPUTE_KEY, ERR_R_MALLOC_FAILURE);
         return 0;
     }
@@ -45,30 +45,40 @@ int VKO_compute_key(unsigned char *shared_key,
         goto err;
     }
 
-    UKM = BN_lebin2bn(ukm, ukm_size, NULL);
-    p = BN_CTX_get(ctx);
-    order = BN_CTX_get(ctx);
-               cofactor = BN_CTX_get(ctx);
+    grp = EC_KEY_get0_group(priv_key);
+    scalar = BN_CTX_get(ctx);
+    order  = BN_CTX_get(ctx);
     X = BN_CTX_get(ctx);
-    Y = BN_CTX_get(ctx);
-    EC_GROUP_get_order(EC_KEY_get0_group(priv_key), order, ctx);
-               EC_GROUP_get_cofactor(EC_KEY_get0_group(priv_key), cofactor, ctx);
-    BN_mod_mul(UKM, UKM, cofactor, order, ctx);
-    BN_mod_mul(p, key, UKM, order, ctx);
-    if (!EC_POINT_mul(EC_KEY_get0_group(priv_key), pnt, NULL, pub_key, p, ctx)) {
+
+    if ((Y = BN_CTX_get(ctx)) == NULL
+        || (pnt = EC_POINT_new(grp)) == NULL
+        || BN_lebin2bn(ukm, ukm_size, scalar) == NULL
+        || !BN_mod_mul(scalar, scalar, EC_KEY_get0_private_key(priv_key),
+                       EC_GROUP_get0_order(grp), ctx))
+        goto err;
+
+    /* these two curves have cofactor 4; the rest have cofactor 1 */
+    switch (EC_GROUP_get_curve_name(grp)) {
+        case NID_id_tc26_gost_3410_2012_256_paramSetA:
+        case NID_id_tc26_gost_3410_2012_512_paramSetC:
+            if (!BN_lshift(scalar, scalar, 2))
+                goto err;
+            break;
+    }
+
+    if (!EC_POINT_mul(grp, pnt, NULL, pub_key, scalar, ctx)) {
         GOSTerr(GOST_F_VKO_COMPUTE_KEY, GOST_R_ERROR_POINT_MUL);
         goto err;
     }
-    if (!EC_POINT_get_affine_coordinates(EC_KEY_get0_group(priv_key),
-                                        pnt, X, Y, ctx)) {
-       GOSTerr(GOST_F_VKO_COMPUTE_KEY, ERR_R_EC_LIB);
-       goto err;
+    if (!EC_POINT_get_affine_coordinates(grp, pnt, X, Y, ctx) ||
+                       !EC_GROUP_get_order(grp, order, ctx)) {
+        GOSTerr(GOST_F_VKO_COMPUTE_KEY, ERR_R_EC_LIB);
+        goto err;
     }
 
     half_len = BN_num_bytes(order);
     buf_len = 2 * half_len;
-    databuf = OPENSSL_zalloc(buf_len);
-    if (!databuf) {
+    if ((databuf = OPENSSL_malloc(buf_len)) == NULL) {
         GOSTerr(GOST_F_VKO_COMPUTE_KEY, ERR_R_MALLOC_FAILURE);
         goto err;
     }
@@ -76,13 +86,11 @@ int VKO_compute_key(unsigned char *shared_key,
     /*
      * Serialize elliptic curve point same way as we do it when saving key
      */
-    store_bignum(Y, databuf, half_len);
-    store_bignum(X, databuf + half_len, half_len);
-    /* And reverse byte order of whole buffer */
-    BUF_reverse(databuf, NULL, buf_len);
+    if (BN_bn2lebinpad(X, databuf, half_len) != half_len
+        || BN_bn2lebinpad(Y, databuf + half_len, half_len) != half_len)
+        goto err;
 
-    mdctx = EVP_MD_CTX_new();
-    if (!mdctx) {
+    if ((mdctx = EVP_MD_CTX_new()) == NULL) {
         GOSTerr(GOST_F_VKO_COMPUTE_KEY, ERR_R_MALLOC_FAILURE);
         goto err;
     }
@@ -93,14 +101,10 @@ int VKO_compute_key(unsigned char *shared_key,
     ret = (EVP_MD_size(md) > 0) ? EVP_MD_size(md) : 0;
 
  err:
-    BN_free(UKM);
     BN_CTX_end(ctx);
     BN_CTX_free(ctx);
-
     EC_POINT_free(pnt);
-
     EVP_MD_CTX_free(mdctx);
-
     OPENSSL_free(databuf);
 
     return ret;

diff --git a/test/04-pkey.t b/test/04-pkey.t
index 5065745473096d16c6a7beaff6fee7acf204b9c8..f5e654da54b74db07c7a40a7731eb4c2867e811d 100644 (file)
--- a/test/04-pkey.t
+++ b/test/04-pkey.t
@@ -226,7 +226,10 @@ MD4CAQAwFwYIKoUDBwEBAQEwCwYJKoUDBwECAQEBBCD5+u2ebYwQ9iDYWHmif4XeGgj2OijJuq4YsbTN
 MD4CAQAwFwYIKoUDBwEBAQEwCwYJKoUDBwECAQEBBCDVwXdvq1zdBBmzVjG1WOBQR/dkwCzF6KSIiVkfQVCsKg==
 -----END PRIVATE KEY-----',
 'c019d8939e12740a328625cea86efa3b39170412772b3c110536410bdd58a854',
-'e9f7c57547fa0cd3c9942c62f9c74a553626d5f9810975a476825cd6f22a4e86'],
+'e9f7c57547fa0cd3c9942c62f9c74a553626d5f9810975a476825cd6f22a4e86',
+'-----BEGIN PUBLIC KEY-----
+MF4wFwYIKoUDBwEBAQEwCwYJKoUDBwECAQEBA0MABEB3WS+MEcXnrMCdavPRgF28U5PDlV1atDh1ADUFxoB/f80OjqQ0T7cGQtk/2nWCGDX7uUrBGA8dql8Bnw9Sgn5+
+-----END PUBLIC KEY-----'],
 'id-tc26-gost-3410-2012-256-paramSetB'=>
 ['-----BEGIN PRIVATE KEY-----
 MD4CAQAwFwYIKoUDBwEBAQEwCwYJKoUDBwECAQECBCDQ6G51VK2+96rvFyG/dRqWOFNJA33jQajAnzra585aIA==
@@ -286,11 +289,14 @@ MF4CAQAwFwYIKoUDBwEBAQIwCwYJKoUDBwECAQIDBEA79FKW7MqF4pQJJvpAhKd9YkwsFXBzcaUhYt3N
 MF4CAQAwFwYIKoUDBwEBAQIwCwYJKoUDBwECAQIDBEAiCNNQAMnur4EG8eSDpr5WjJaoHquSsK3wydCrGM3Cdbaa0kiuj5m0Mx16Vow7AwvG2DvlKJL8HgwuBqWlDaYa
 -----END PRIVATE KEY-----',
 '6e1db0da8832660fbf761119e41d356a1599686a157c9a598b8e18b56cb09791',
-'2df0dfa8d437689d41fad965f13ea28ce27c29dd84514b376ea6ad9f0c7e3ece']
+'2df0dfa8d437689d41fad965f13ea28ce27c29dd84514b376ea6ad9f0c7e3ece',
+'-----BEGIN PUBLIC KEY-----
+MIGgMBcGCCqFAwcBAQECMAsGCSqFAwcBAgECAwOBhAAEgYCPdAER26Ym73DSUXBamTLJcntdV3oZ7RRx/+Ijf13GnF36o36i8tEC13uJqOOmujEkAGPtui6yE4iJNVU0uM6yHmIEM5H0c81Sd/VQD8yXW1hyGAZvTMc+U/6oa30YU9YY7+t759d1CIVznPmq9C+VbAApyDCMFjuYnKD/nChsGA==
+-----END PUBLIC KEY-----']
 );
-    plan(52);
+    plan(54);
     while(my($id, $v) = each %derives) {
-        my ($alice,$alicehash,$bob,$bobhash,$secrethash) = @$v;
+        my ($alice,$alicehash,$bob,$bobhash,$secrethash,$malice) = @$v;
         # Alice: keygen
         open $F,">",'alice.prv';
         print $F $alice;
@@ -309,6 +315,14 @@ MF4CAQAwFwYIKoUDBwEBAQIwCwYJKoUDBwECAQIDBEAiCNNQAMnur4EG8eSDpr5WjJaoHquSsK3wydCr
         # Bob: derive
         system("openssl pkeyutl -derive -inkey bob.prv -keyform PEM -peerkey alice.pub.der -peerform DER -pkeyopt ukmhex:0100000000000000 -out secret_b.bin");
         like(`openssl dgst -sha256 -r secret_b.bin`, qr/^$secrethash/, "Compute shared key:$id:Bob");
+        if ($malice ne "") {
+            # Malice: negative test -- this PEM is in the small subgroup
+            open $F,">",'malice.pub';
+            print $F $malice;
+            close $F;
+            # NB system should return true on failure, so this is a negative test
+            ok(system("openssl pkeyutl -derive -inkey bob.prv -keyform PEM -peerkey malice.pub -peerform PEM -pkeyopt ukmhex:0100000000000000 -out secret_m.bin"), "Compute shared key:$id:Malice");
+        }
     }
     unlink "alice.prv";
     unlink "alice.pub.der";
@@ -316,6 +330,8 @@ MF4CAQAwFwYIKoUDBwEBAQIwCwYJKoUDBwECAQIDBEAiCNNQAMnur4EG8eSDpr5WjJaoHquSsK3wydCr
     unlink "bob.pub.der";
     unlink "secret_a.bin";
     unlink "secret_b.bin";
+    unlink "malice.pub";
+    unlink "secret_m.bin";
 };
 
 # 10. Разобрать стандартый encrypted key