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)

diff --git a/crypto/params_from_text.c b/crypto/params_from_text.c
index ddc3c38aa42ef513c339c2fd7ac3849bab1d1033..b019744f9b71afec614cb9af46a3c00c95c7f628 100644 (file)
--- 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;
 }

diff --git a/doc/man3/OSSL_PARAM_allocate_from_text.pod b/doc/man3/OSSL_PARAM_allocate_from_text.pod
index ef68f0e10caa28eed7fba35fe69c4d19cde94840..80ba555a8feac6fd434a98a2c9f6657470097057 100644 (file)
--- a/doc/man3/OSSL_PARAM_allocate_from_text.pod
+++ b/doc/man3/OSSL_PARAM_allocate_from_text.pod
@@ -55,15 +55,16 @@ depending on that item's I<data_type>, as follows:
 
 =item B<OSSL_PARAM_INTEGER> and B<OSSL_PARAM_UNSIGNED_INTEGER>
 
-If I<key> started with "hex", I<value> is assumed to contain
-I<value_n> hexadecimal characters, which are decoded, and the
-resulting bytes become the number stored in the I<< to->data >>
-storage.
-
 If I<key> didn't start with "hex", I<value> is assumed to contain
 I<value_n> decimal characters, which are decoded, and the resulting
 bytes become the number stored in the I<< to->data >> storage.
 
+If I<value> starts with "0x", it is assumed to contain I<value_n>
+hexadecimal characters.
+
+If I<key> started with "hex", I<value> is assumed to contain
+I<value_n> hexadecimal characters without the "0x" prefix.
+
 If I<value> contains characters that couldn't be decoded as
 hexadecimal or decimal characters, OSSL_PARAM_allocate_from_text()
 considers that an error.

diff --git a/test/params_test.c b/test/params_test.c
index 8ee2e1594c06bf97f6dd4124f398b2b99ca540b9..913df9eb8ab78008ff4462418e473fd590721be2 100644 (file)
--- a/test/params_test.c
+++ b/test/params_test.c
@@ -541,8 +541,81 @@ static int test_case(int i)
                                  test_cases[i].prov));
 }
 
+/*-
+ * OSSL_PARAM_allocate_from_text() tests
+ * =====================================
+ */
+
+static const OSSL_PARAM params_from_text[] = {
+    OSSL_PARAM_int32("int", NULL),
+    OSSL_PARAM_DEFN("short", OSSL_PARAM_INTEGER, NULL, sizeof(int16_t)),
+    OSSL_PARAM_DEFN("ushort", OSSL_PARAM_UNSIGNED_INTEGER, NULL, sizeof(uint16_t)),
+    OSSL_PARAM_END,
+};
+
+struct int_from_text_test_st {
+    const char *argname;
+    const char *strval;
+    long int intval;
+    int res;
+};
+
+static struct int_from_text_test_st int_from_text_test_cases[] = {
+    { "int",               "",          0, 0 },
+    { "int",              "0",          0, 1 },
+    { "int",            "101",        101, 1 },
+    { "int",           "-102",       -102, 1 },
+    { "int",            "12A",         12, 1 }, /* incomplete */
+    { "int",          "0x12B",      0x12B, 1 },
+    { "hexint",         "12C",      0x12C, 1 },
+    { "hexint",       "0x12D",          0, 1 }, /* zero */
+    /* test check of the target buffer size */
+    { "int",     "0x7fffffff",  INT32_MAX, 1 },
+    { "int",     "2147483647",  INT32_MAX, 1 },
+    { "int",     "2147483648",          0, 0 }, /* too small buffer */
+    { "int",    "-2147483648",  INT32_MIN, 1 },
+    { "int",    "-2147483649",          0, 0 }, /* too small buffer */
+    { "short",       "0x7fff",  INT16_MAX, 1 },
+    { "short",        "32767",  INT16_MAX, 1 },
+    { "short",        "32768",          0, 0 }, /* too small buffer */
+    { "ushort",      "0xffff", UINT16_MAX, 1 },
+    { "ushort",       "65535", UINT16_MAX, 1 },
+    { "ushort",       "65536",          0, 0 }, /* too small buffer */
+};
+
+static int check_int_from_text(const struct int_from_text_test_st a)
+{
+    OSSL_PARAM param;
+    long int val = 0;
+    int res;
+
+    if (!OSSL_PARAM_allocate_from_text(&param, params_from_text,
+                                       a.argname, a.strval, 0, NULL)) {
+        if (a.res)
+            TEST_error("errant %s param \"%s\"", a.argname, a.strval);
+        return !a.res;
+    }
+
+    res = OSSL_PARAM_get_long(&param, &val);
+    OPENSSL_free(param.data);
+
+    if (res ^ a.res || val != a.intval) {
+        TEST_error("errant %s \"%s\" %li != %li",
+                   a.argname, a.strval, a.intval, val);
+        return 0;
+    }
+
+    return a.res;
+}
+
+static int test_allocate_from_text(int i)
+{
+    return check_int_from_text(int_from_text_test_cases[i]);
+}
+
 int setup_tests(void)
 {
     ADD_ALL_TESTS(test_case, OSSL_NELEM(test_cases));
+    ADD_ALL_TESTS(test_allocate_from_text, OSSL_NELEM(int_from_text_test_cases));
     return 1;
 }