Call dh_params_check in the client cases

Way back in July Oscar Reparaz got in touch to report that s2n does not fuly
validate DH parameters. This is intentional, as s2n only acts as a server and
doesn't perform any client-side validation, but it did kick start some
interesting conversations with members of the OpenSSL team: should this kind
of check happen inside of OpenSSL, or is it really the caller's responsibility.

I got a chance to round out these discussions at the HACS conference recently,
as it also touched on issues related to how OpenSSL chooses to mitigate both
LogJam and CVE-2016-0701. The end result though is that it would be too
burdensome and large a change to do the DH_check implicity and always; there
are existing applications and caller paths this may break.

This change adds the check to s2n. Though s2n still performs no meaningful
client side validation (e.g. certificate) and client mode is disabled.

crypto/s2n_dhe.c

@@ -79,6 +79,9 @@ int s2n_pkcs3_to_dh_params(struct s2n_dh_params *dh_params, struct s2n_blob *pkc
         S2N_ERROR(S2N_ERR_DH_TOO_SMALL);
     }
 
+    /* Check the generator and prime */
+    GUARD(s2n_dh_params_check(dh_params));
+
     return 0;
 }
 
@@ -145,6 +148,7 @@ int s2n_dh_compute_shared_secret_as_client(struct s2n_dh_params *server_dh_param
     uint16_t public_key_size;
     int shared_key_size;
 
+    GUARD(s2n_dh_params_check(server_dh_params));
     GUARD(s2n_dh_params_copy(server_dh_params, &client_params));
     GUARD(s2n_dh_generate_ephemeral_key(&client_params));
     GUARD(s2n_alloc(shared_key, DH_size(server_dh_params->dh)));
@@ -209,6 +213,20 @@ int s2n_dh_compute_shared_secret_as_server(struct s2n_dh_params *server_dh_param
     return 0;
 }
 
+int s2n_dh_params_check(struct s2n_dh_params *params) {
+    int codes = 0;
+
+    if (DH_check(params->dh, &codes) == 0) {
+        S2N_ERROR(S2N_ERR_DH_PARAMETER_CHECK);;
+    }
+
+    if (codes != 0) {
+        S2N_ERROR(S2N_ERR_DH_PARAMETER_CHECK);;
+    }
+
+    return 0;
+}
+
 int s2n_dh_params_copy(struct s2n_dh_params *from, struct s2n_dh_params *to)
 {
     GUARD(s2n_check_p_g_dh_params(from));

crypto/s2n_dhe.h

@@ -31,5 +31,6 @@ extern int s2n_dh_params_to_p_g_Ys(struct s2n_dh_params *server_dh_params, struc
 extern int s2n_dh_compute_shared_secret_as_server(struct s2n_dh_params *server_dh_params, struct s2n_stuffer *Yc_in, struct s2n_blob *shared_key);
 extern int s2n_dh_compute_shared_secret_as_client(struct s2n_dh_params *server_dh_params, struct s2n_stuffer *Yc_out, struct s2n_blob *shared_key);
 extern int s2n_dh_params_copy(struct s2n_dh_params *from, struct s2n_dh_params *to);
+extern int s2n_dh_params_check(struct s2n_dh_params *params);
 extern int s2n_dh_generate_ephemeral_key(struct s2n_dh_params *dh_params);
 extern int s2n_dh_params_free(struct s2n_dh_params *dh_params);

error/s2n_errno.c

@@ -61,6 +61,7 @@ struct s2n_error_translation EN[] = {
     { S2N_ERR_DH_WRITING_PUBLIC_KEY, "error writing Diffie-Hellman public key" },
     { S2N_ERR_DH_FAILED_SIGNING, "error signing Diffie-Hellman values" },
     { S2N_ERR_DH_TOO_SMALL, "Diffie-Hellman parameters are too small" },
+    { S2N_ERR_DH_PARAMETER_CHECK, "Diffie-Hellman parameter check failed" },
     { S2N_ERR_INVALID_PKCS3, "invalid PKCS3 encountered" },
     { S2N_ERR_HASH_DIGEST_FAILED, "failed to create hash digest" },
     { S2N_ERR_HASH_INIT_FAILED, "error initializing hash" },

error/s2n_errno.h

@@ -52,6 +52,7 @@ typedef enum {
     S2N_ERR_DH_WRITING_PUBLIC_KEY,
     S2N_ERR_DH_FAILED_SIGNING,
     S2N_ERR_DH_TOO_SMALL,
+    S2N_ERR_DH_PARAMETER_CHECK,
     S2N_ERR_INVALID_PKCS3,
     S2N_ERR_HASH_DIGEST_FAILED,
     S2N_ERR_HASH_INIT_FAILED,

tests/unit/s2n_override_openssl_random_test.c

@@ -44,6 +44,8 @@ int main(int argc, char **argv)
 
     EXPECT_SUCCESS(s2n_init());
 
+    EXPECT_EQUAL(s2n_get_private_random_bytes_used(), 0);
+
     /* Parse the DH params */
     b.data = dhparams;
     b.size = sizeof(dhparams);
@@ -55,8 +57,6 @@ int main(int argc, char **argv)
     b.data = s2n_stuffer_raw_read(&dhparams_out, b.size);
     EXPECT_SUCCESS(s2n_pkcs3_to_dh_params(&dh_params, &b));
 
-    EXPECT_EQUAL(s2n_get_private_random_bytes_used(), 0);
-
     EXPECT_SUCCESS(s2n_dh_generate_ephemeral_key(&dh_params));
 
     /* Verify that our DRBG is called and that over-riding works */