Browse code

Use SHA256 for the internal digest, instead of MD5

Our internal options digest uses MD5 hashes to store the state, instead of
storing the full options string. There's nothing wrong with that, but it
would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine too

The slightly less bytes for the digest (16 instead of 32) and operations
per connection setup are not worth sticking to MD5.

Note that might SHA256 not be available in de crypto lib, OpenVPN will
refuse to start and shout "Message hash algorithm 'SHA256' not found".

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <1485101081-9784-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13926.html
Signed-off-by: David Sommerseth <davids@openvpn.net>

Steffan Karger authored on 2017/01/23 01:04:41
Showing 6 changed files
... ...
@@ -131,9 +131,9 @@
131 131
 #include "packet_id.h"
132 132
 #include "mtu.h"
133 133
 
134
-/** Wrapper struct to pass around MD5 digests */
135
-struct md5_digest {
136
-    uint8_t digest[MD5_DIGEST_LENGTH];
134
+/** Wrapper struct to pass around SHA256 digests */
135
+struct sha256_digest {
136
+    uint8_t digest[SHA256_DIGEST_LENGTH];
137 137
 };
138 138
 
139 139
 /*
... ...
@@ -73,6 +73,7 @@ typedef mbedtls_md_context_t hmac_ctx_t;
73 73
 #define MD4_DIGEST_LENGTH       16
74 74
 #define MD5_DIGEST_LENGTH       16
75 75
 #define SHA_DIGEST_LENGTH       20
76
+#define SHA256_DIGEST_LENGTH    32
76 77
 #define DES_KEY_LENGTH 8
77 78
 
78 79
 /**
... ...
@@ -33,6 +33,7 @@
33 33
 #include <openssl/evp.h>
34 34
 #include <openssl/hmac.h>
35 35
 #include <openssl/md5.h>
36
+#include <openssl/sha.h>
36 37
 
37 38
 /** Generic cipher key type %context. */
38 39
 typedef EVP_CIPHER cipher_kt_t;
... ...
@@ -1919,12 +1919,12 @@ tun_abort()
1919 1919
  * equal, or either one is all-zeroes.
1920 1920
  */
1921 1921
 static bool
1922
-options_hash_changed_or_zero(const struct md5_digest *a,
1923
-                             const struct md5_digest *b)
1922
+options_hash_changed_or_zero(const struct sha256_digest *a,
1923
+                             const struct sha256_digest *b)
1924 1924
 {
1925
-    const struct md5_digest zero = {{0}};
1926
-    return memcmp(a, b, sizeof(struct md5_digest))
1927
-           || !memcmp(a, &zero, sizeof(struct md5_digest));
1925
+    const struct sha256_digest zero = {{0}};
1926
+    return memcmp(a, b, sizeof(struct sha256_digest))
1927
+           || !memcmp(a, &zero, sizeof(struct sha256_digest));
1928 1928
 }
1929 1929
 #endif /* P2MP */
1930 1930
 
... ...
@@ -202,7 +202,7 @@ struct context_1
202 202
 #endif
203 203
 
204 204
     /* if client mode, hash of option strings we pulled from server */
205
-    struct md5_digest pulled_options_digest_save;
205
+    struct sha256_digest pulled_options_digest_save;
206 206
     /**< Hash of option strings received from the
207 207
      *   remote OpenVPN server.  Only used in
208 208
      *   client-mode. */
... ...
@@ -471,9 +471,9 @@ struct context_2
471 471
     bool did_pre_pull_restore;
472 472
 
473 473
     /* hash of pulled options, so we can compare when options change */
474
-    bool pulled_options_md5_init_done;
474
+    bool pulled_options_digest_init_done;
475 475
     md_ctx_t pulled_options_state;
476
-    struct md5_digest pulled_options_digest;
476
+    struct sha256_digest pulled_options_digest;
477 477
 
478 478
     struct event_timeout scheduled_exit;
479 479
     int scheduled_exit_signal;
... ...
@@ -720,10 +720,10 @@ process_incoming_push_msg(struct context *c,
720 720
         if (ch == ',')
721 721
         {
722 722
             struct buffer buf_orig = buf;
723
-            if (!c->c2.pulled_options_md5_init_done)
723
+            if (!c->c2.pulled_options_digest_init_done)
724 724
             {
725
-                md_ctx_init(&c->c2.pulled_options_state, md_kt_get("MD5"));
726
-                c->c2.pulled_options_md5_init_done = true;
725
+                md_ctx_init(&c->c2.pulled_options_state, md_kt_get("SHA256"));
726
+                c->c2.pulled_options_digest_init_done = true;
727 727
             }
728 728
             if (!c->c2.did_pre_pull_restore)
729 729
             {
... ...
@@ -744,7 +744,7 @@ process_incoming_push_msg(struct context *c,
744 744
                     case 1:
745 745
                         md_ctx_final(&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
746 746
                         md_ctx_cleanup(&c->c2.pulled_options_state);
747
-                        c->c2.pulled_options_md5_init_done = false;
747
+                        c->c2.pulled_options_digest_init_done = false;
748 748
                         ret = PUSH_MSG_REPLY;
749 749
                         break;
750 750