Enhanced integer parsing in OSSL_PARAM_allocate_from_text

Fixes #14041 and additional bugs discovered by the newly created
tests.

This patch:
 - Introduces support for 0x prefixed integers
 - Fixes parsing of negative integers (negative numbers were
   shifted by -2)
 - Fixes ability to parse maximal unsigned numbers ("too small
   buffer" error used to be reported incorrectly)
 - Fixes a memory leak when OSSL_PARAM_allocate_from_text fails
   leaving a temporary BN allocated

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14093)

1 files changed, 15 insertions, 6 deletions

diff --git a/crypto/params_from_text.c b/crypto/params_from_text.c
index ddc3c38aa4..b019744f9b 100644
--- a/crypto/params_from_text.c
+++ b/crypto/params_from_text.c
@@ -28,6 +28,7 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key,
                              size_t *buf_n, BIGNUM **tmpbn, int *found)
 {
     const OSSL_PARAM *p;
+    size_t buf_bits;
 
     /*
      * ishex is used to translate legacy style string controls in hex format
@@ -50,7 +51,7 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key,
         if (*ishex)
             BN_hex2bn(tmpbn, value);
         else
-            BN_dec2bn(tmpbn, value);
+            BN_asc2bn(tmpbn, value);
 
         if (*tmpbn == NULL)
             return 0;
@@ -62,20 +63,25 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key,
          * buffer, i.e. if it's negative, we need to deal with it.  We do
          * it by subtracting 1 here and inverting the bytes in
          * construct_from_text() below.
+         * To subtract 1 from an absolute value of a negative number we
+         * actually have to add 1: -3 - 1 = -4, |-3| = 3 + 1 = 4.
          */
         if (p->data_type == OSSL_PARAM_INTEGER && BN_is_negative(*tmpbn)
-            && !BN_sub_word(*tmpbn, 1)) {
+            && !BN_add_word(*tmpbn, 1)) {
             return 0;
         }
 
-        *buf_n = BN_num_bytes(*tmpbn);
+        buf_bits = (size_t)BN_num_bits(*tmpbn);
+        *buf_n = (buf_bits + 7) / 8;
 
         /*
          * TODO(v3.0) is this the right way to do this?  This code expects
          * a zero data size to simply mean "arbitrary size".
          */
         if (p->data_size > 0) {
-            if (*buf_n >= p->data_size) {
+            if (buf_bits > p->data_size * 8
+                || (p->data_type == OSSL_PARAM_INTEGER
+                    && buf_bits == p->data_size * 8)) {
                 ERR_raise(ERR_LIB_CRYPTO, CRYPTO_R_TOO_SMALL_BUFFER);
                 /* Since this is a different error, we don't break */
                 return 0;
@@ -184,11 +190,11 @@ int OSSL_PARAM_allocate_from_text(OSSL_PARAM *to,
 
     if (!prepare_from_text(paramdefs, key, value, value_n,
                            &paramdef, &ishex, &buf_n, &tmpbn, found))
-        return 0;
+        goto err;
 
     if ((buf = OPENSSL_zalloc(buf_n > 0 ? buf_n : 1)) == NULL) {
         ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
-        return 0;
+        goto err;
     }
 
     ok = construct_from_text(to, paramdef, value, value_n, ishex,
@@ -197,4 +203,7 @@ int OSSL_PARAM_allocate_from_text(OSSL_PARAM *to,
     if (!ok)
         OPENSSL_free(buf);
     return ok;
+ err:
+    BN_free(tmpbn);
+    return 0;
 }