Re: [Openvpn-devel] [PATCH 1/3] Refactor/Reformat tls_pre_decrypt
3x minor typos
On 22/07/2020 10:30, Arne Schwabe wrote:
- Extract data packet handling to its own function
- Replace two instances of
if (x) { code }
with
if (!x) return; code
- Remove extra curly braces that were used for pre C99 code style
to be able to declare variables in the middle of a block
This patch is easier to review with "ignore white space" as the
diff is then a lot smaller in that case and the changes more obvious.
Signed-off-by: Arne Schwabe
---
src/openvpn/ssl.c | 791 --
1 file changed, 410 insertions(+), 381 deletions(-)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 06dc9f8f..6d146a63 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3166,6 +3166,102 @@ nohard:
* to implement a multiplexed TLS channel over the TCP/UDP port.
*/
+static inline void
+handle_data_channel_paket(struct tls_multi *multi,
+ const struct link_socket_actual *from,
+ struct buffer *buf,
+ struct crypto_options **opt,
+ bool floated,
+ const uint8_t **ad_start)
+{
+struct gc_arena gc = gc_new();
+
+uint8_t c = *BPTR(buf);
+int op = c >> P_OPCODE_SHIFT;
+int key_id = c & P_KEY_ID_MASK;
+
+/* data channel packet */
+for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+{
+struct key_state *ks = multi->key_scan[i];
+
+/*
+ * This is the basic test of TLS state compatibility between a local
OpenVPN
+ * instance and its remote peer.
+ *
+ * If the test fails, it tells us that we are getting a packet from a
source
+ * which claims reference to a prior negotiated TLS session, but the
local
+ * OpenVPN instance has no memory of such a negotiation.
+ *
+ * It almost always occurs on UDP sessions when the passive side of the
+ * connection is restarted without the active side restarting as well
(the
+ * passive side is the server which only listens for the connections,
the
+ * active side is the client which initiates connections).
+ */
+if (DECRYPT_KEY_ENABLED(multi, ks)
+&& key_id == ks->key_id
+&& (ks->authenticated == KS_AUTH_TRUE)
+&& (floated || link_socket_actual_match(from, >remote_addr)))
+{
+if (!ks->crypto_options.key_ctx_bi.initialized)
+{
+msg(D_MULTI_DROPPED,
+"Key %s [%d] not initialized (yet), dropping packet.",
+print_link_socket_actual(from, ), key_id);
+goto error_lite;
+}
+
+/* return appropriate data channel decrypt key in opt */
+*opt = >crypto_options;
+if (op == P_DATA_V2)
+{
+*ad_start = BPTR(buf);
+}
+ASSERT(buf_advance(buf, 1));
+if (op == P_DATA_V1)
+{
+*ad_start = BPTR(buf);
+}
+else if (op == P_DATA_V2)
+{
+if (buf->len < 4)
+{
+msg(D_TLS_ERRORS, "Protocol error: received P_DATA_V2 from %s but
length is < 4",
+print_link_socket_actual(from, ));
+goto error;
+}
+ASSERT(buf_advance(buf, 3));
+}
+
+++ks->n_packets;
+ks->n_bytes += buf->len;
+dmsg(D_TLS_KEYSELECT,
+ "TLS: tls_pre_decrypt, key_id=%d, IP=%s",
+ key_id, print_link_socket_actual(from, ));
+gc_free();
+return;
+}
+}
+
+msg(D_TLS_ERRORS,
+"TLS Error: local/remote TLS keys are out of sync: %s [%d]",
+print_link_socket_actual(from, ), key_id);
+goto error_lite;
+
+
+done:
+buf->len = 0;
+*opt = NULL;
+gc_free();
+return;
+
+error:
+++multi->n_soft_errors;
+error_lite:
+tls_clear_error();
+goto done;
+}
+
/*
*
* When we are in TLS mode, this is the first routine which sees
@@ -3199,440 +3295,374 @@ tls_pre_decrypt(struct tls_multi *multi,
bool floated,
const uint8_t **ad_start)
{
+
+if (buf->len <= 0)
+{
+buf->len = 0;
+*opt = NULL;
+return false;
+}
+
struct gc_arena gc = gc_new();
bool ret = false;
-if (buf->len > 0)
+/* get opcode */
+uint8_t pkt_firstbyte = *BPTR(buf);
+int op = pkt_firstbyte >> P_OPCODE_SHIFT;
+
+if ((op == P_DATA_V1) || (op == P_DATA_V2))
{
-int i;
-int op;
-int key_id;
+handle_data_channel_paket(multi, from, buf, opt, floated, ad_start);
+return false;
+}
-/* get opcode and key ID */
+/* get key_id */
+int key_id = pkt_firstbyte & P_KEY_ID_MASK;
+
+/*
[Openvpn-devel] [PATCH 1/3] Refactor/Reformat tls_pre_decrypt
- Extract data packet handling to its own function
- Replace two instances of
if (x) { code }
with
if (!x) return; code
- Remove extra curly braces that were used for pre C99 code style
to be able to declare variables in the middle of a block
This patch is easier to review with "ignore white space" as the
diff is then a lot smaller in that case and the changes more obvious.
Signed-off-by: Arne Schwabe
---
src/openvpn/ssl.c | 791 --
1 file changed, 410 insertions(+), 381 deletions(-)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 06dc9f8f..6d146a63 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3166,6 +3166,102 @@ nohard:
* to implement a multiplexed TLS channel over the TCP/UDP port.
*/
+static inline void
+handle_data_channel_paket(struct tls_multi *multi,
+ const struct link_socket_actual *from,
+ struct buffer *buf,
+ struct crypto_options **opt,
+ bool floated,
+ const uint8_t **ad_start)
+{
+struct gc_arena gc = gc_new();
+
+uint8_t c = *BPTR(buf);
+int op = c >> P_OPCODE_SHIFT;
+int key_id = c & P_KEY_ID_MASK;
+
+/* data channel packet */
+for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+{
+struct key_state *ks = multi->key_scan[i];
+
+/*
+ * This is the basic test of TLS state compatibility between a local
OpenVPN
+ * instance and its remote peer.
+ *
+ * If the test fails, it tells us that we are getting a packet from a
source
+ * which claims reference to a prior negotiated TLS session, but the
local
+ * OpenVPN instance has no memory of such a negotiation.
+ *
+ * It almost always occurs on UDP sessions when the passive side of the
+ * connection is restarted without the active side restarting as well
(the
+ * passive side is the server which only listens for the connections,
the
+ * active side is the client which initiates connections).
+ */
+if (DECRYPT_KEY_ENABLED(multi, ks)
+&& key_id == ks->key_id
+&& (ks->authenticated == KS_AUTH_TRUE)
+&& (floated || link_socket_actual_match(from, >remote_addr)))
+{
+if (!ks->crypto_options.key_ctx_bi.initialized)
+{
+msg(D_MULTI_DROPPED,
+"Key %s [%d] not initialized (yet), dropping packet.",
+print_link_socket_actual(from, ), key_id);
+goto error_lite;
+}
+
+/* return appropriate data channel decrypt key in opt */
+*opt = >crypto_options;
+if (op == P_DATA_V2)
+{
+*ad_start = BPTR(buf);
+}
+ASSERT(buf_advance(buf, 1));
+if (op == P_DATA_V1)
+{
+*ad_start = BPTR(buf);
+}
+else if (op == P_DATA_V2)
+{
+if (buf->len < 4)
+{
+msg(D_TLS_ERRORS, "Protocol error: received P_DATA_V2 from
%s but length is < 4",
+print_link_socket_actual(from, ));
+goto error;
+}
+ASSERT(buf_advance(buf, 3));
+}
+
+++ks->n_packets;
+ks->n_bytes += buf->len;
+dmsg(D_TLS_KEYSELECT,
+ "TLS: tls_pre_decrypt, key_id=%d, IP=%s",
+ key_id, print_link_socket_actual(from, ));
+gc_free();
+return;
+}
+}
+
+msg(D_TLS_ERRORS,
+"TLS Error: local/remote TLS keys are out of sync: %s [%d]",
+print_link_socket_actual(from, ), key_id);
+goto error_lite;
+
+
+done:
+buf->len = 0;
+*opt = NULL;
+gc_free();
+return;
+
+error:
+++multi->n_soft_errors;
+error_lite:
+tls_clear_error();
+goto done;
+}
+
/*
*
* When we are in TLS mode, this is the first routine which sees
@@ -3199,440 +3295,374 @@ tls_pre_decrypt(struct tls_multi *multi,
bool floated,
const uint8_t **ad_start)
{
+
+if (buf->len <= 0)
+{
+buf->len = 0;
+*opt = NULL;
+return false;
+}
+
struct gc_arena gc = gc_new();
bool ret = false;
-if (buf->len > 0)
+/* get opcode */
+uint8_t pkt_firstbyte = *BPTR(buf);
+int op = pkt_firstbyte >> P_OPCODE_SHIFT;
+
+if ((op == P_DATA_V1) || (op == P_DATA_V2))
{
-int i;
-int op;
-int key_id;
+handle_data_channel_paket(multi, from, buf, opt, floated, ad_start);
+return false;
+}
-/* get opcode and key ID */
+/* get key_id */
+int key_id = pkt_firstbyte & P_KEY_ID_MASK;
+
+/* control channel packet */
+bool do_burst = false;
+bool new_link = false;
+
