From 80ee6c090120cc97dbaa5124f09142f92fb0388a Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Fri, 10 Apr 2020 12:42:22 +0200
Subject: [PATCH] crypto: check DiffieHellman p and g params

It's possible to pass in the prime and generator params as buffers
but that mode of input wasn't as rigorously checked as numeric input.
---
 src/node_crypto.cc              | 25 +++++++++++++++++++++++--
 test/parallel/test-crypto-dh.js | 31 +++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 348d407f0eb13a..34b19407c93150 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -5136,6 +5136,14 @@ bool DiffieHellman::Init(int primeLength, int g) {
 
 bool DiffieHellman::Init(const char* p, int p_len, int g) {
   dh_.reset(DH_new());
+  if (p_len <= 0) {
+    BNerr(BN_F_BN_GENERATE_PRIME_EX, BN_R_BITS_TOO_SMALL);
+    return false;
+  }
+  if (g <= 1) {
+    DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR);
+    return false;
+  }
   BIGNUM* bn_p =
       BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr);
   BIGNUM* bn_g = BN_new();
@@ -5151,10 +5159,23 @@ bool DiffieHellman::Init(const char* p, int p_len, int g) {
 
 bool DiffieHellman::Init(const char* p, int p_len, const char* g, int g_len) {
   dh_.reset(DH_new());
-  BIGNUM* bn_p =
-      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr);
+  if (p_len <= 0) {
+    BNerr(BN_F_BN_GENERATE_PRIME_EX, BN_R_BITS_TOO_SMALL);
+    return false;
+  }
+  if (g_len <= 0) {
+    DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR);
+    return false;
+  }
   BIGNUM* bn_g =
       BN_bin2bn(reinterpret_cast<const unsigned char*>(g), g_len, nullptr);
+  if (BN_is_zero(bn_g) || BN_is_one(bn_g)) {
+    BN_free(bn_g);
+    DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR);
+    return false;
+  }
+  BIGNUM* bn_p =
+      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr);
   if (!DH_set0_pqg(dh_.get(), bn_p, nullptr, bn_g)) {
     BN_free(bn_p);
     BN_free(bn_g);
diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js
index e1c5db968329f3..b23a51c491a9b5 100644
--- a/test/parallel/test-crypto-dh.js
+++ b/test/parallel/test-crypto-dh.js
@@ -45,6 +45,37 @@ for (const bits of [-1, 0, 1]) {
   });
 }
 
+// Through a fluke of history, g=0 defaults to DH_GENERATOR (2).
+{
+  const g = 0;
+  crypto.createDiffieHellman('abcdef', g);
+  crypto.createDiffieHellman('abcdef', 'hex', g);
+}
+
+for (const g of [-1, 1]) {
+  const ex = {
+    code: 'ERR_OSSL_DH_BAD_GENERATOR',
+    name: 'Error',
+    message: /bad generator/,
+  };
+  assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex);
+  assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex);
+}
+
+crypto.createDiffieHellman('abcdef', Buffer.from([2]));  // OK
+
+for (const g of [Buffer.from([]),
+                 Buffer.from([0]),
+                 Buffer.from([1])]) {
+  const ex = {
+    code: 'ERR_OSSL_DH_BAD_GENERATOR',
+    name: 'Error',
+    message: /bad generator/,
+  };
+  assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex);
+  assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex);
+}
+
 {
   const DiffieHellman = crypto.DiffieHellman;
   const dh = DiffieHellman(p1, 'buffer');