From 5f9df9b2ad95ba5220356ae24f34d4d6621a20b8 Mon Sep 17 00:00:00 2001
From: Janos Follath <janos.follath@arm.com>
Date: Wed, 20 Sep 2017 13:46:37 +0100
Subject: [PATCH 1/2] DHM: Add negative tests for parameter checking

A bug in the dhm_check_range() function makes it pass even when the
parameters are not in the range. This commit adds tests for signalling
this problem as well as a couple of other negative tests.
---
 tests/suites/test_suite_dhm.data     | 18 +++++++++++++++---
 tests/suites/test_suite_dhm.function |  7 +++++--
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data
index f2cdeffa5..5dbe4c8ae 100644
--- a/tests/suites/test_suite_dhm.data
+++ b/tests/suites/test_suite_dhm.data
@@ -1,11 +1,23 @@
 Diffie-Hellman full exchange #1
-dhm_do_dhm:10:"23":10:"5"
+dhm_do_dhm:10:"23":10:"5":0
 
 Diffie-Hellman full exchange #2
-dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622"
+dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0
 
 Diffie-Hellman full exchange #3
-dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271"
+dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271":0
+
+Diffie-Hellman trivial subgroup #1
+dhm_do_dhm:10:"23":10:"1":POLARSSL_ERR_DHM_BAD_INPUT_DATA
+
+Diffie-Hellman trivial subgroup #2
+dhm_do_dhm:10:"23":10:"-1":POLARSSL_ERR_DHM_BAD_INPUT_DATA
+
+Diffie-Hellman small modulus
+dhm_do_dhm:10:"3":10:"5":POLARSSL_ERR_DHM_MAKE_PARAMS_FAILED
+
+Diffie-Hellman zero modulus
+dhm_do_dhm:10:"0":10:"5":POLARSSL_ERR_DHM_BAD_INPUT_DATA
 
 Diffie-Hallman load parameters from file
 dhm_file:"data_files/dhparams.pem":"9e35f430443a09904f3a39a979797d070df53378e79c2438bef4e761f3c714553328589b041c809be1d6c6b5f1fc9f47d3a25443188253a992a56818b37ba9de5a40d362e56eff0be5417474c125c199272c8fe41dea733df6f662c92ae76556e755d10c64e6a50968f67fc6ea73d0dca8569be2ba204e23580d8bca2f4975b3":"02":128
diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function
index d7cabf464..f21e6e6c1 100644
--- a/tests/suites/test_suite_dhm.function
+++ b/tests/suites/test_suite_dhm.function
@@ -9,7 +9,7 @@
 
 /* BEGIN_CASE */
 void dhm_do_dhm( int radix_P, char *input_P,
-                 int radix_G, char *input_G )
+                 int radix_G, char *input_G, int result )
 {
     dhm_context ctx_srv;
     dhm_context ctx_cli;
@@ -44,7 +44,10 @@ void dhm_do_dhm( int radix_P, char *input_P,
     /*
      * First key exchange
      */
-    TEST_ASSERT( dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == 0 );
+    TEST_ASSERT( dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == result );
+    if ( result != 0 )
+        goto exit;
+
     ske[ske_len++] = 0;
     ske[ske_len++] = 0;
     TEST_ASSERT( dhm_read_params( &ctx_cli, &p, ske + ske_len ) == 0 );

From 77359c93e4646bfa8f3c2dc0700574d6b6936f28 Mon Sep 17 00:00:00 2001
From: Janos Follath <janos.follath@arm.com>
Date: Wed, 20 Sep 2017 15:33:24 +0100
Subject: [PATCH 2/2] DHM: Fix dhm_check_range() always returning 0

Although the variable ret was initialised to an error, the
MBEDTLS_MPI_CHK macro was overwriting it. Therefore it ended up being
0 whenewer the bignum computation was successfull and stayed 0
independently of the actual check.
---
 ChangeLog     |  6 ++++++
 library/dhm.c | 11 +++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a3171d7eb..15e62149c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 mbed TLS ChangeLog (Sorted per branch, date)
 
+= mbed TLS 1.3.x branch released xxxx-xx-xx
+
+Security
+   * Fix dhm_check_range() failing to detect trivial subgroups and potentially
+     leaking 1 bit of the private key. Reported by prashantkspatil.
+
 = mbed TLS 1.3.21 branch released 2017-08-10
 
 Security
diff --git a/library/dhm.c b/library/dhm.c
index 48fba2a73..6f1c51cc0 100644
--- a/library/dhm.c
+++ b/library/dhm.c
@@ -91,6 +91,9 @@ static int dhm_read_bignum( mpi *X,
  *
  * Parameter should be: 2 <= public_param <= P - 2
  *
+ * This means that we need to return an error if
+ *              public_param < 2 or public_param > P-2
+ *
  * For more information on the attack, see:
  *  http://www.cl.cam.ac.uk/~rja14/Papers/psandqs.pdf
  *  http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2005-2643
@@ -98,17 +101,17 @@ static int dhm_read_bignum( mpi *X,
 static int dhm_check_range( const mpi *param, const mpi *P )
 {
     mpi L, U;
-    int ret = POLARSSL_ERR_DHM_BAD_INPUT_DATA;
+    int ret = 0;
 
     mpi_init( &L ); mpi_init( &U );
 
     MPI_CHK( mpi_lset( &L, 2 ) );
     MPI_CHK( mpi_sub_int( &U, P, 2 ) );
 
-    if( mpi_cmp_mpi( param, &L ) >= 0 &&
-        mpi_cmp_mpi( param, &U ) <= 0 )
+    if( mpi_cmp_mpi( param, &L ) < 0 ||
+        mpi_cmp_mpi( param, &U ) > 0 )
     {
-        ret = 0;
+        ret = POLARSSL_ERR_DHM_BAD_INPUT_DATA;
     }
 
 cleanup: