[Openvpn-devel,v2] rewrite parse_hash_fingerprint()

Message ID 20210427110300.6911-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] rewrite parse_hash_fingerprint()
Related show

Commit Message

Gert Doering April 27, 2021, 11:03 a.m.
The existing code was doing far too much work for too little
gain - copying the string segment for scanf(), checking extra
for spaces, making the result quite unreadable.

Verify each segment with (short-circuited) isxdigit() checks,
then feed directly to scanf(), which will stop parsing on ':'
or end-of-string.

Rewrite error message to differentiate "hash too short" (including
number of bytes read) and "hash too long" (it did not terminate when
we had enough bytes).

While at it, add an option printer for the resulting o->verify_hash
list to show_settings().

v2:
   fix typo in commit message
   appease whitespace dragon
   add printing of verify_hash_algo and verify_hash_depth
   print correct hash length for SHA1 certs
   fix incorrect assignment to options->verify_hash_algo in c3a7065d5

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/options.c | 69 ++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

Comments

Antonio Quartulli April 27, 2021, 2:40 p.m. | #1
Hi,

On 27/04/2021 13:03, Gert Doering wrote:
> The existing code was doing far too much work for too little
> gain - copying the string segment for scanf(), checking extra
> for spaces, making the result quite unreadable.
> 
> Verify each segment with (short-circuited) isxdigit() checks,
> then feed directly to scanf(), which will stop parsing on ':'
> or end-of-string.
> 
> Rewrite error message to differentiate "hash too short" (including
> number of bytes read) and "hash too long" (it did not terminate when
> we had enough bytes).
> 
> While at it, add an option printer for the resulting o->verify_hash
> list to show_settings().
> 
> v2:
>    fix typo in commit message
>    appease whitespace dragon
>    add printing of verify_hash_algo and verify_hash_depth
>    print correct hash length for SHA1 certs
>    fix incorrect assignment to options->verify_hash_algo in c3a7065d5
> 

Thanks for the changes :)

> Signed-off-by: Gert Doering <gert@greenie.muc.de>

Patch looks good and does what it says. Couldn't break it in any way.

Compile tests passed.

Basic tests on various broken/non-broken hex strings passed too.

Acked-by: Antonio Quartulli <antonio@openvpn.net>


Regards,
Gert Doering April 27, 2021, 3:06 p.m. | #2
Patch has been applied to the master branch.

.. and I still have no test setup for this... "next FOOM!"...

commit 925f0180318033f9ea7885b40b4b8200b35abbca (master)
Author: Gert Doering
Date:   Tue Apr 27 13:03:00 2021 +0200

     rewrite parse_hash_fingerprint()

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210427110300.6911-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22241.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2a5b1393..795a8fc7 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1082,56 +1082,43 @@  string_substitute(const char *src, int from, int to, struct gc_arena *gc)
 static struct verify_hash_list *
 parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_arena *gc)
 {
-    int i;
+    int i = 0;
     const char *cp = str;
 
     struct verify_hash_list *ret;
     ALLOC_OBJ_CLEAR_GC(ret, struct verify_hash_list, gc);
 
-    char term = 1;
+    char term = 0;
     int byte;
-    char bs[3];
 
-    for (i = 0; i < nbytes; ++i)
+    while (*cp && i < nbytes)
     {
-        if (strlen(cp) < 2)
+        /* valid segments consist of exactly two hex digits, then ':' or EOS */
+        if (!isxdigit(cp[0])
+            || !isxdigit(cp[1])
+            || (cp[2] != ':' && cp[2] != '\0')
+            || sscanf(cp, "%x", &byte) != 1)
         {
             msg(msglevel, "format error in hash fingerprint: %s", str);
+            break;
         }
-        bs[0] = *cp++;
-        bs[1] = *cp++;
-        bs[2] = 0;
 
-        /* the format string "%x" passed to sscanf will ignore any space and
-         * will still try to parse the other character. However, this is not
-         * expected format for a fingerprint, therefore explictly check for
-         * blanks in the string and error out if any is found
-         */
-        if (bs[0] == ' ' || bs[1] == ' ')
-        {
-            msg(msglevel, "format error in hash fingerprint unexpected blank: %s",
-                str);
-        }
+        ret->hash[i++] = (uint8_t)byte;
 
-        byte = 0;
-        if (sscanf(bs, "%x", &byte) != 1)
-        {
-            msg(msglevel, "format error in hash fingerprint hex byte: %s", str);
-        }
-        ret->hash[i] = (uint8_t)byte;
-        term = *cp++;
-        if (term != ':' && term != 0)
-        {
-            msg(msglevel, "format error in hash fingerprint delimiter: %s", str);
-        }
-        if (term == 0)
+        term = cp[2];
+        if (term == '\0')
         {
             break;
         }
+        cp += 3;
     }
-    if (term != 0 || i != nbytes-1)
+    if (i < nbytes)
     {
-        msg(msglevel, "hash fingerprint is different length than expected (%d bytes): %s", nbytes, str);
+        msg(msglevel, "hash fingerprint is wrong length - expected %d bytes, got %d: %s", nbytes, i, str);
+    }
+    else if (term != '\0')
+    {
+        msg(msglevel, "hash fingerprint too long - expected only %d bytes: %s", nbytes, str);
     }
     return ret;
 }
@@ -1791,6 +1778,23 @@  show_settings(const struct options *o)
         }
     }
     SHOW_STR(remote_cert_eku);
+    if (o->verify_hash)
+    {
+        SHOW_INT(verify_hash_algo);
+        SHOW_INT(verify_hash_depth);
+        struct gc_arena gc = gc_new();
+        struct verify_hash_list *hl = o->verify_hash;
+        int digest_len = (o->verify_hash_algo == MD_SHA1) ? SHA_DIGEST_LENGTH :
+                         SHA256_DIGEST_LENGTH;
+        while (hl)
+        {
+            char *s = format_hex_ex(hl->hash, digest_len, 0,
+                                    1, ":", &gc);
+            SHOW_PARM(verify_hash, s, "%s");
+            hl = hl->next;
+        }
+        gc_free(&gc);
+    }
     SHOW_INT(ssl_flags);
 
     SHOW_INT(tls_timeout);
@@ -8190,7 +8194,6 @@  add_option(struct options *options,
             if ((!p[2] && !is_inline) || (p[2] && streq(p[2], "SHA1")))
             {
                 options->verify_hash_algo = MD_SHA1;
-                options->verify_hash_algo = SHA_DIGEST_LENGTH;
                 digest_len = SHA_DIGEST_LENGTH;
             }
             else if (p[2] && !streq(p[2], "SHA256"))