mpi: Explicitly limit the allowed input length for gcry_mpi_scan.

* mpi/mpicoder.c (MAX_EXTERN_SCAN_BYTES): New.
(mpi_fromstr): Check against this limit.
(_gcry_mpi_scan): Ditto.
* tests/mpitests.c (test_maxsize): New.
(main): Cal that test.
--

A too large buffer length may lead to an unsigned integer overflow on
systems where size_t > unsigned int (ie. 64 bit systems).  The
computation of the required number of nlimbs may also be affected by
this.  However this is not a real world case because any processing
which has allocated such a long buffer from an external source would
be prone to other DoS attacks: The required buffer length to exhibit
this overflow is at least 2^32 - 8 bytes.

Signed-off-by: Werner Koch <wk@gnupg.org>

3 files changed, 70 insertions, 5 deletions

diff --git a/doc/gcrypt.texi b/doc/gcrypt.texi
index 437dddb7..c710765b 100644
--- a/doc/gcrypt.texi
+++ b/doc/gcrypt.texi
@@ -4526,7 +4526,8 @@ representation of an MPI and the internal one of Libgcrypt.
 Convert the external representation of an integer stored in @var{buffer}
 with a length of @var{buflen} into a newly created MPI returned which
 will be stored at the address of @var{r_mpi}.  For certain formats the
-length argument is not required and should be passed as @code{0}.  After a
+length argument is not required and should be passed as @code{0}. A
+@var{buflen} larger than 16 MiByte will be rejected.  After a
 successful operation the variable @var{nscanned} receives the number of
 bytes actually scanned unless @var{nscanned} was given as
 @code{NULL}. @var{format} describes the format of the MPI as stored in
@@ -4540,6 +4541,7 @@ bytes actually scanned unless @var{nscanned} was given as
 @item GCRYMPI_FMT_PGP
 As used by OpenPGP (only defined as unsigned). This is basically
 @code{GCRYMPI_FMT_STD} with a 2 byte big endian length header.
+A length header indicating a length of more than 16384 is not allowed.
 
 @item GCRYMPI_FMT_SSH
 As used in the Secure Shell protocol.  This is @code{GCRYMPI_FMT_STD}
diff --git a/mpi/mpicoder.c b/mpi/mpicoder.c
index 896dda14..e3155766 100644
--- a/mpi/mpicoder.c
+++ b/mpi/mpicoder.c
@@ -27,8 +27,21 @@
 #include "mpi-internal.h"
 #include "g10lib.h"
 
+/* The maximum length we support in the functions converting an
+ * external representation to an MPI.  This limit is used to catch
+ * programming errors and to avoid DoS due to insane long allocations.
+ * The 16 MiB limit is actually ridiculous large but some of those PQC
+ * algorithms use quite large keys and they might end up using MPIs
+ * for that.  */
+#define MAX_EXTERN_SCAN_BYTES (16*1024*1024)
+
+/* The maximum length (in bits) we support for OpenPGP MPIs.  Note
+ * that OpenPGP's MPI format uses only two bytes and thus would be
+ * limited to 64k anyway.  Note that this limit matches that used by
+ * GnuPG.  */
 #define MAX_EXTERN_MPI_BITS 16384
 
+
 /* Helper used to scan PGP style MPIs.  Returns NULL on failure. */
 static gcry_mpi_t
 mpi_read_from_buffer (const unsigned char *buffer, unsigned *ret_nread,
@@ -104,7 +117,13 @@ mpi_fromstr (gcry_mpi_t val, const char *str)
   if ( *str == '0' && str[1] == 'x' )
     str += 2;
 
-  nbits = 4 * strlen (str);
+  nbits = strlen (str);
+  if (nbits > MAX_EXTERN_SCAN_BYTES)
+    {
+      mpi_clear (val);
+      return 1;  /* Error.  */
+    }
+  nbits *= 4;
   if ((nbits % 8))
     prepend_zero = 1;
 
@@ -438,9 +457,9 @@ twocompl (unsigned char *p, unsigned int n)
 
 
 /* Convert the external representation of an integer stored in BUFFER
-   with a length of BUFLEN into a newly create MPI returned in
-   RET_MPI.  If NBYTES is not NULL, it will receive the number of
-   bytes actually scanned after a successful operation.  */
+ * with a length of BUFLEN into a newly create MPI returned in
+ * RET_MPI.  If NSCANNED is not NULL, it will receive the number of
+ * bytes actually scanned after a successful operation.  */
 gcry_err_code_t
 _gcry_mpi_scan (struct gcry_mpi **ret_mpi, enum gcry_mpi_format format,
                 const void *buffer_arg, size_t buflen, size_t *nscanned)
@@ -450,6 +469,13 @@ _gcry_mpi_scan (struct gcry_mpi **ret_mpi, enum gcry_mpi_format format,
   unsigned int len;
   int secure = (buffer && _gcry_is_secure (buffer));
 
+  if (buflen > MAX_EXTERN_SCAN_BYTES)
+    {
+      if (nscanned)
+        *nscanned = 0;
+      return GPG_ERR_INV_OBJ;
+    }
+
   if (format == GCRYMPI_FMT_SSH)
     len = 0;
   else
diff --git a/tests/mpitests.c b/tests/mpitests.c
index e6f8525c..b6630291 100644
--- a/tests/mpitests.c
+++ b/tests/mpitests.c
@@ -237,6 +237,42 @@ test_opaque (void)
 
 
 static void
+test_maxsize (void)
+{
+  gpg_error_t err;
+  gcry_mpi_t a;
+  char buffer[2+2048]; /* For PGP: 2 length bytes and 16384 bits.  */
+
+  memset (buffer, 0x55, sizeof buffer);
+
+  /* We use a short buffer but a give a too large length to simulate a
+   * programming error.  In case this test fails (i.e. die() is
+   * called) the scan function may have access data outside of BUFFER
+   * which may result in a segv but we ignore that to avoid actually
+   * allocating such a long buffer.  */
+  err = gcry_mpi_scan (&a, GCRYMPI_FMT_USG, buffer, 16*1024*1024 +1, NULL);
+  if (gpg_err_code (err) != GPG_ERR_INV_OBJ)
+    die ("gcry_mpi_scan does not detect its generic input limit\n");
+
+  /* Now test the PGP limit.  The scan code check the two length bytes
+   * from the buffer and thus it is sufficient to fake them.  */
+  buffer[0] = (16385 >> 8);
+  buffer[1] = (16385 & 0xff);
+  err = gcry_mpi_scan (&a, GCRYMPI_FMT_PGP, buffer, sizeof buffer, NULL);
+  if (gpg_err_code (err) != GPG_ERR_INV_OBJ)
+    die ("gcry_mpi_scan does not detect the PGP input limit\n");
+
+  buffer[0] = (16384 >> 8);
+  buffer[1] = (16384 & 0xff);
+
+  err = gcry_mpi_scan (&a, GCRYMPI_FMT_PGP, buffer, sizeof buffer, NULL);
+  if (err)
+    die ("gcry_mpi_scan did not parse a large PGP: %s\n", gpg_strerror (err));
+  gcry_mpi_release (a);
+}
+
+
+static void
 test_cmp (void)
 {
   gpg_error_t rc;
@@ -569,6 +605,7 @@ main (int argc, char* argv[])
 
   test_const_and_immutable ();
   test_opaque ();
+  test_maxsize ();
   test_cmp ();
   test_add ();
   test_sub ();