[Openvpn-devel] [M] Change in openvpn[master]: Replace SHA256 with SIPHASH24 in HMAC cookie approach
"plaisthos \(Code Review\) via Openvpn-devel" <[email protected]>
| Newsgroups | net.sourceforge.lists.openvpn-devel |
|---|---|
| Message-ID | <0ca8bd313bbebb84d74a9580c12f72a8ef907305-EmailReplacePatchSet-HTML@gerrit.openvpn.net> |
Attention is currently required from: flichtenheld.
Hello flichtenheld,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1827?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Code-Review-1 by flichtenheld
Change subject: Replace SHA256 with SIPHASH24 in HMAC cookie approach
......................................................................
Replace SHA256 with SIPHASH24 in HMAC cookie approach
Using SHA256 for this is overkill since we only need a 64bit hash
value that is not predictable. Siphash24 also fulfils these
requirements while being much faster.
Change-Id: I3b6bb178ffb2bb49981bc23eabf04fe8e06d6fc3
---
M CMakeLists.txt
M src/openvpn/init.c
M src/openvpn/mudp.c
M src/openvpn/openvpn.h
M src/openvpn/siphash.h
M src/openvpn/ssl_pkt.c
M src/openvpn/ssl_pkt.h
M tests/unit_tests/openvpn/Makefile.am
M tests/unit_tests/openvpn/test_pkt.c
9 files changed, 79 insertions(+), 102 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/27/1827/5
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9e1dde1..29b53a9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -875,6 +875,7 @@
src/openvpn/packet_id.c
src/openvpn/reliable.c
src/openvpn/run_command.c
+ src/openvpn/siphash_reference.c
src/openvpn/session_id.c
src/openvpn/ssl_pkt.c
src/openvpn/tls_crypt.c
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index db753dc..9b6ad7b 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3473,7 +3473,7 @@
if (flags & CF_INIT_TLS_AUTH_STANDALONE)
{
c->c2.tls_auth_standalone = tls_auth_standalone_init(&to, &c->c2.gc);
- c->c2.session_id_hmac = session_id_hmac_init();
+ siphash_key_init(c->c2.session_id_key);
}
}
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 9acf297..b64cdd7 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -87,7 +87,7 @@
verdict = tls_pre_decrypt_lite(tas, state, &m->top.c2.from, &m->top.c2.buf);
- hmac_ctx_t *hmac = m->top.c2.session_id_hmac;
+ uint8_t *hmac_key = m->top.c2.session_id_key;
struct openvpn_sockaddr *from = &m->top.c2.from.dest;
int handwindow = m->top.options.handshake_window;
@@ -119,7 +119,7 @@
{
/* Calculate the session ID HMAC for our reply and create reset packet */
struct session_id sid =
- calculate_session_id_hmac(state->peer_session_id, from, hmac, handwindow, 0);
+ calculate_session_id_hmac(state->peer_session_id, from, hmac_key, handwindow, 0);
send_hmac_reset_packet(m, state, tas, &sid, true, sock);
return false;
@@ -151,7 +151,7 @@
{
/* Calculate the session ID HMAC for our reply and create reset packet */
struct session_id sid =
- calculate_session_id_hmac(state->peer_session_id, from, hmac, handwindow, 0);
+ calculate_session_id_hmac(state->peer_session_id, from, hmac_key, handwindow, 0);
send_hmac_reset_packet(m, state, tas, &sid, false, sock);
@@ -166,7 +166,7 @@
struct gc_arena gc = gc_new();
bool pkt_is_ack = (verdict == VERDICT_VALID_ACK_V1);
- bool ret = check_session_hmac_and_pkt_id(state, from, hmac, handwindow, pkt_is_ack);
+ bool ret = check_session_hmac_and_pkt_id(state, from, hmac_key, handwindow, pkt_is_ack);
const char *peer = print_link_socket_actual(&m->top.c2.from, &gc);
uint8_t pkt_firstbyte = *BPTR(&m->top.c2.buf);
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index fa00822..e9e18bf 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -45,6 +45,7 @@
#include "plugin.h"
#include "manage.h"
#include "dns.h"
+#include "siphash.h"
/*
* Our global key schedules, packaged thusly
@@ -335,10 +336,9 @@
* \c --tls-auth commandline option. */
- hmac_ctx_t *session_id_hmac;
- /**< the HMAC we use to generate and verify our syn cookie like
- * session ids from the server.
- */
+ uint8_t session_id_key[SIPHASH_KEY_SIZE];
+ /**< the siphash secret we use to generate and verify our syn cookie like
+ * session ids from the server. */
/* used to optimize calls to tls_multi_process */
struct interval tmp_int;
diff --git a/src/openvpn/siphash.h b/src/openvpn/siphash.h
index ade7762..462175c 100644
--- a/src/openvpn/siphash.h
+++ b/src/openvpn/siphash.h
@@ -24,7 +24,7 @@
#include <stdint.h>
#include <stdio.h>
-#include <stdbool.h>
+#include "crypto.h"
/* We need to include this to check for the OPENSSL_IS_AWSLC macro */
#ifdef ENABLE_CRYPTO_OPENSSL
@@ -77,4 +77,14 @@
#endif
}
+/**
+ * Initialises a SIPHASH key with a random value
+ * @param key the key to be initialised
+ */
+static inline void
+siphash_key_init(uint8_t *key)
+{
+ prng_bytes(key, SIPHASH_KEY_SIZE);
+}
+
#endif /* ifndef SIPHASH_H */
diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index f8444451..6a6d9f9 100644
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -31,6 +31,7 @@
#include "crypto.h"
#include "session_id.h"
#include "reliable.h"
+#include "siphash.h"
#include "tls_crypt.h"
/*
@@ -442,64 +443,53 @@
return buf;
}
-hmac_ctx_t *
-session_id_hmac_init(void)
-{
- /* We assume that SHA256 is always available */
- ASSERT(md_valid("SHA256"));
- hmac_ctx_t *hmac_ctx = hmac_ctx_new();
-
- uint8_t key[SHA256_DIGEST_LENGTH];
- ASSERT(rand_bytes(key, sizeof(key)));
-
- hmac_ctx_init(hmac_ctx, key, "SHA256");
- return hmac_ctx;
-}
-
struct session_id
calculate_session_id_hmac(struct session_id client_sid, const struct openvpn_sockaddr *from,
- hmac_ctx_t *hmac, int handwindow, int offset)
+ const uint8_t *key, int handwindow, int offset)
{
- union
- {
- uint8_t hmac_result[SHA256_DIGEST_LENGTH];
- struct session_id sid;
- } result;
-
/* Get the valid time quantisation for our hmac,
* we divide time by handwindow/2 and allow the previous
* and future session time if specified by offset */
uint32_t session_id_time = ntohl((uint32_t)(now / ((handwindow + 1) / 2) + offset));
- hmac_ctx_reset(hmac);
+ uint8_t input[64];
+
+ /* ensure input array is large enough */
+ static_assert(sizeof(input) >= sizeof(struct sockaddr_in6) + sizeof(session_id_time) + sizeof(client_sid.id), "input buffer not sized correctly");
+ static_assert(sizeof(input) >= sizeof(struct sockaddr_in) + sizeof(session_id_time) + sizeof(client_sid.id), "input buffer not sized correctly");
+
+ struct buffer in = { 0 };
+ buf_set_write(&in, input, sizeof(input));
+
/* We do not care about endian here since it does not need to be
* portable */
- hmac_ctx_update(hmac, (const uint8_t *)&session_id_time, sizeof(session_id_time));
+ buf_write(&in, (const uint8_t *)&session_id_time, sizeof(session_id_time));
/* add client IP and port */
switch (from->addr.sa.sa_family)
{
case AF_INET:
- hmac_ctx_update(hmac, (const uint8_t *)&from->addr.in4, sizeof(struct sockaddr_in));
+ buf_write(&in, (const uint8_t *)&from->addr.in4, sizeof(struct sockaddr_in));
break;
case AF_INET6:
- hmac_ctx_update(hmac, (const uint8_t *)&from->addr.in6, sizeof(struct sockaddr_in6));
+ buf_write(&in, (const uint8_t *)&from->addr.in6, sizeof(struct sockaddr_in6));
break;
}
/* add session id of client */
- hmac_ctx_update(hmac, client_sid.id, SID_SIZE);
+ buf_write(&in, client_sid.id, SID_SIZE);
- hmac_ctx_final(hmac, result.hmac_result);
+ struct session_id sid;
+ siphash(buf_bptr(&in), buf_len(&in), key, sid.id, sizeof(sid.id));
- return result.sid;
+ return sid;
}
bool
check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state,
const struct openvpn_sockaddr *from,
- hmac_ctx_t *hmac,
+ uint8_t *key,
int handwindow,
bool pkt_is_ack)
{
@@ -551,7 +541,7 @@
for (int offset = -2; offset <= 0; offset++)
{
struct session_id expected_id =
- calculate_session_id_hmac(state->peer_session_id, from, hmac, handwindow, offset);
+ calculate_session_id_hmac(state->peer_session_id, from, key, handwindow, offset);
if (memcmp_constant_time(&expected_id, &state->server_session_id, SID_SIZE) == 0)
{
diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h
index 82cb5b1..03e8930 100644
--- a/src/openvpn/ssl_pkt.h
+++ b/src/openvpn/ssl_pkt.h
@@ -151,28 +151,20 @@
const struct link_socket_actual *from,
const struct buffer *buf);
-/* Creates an SHA256 HMAC context with a random key that is used for the
- * session id.
- *
- * We do not support loading this from a config file since continuing session
- * between restarts of OpenVPN has never been supported and that includes
- * early session setup.
- */
-hmac_ctx_t *session_id_hmac_init(void);
-
/**
* Calculates the HMAC based server session id based on a client session id
* and socket addr.
*
* @param client_sid session id of the client
* @param from link_socket from the client
- * @param hmac the hmac context to use for the calculation
+ * @param key the siphash key to use for the calculation
* @param handwindow the quantisation of the current time
* @param offset offset to 'now' to use
* @return the expected server session id
*/
struct session_id calculate_session_id_hmac(struct session_id client_sid,
- const struct openvpn_sockaddr *from, hmac_ctx_t *hmac,
+ const struct openvpn_sockaddr *from,
+ const uint8_t *key,
int handwindow, int offset);
/**
@@ -185,13 +177,13 @@
*
* @param state session information
* @param from link_socket from the client
- * @param hmac the hmac context to use for the calculation
+ * @param key the siphash key to use for the calculation
* @param handwindow the quantisation of the current time
* @param pkt_is_ack the packet being checked is a P_ACK_V1
* @return the expected server session id
*/
bool check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state, const struct openvpn_sockaddr *from,
- hmac_ctx_t *hmac, int handwindow, bool pkt_is_ack);
+ uint8_t *key, int handwindow, bool pkt_is_ack);
/*
* Write a control channel authentication record.
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index ae15759..c76d94a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -172,6 +172,7 @@
$(top_srcdir)/src/openvpn/reliable.c \
$(top_srcdir)/src/openvpn/run_command.c \
$(top_srcdir)/src/openvpn/session_id.c \
+ $(top_srcdir)/src/openvpn/siphash_reference.c \
$(top_srcdir)/src/openvpn/ssl_pkt.c \
$(top_srcdir)/src/openvpn/win32-util.c \
$(top_srcdir)/src/openvpn/tls_crypt.c
diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c
index cad2ce0..dc8144c 100644
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -42,6 +42,7 @@
#include "mss.h"
#include "reliable.h"
+#include "siphash.h"
int
parse_line(const char *line, char **p, const int n, const char *file, const int line_num,
@@ -403,7 +404,8 @@
static void
test_verify_hmac_tls_auth(void **ut_state)
{
- hmac_ctx_t *hmac = session_id_hmac_init();
+ uint8_t key[SIPHASH_KEY_SIZE] = { 0 };
+ siphash_key_init(key);
struct link_socket_actual from = { 0 };
from.dest.addr.sa.sa_family = AF_INET;
@@ -422,21 +424,20 @@
assert_int_equal(verdict, VERDICT_VALID_CONTROL_V1);
/* This is a valid packet but containing a random id instead of an HMAC id*/
- bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, false);
+ bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, false);
assert_false(valid);
free_tls_pre_decrypt_state(&state);
free_buf(&buf);
free_tas(&tas);
- hmac_ctx_cleanup(hmac);
- hmac_ctx_free(hmac);
}
static void
test_verify_hmac_none(void **ut_state)
{
now = 1000;
- hmac_ctx_t *hmac = session_id_hmac_init();
+ uint8_t key[SIPHASH_KEY_SIZE] = { 0 };
+ siphash_key_init(key);
struct link_socket_actual from = { 0 };
from.dest.addr.sa.sa_family = AF_INET;
@@ -456,13 +457,13 @@
assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
/* This packet has a random hmac, so it should fail to validate */
- bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true);
+ bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true);
assert_false(valid);
struct session_id client_id = { { 0xae, 0xb9, 0xaf, 0xe1, 0xf0, 0x1d, 0x79, 0xc8 } };
assert_memory_equal(&client_id, &state.peer_session_id, sizeof(struct session_id));
- struct session_id expected_id = calculate_session_id_hmac(client_id, &from.dest, hmac, 30, 0);
+ struct session_id expected_id = calculate_session_id_hmac(client_id, &from.dest, key, 30, 0);
free_tls_pre_decrypt_state(&state);
buf_reset_len(&buf);
@@ -474,7 +475,7 @@
verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf);
assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
- valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true);
+ valid = check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true);
assert_true(valid);
@@ -483,23 +484,23 @@
* So setting time to the two future ones should work
*/
now = 980;
- assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
now = 1040;
- assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
now = 1002;
- assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
now = 1022;
- assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
now = 1010;
- assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
/* Changing the IP address should make this invalid */
from.dest.addr.in4.sin_addr.s_addr = ntohl(0x01020305);
- assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
/* Change to the correct one again */
from.dest.addr.in4.sin_addr.s_addr = ntohl(0x01020304);
- assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_true(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
/* Modify the peer id, should now fail hmac verification */
buf_inc_len(&buf, -4);
@@ -508,18 +509,17 @@
free_tls_pre_decrypt_state(&state);
verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf);
assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
- assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true));
+ assert_false(check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true));
free_tls_pre_decrypt_state(&state);
free_buf(&buf);
- hmac_ctx_cleanup(hmac);
- hmac_ctx_free(hmac);
}
static void
test_verify_hmac_none_out_of_range_ack(void **ut_state)
{
- hmac_ctx_t *hmac = session_id_hmac_init();
+ uint8_t key[SIPHASH_KEY_SIZE] = { 0 };
+ rand_bytes(key, sizeof(key));
struct link_socket_actual from = { 0 };
from.dest.addr.sa.sa_family = AF_INET;
@@ -540,7 +540,7 @@
assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
/* should fail because it acks 2 */
- bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true);
+ bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true);
assert_false(valid);
free_tls_pre_decrypt_state(&state);
@@ -552,31 +552,17 @@
assert_int_equal(verdict, VERDICT_VALID_CONTROL_V1);
/* should fail because it has message id 2 */
- valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true);
+ valid = check_session_hmac_and_pkt_id(&state, &from.dest, key, 30, true);
assert_false(valid);
free_tls_pre_decrypt_state(&state);
free_buf(&buf);
- hmac_ctx_cleanup(hmac);
- hmac_ctx_free(hmac);
-}
-
-static hmac_ctx_t *
-init_static_hmac(void)
-{
- ASSERT(md_valid("SHA256"));
- hmac_ctx_t *hmac_ctx = hmac_ctx_new();
-
- uint8_t key[SHA256_DIGEST_LENGTH] = { 1, 2, 3, 0 };
-
- hmac_ctx_init(hmac_ctx, key, "SHA256");
- return hmac_ctx;
}
static void
test_calc_session_id_hmac_static(void **ut_state)
{
- hmac_ctx_t *hmac = init_static_hmac();
+ uint8_t key[SIPHASH_KEY_SIZE] = { 1, 2, 3, 0 };
static const int handwindow = 100;
struct openvpn_sockaddr addr = { 0 };
@@ -588,27 +574,27 @@
struct session_id client_id = { { 0, 1, 2, 3, 4, 5, 6, 7 } };
now = 1005;
- struct session_id server_id = calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 0);
+ struct session_id server_id = calculate_session_id_hmac(client_id, &addr, key, handwindow, 0);
- struct session_id expected_server_id = { { 0x84, 0x73, 0x52, 0x2b, 0x5b, 0xa9, 0x2a, 0x70 } };
+ struct session_id expected_server_id = { { 0xec, 0xa3, 0xd5, 0xcc, 0xb4, 0x7c, 0xa1, 0xee } };
/* We have to deal with different structs here annoyingly */
/* Linux has an unsigned short int as family_t and this is field is always
* stored in host endianness even though the rest of the struct isn't...,
* so Linux little endian differs from all BSD and Linux big endian */
if (sizeof(addr.addr.in4.sin_family) == sizeof(unsigned short int) && ntohs(AF_INET) != AF_INET)
{
- struct session_id linuxle = { { 0x8b, 0xeb, 0x3d, 0x20, 0x14, 0x53, 0xbe, 0x0a } };
+ struct session_id linuxle = { { 0x70, 0x04, 0x8c, 0x0f, 0xfe, 0x30, 0x85, 0x12 } };
expected_server_id = linuxle;
}
assert_memory_equal(expected_server_id.id, server_id.id, SID_SIZE);
struct session_id server_id_m1 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, -1);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, -1);
struct session_id server_id_p1 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 1);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, 1);
struct session_id server_id_p2 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 2);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, 2);
assert_memory_not_equal(expected_server_id.id, server_id_m1.id, SID_SIZE);
assert_memory_not_equal(expected_server_id.id, server_id_p1.id, SID_SIZE);
@@ -618,20 +604,17 @@
now = 1062;
struct session_id server_id2_m2 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, -2);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, -2);
struct session_id server_id2_m1 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, -1);
- struct session_id server_id2 = calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 0);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, -1);
+ struct session_id server_id2 = calculate_session_id_hmac(client_id, &addr, key, handwindow, 0);
struct session_id server_id2_p1 =
- calculate_session_id_hmac(client_id, &addr, hmac, handwindow, 1);
+ calculate_session_id_hmac(client_id, &addr, key, handwindow, 1);
assert_memory_equal(server_id2_m2.id, server_id_m1.id, SID_SIZE);
assert_memory_equal(server_id2_m1.id, expected_server_id.id, SID_SIZE);
assert_memory_equal(server_id2.id, server_id_p1.id, SID_SIZE);
assert_memory_equal(server_id2_p1.id, server_id_p2.id, SID_SIZE);
-
- hmac_ctx_cleanup(hmac);
- hmac_ctx_free(hmac);
}
static void
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1827?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3b6bb178ffb2bb49981bc23eabf04fe8e06d6fc3
Gerrit-Change-Number: 1827
Gerrit-PatchSet: 5
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel