The socket_info->connection_establish is set through
link_socket_set_outgoing_addr when we reach FULL_SYNC. This patch
introduces a new state in context_auth that replaces the
connection_established state for TLS connections. This make the state
machine easier to understand.
Also, rename "enum client_connect_status" to "multi_status", re-order
states so CAS_NOT_CONNECTED (=0) is the default state, and introduce
CAS_CONNECT_DONE as numerically highest so "are we done?" can be
easily checked.
This is part of the patchset to fix CVE-2020-15078 in "master" by
reorganizing the handling of incoming new and renegotiated TLS sessions
to make the code easier to understand and less prone to "edge case"
issues.
Patch v2: fix p2p mode server without (without ncp)
CVE: 2020-15078
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210520151148.2565578-3-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22419.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
... | ... |
@@ -280,7 +280,7 @@ static void |
280 | 280 |
check_connection_established(struct context *c) |
281 | 281 |
{ |
282 | 282 |
|
283 |
- if (CONNECTION_ESTABLISHED(c)) |
|
283 |
+ if (connection_established(c)) |
|
284 | 284 |
{ |
285 | 285 |
/* if --pull was specified, send a push request to server */ |
286 | 286 |
if (c->c2.tls_multi && c->options.pull) |
... | ... |
@@ -536,7 +536,7 @@ encrypt_sign(struct context *c, bool comp_frag) |
536 | 536 |
* has not yet succeeded. In non-TLS tls_multi mode is not defined |
537 | 537 |
* and we always pass packets. |
538 | 538 |
*/ |
539 |
- if (c->c2.tls_multi && c->c2.tls_multi->multi_state != CAS_SUCCEEDED) |
|
539 |
+ if (c->c2.tls_multi && c->c2.tls_multi->multi_state < CAS_CONNECT_DONE) |
|
540 | 540 |
{ |
541 | 541 |
c->c2.buf.len = 0; |
542 | 542 |
} |
... | ... |
@@ -971,7 +971,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo |
971 | 971 |
* has not yet succeeded. In non-TLS mode tls_multi is not defined |
972 | 972 |
* and we always pass packets. |
973 | 973 |
*/ |
974 |
- if (c->c2.tls_multi && c->c2.tls_multi->multi_state != CAS_SUCCEEDED) |
|
974 |
+ if (c->c2.tls_multi && c->c2.tls_multi->multi_state < CAS_CONNECT_DONE) |
|
975 | 975 |
{ |
976 | 976 |
c->c2.buf.len = 0; |
977 | 977 |
} |
... | ... |
@@ -411,6 +411,17 @@ io_wait(struct context *c, const unsigned int flags) |
411 | 411 |
} |
412 | 412 |
} |
413 | 413 |
|
414 |
-#define CONNECTION_ESTABLISHED(c) (get_link_socket_info(c)->connection_established) |
|
414 |
+static inline bool |
|
415 |
+connection_established(struct context *c) |
|
416 |
+{ |
|
417 |
+ if (c->c2.tls_multi) |
|
418 |
+ { |
|
419 |
+ return c->c2.tls_multi->multi_state >= CAS_CONNECT_DONE; |
|
420 |
+ } |
|
421 |
+ else |
|
422 |
+ { |
|
423 |
+ return get_link_socket_info(c)->connection_established; |
|
424 |
+ } |
|
425 |
+} |
|
415 | 426 |
|
416 | 427 |
#endif /* FORWARD_H */ |
... | ... |
@@ -674,7 +674,8 @@ multi_close_instance(struct multi_context *m, |
674 | 674 |
#ifdef ENABLE_MANAGEMENT |
675 | 675 |
set_cc_config(mi, NULL); |
676 | 676 |
#endif |
677 |
- if (mi->context.c2.tls_multi->multi_state == CAS_SUCCEEDED) |
|
677 |
+ |
|
678 |
+ if (mi->context.c2.tls_multi->multi_state >= CAS_CONNECT_DONE) |
|
678 | 679 |
{ |
679 | 680 |
multi_client_disconnect_script(mi); |
680 | 681 |
} |
... | ... |
@@ -775,7 +776,7 @@ multi_create_instance(struct multi_context *m, const struct mroute_addr *real) |
775 | 775 |
goto err; |
776 | 776 |
} |
777 | 777 |
|
778 |
- mi->context.c2.tls_multi->multi_state = CAS_PENDING; |
|
778 |
+ mi->context.c2.tls_multi->multi_state = CAS_NOT_CONNECTED; |
|
779 | 779 |
|
780 | 780 |
if (hash_n_elements(m->hash) >= m->max_clients) |
781 | 781 |
{ |
... | ... |
@@ -2407,7 +2408,7 @@ multi_client_connect_late_setup(struct multi_context *m, |
2407 | 2407 |
mi->reporting_addr_ipv6 = mi->context.c2.push_ifconfig_ipv6_local; |
2408 | 2408 |
|
2409 | 2409 |
/* set context-level authentication flag */ |
2410 |
- mi->context.c2.tls_multi->multi_state = CAS_SUCCEEDED; |
|
2410 |
+ mi->context.c2.tls_multi->multi_state = CAS_CONNECT_DONE; |
|
2411 | 2411 |
|
2412 | 2412 |
/* authentication complete, calculate dynamic client specific options */ |
2413 | 2413 |
if (!multi_client_set_protocol_options(&mi->context)) |
... | ... |
@@ -2649,9 +2650,9 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi) |
2649 | 2649 |
|
2650 | 2650 |
case CC_RET_DEFERRED: |
2651 | 2651 |
/* |
2652 |
- * we already set client_connect_status to DEFERRED_RESULT or |
|
2652 |
+ * we already set multi_status to DEFERRED_RESULT or |
|
2653 | 2653 |
* DEFERRED_NO_RESULT. We just return |
2654 |
- * from the function as having client_connect_status |
|
2654 |
+ * from the function as having multi_status |
|
2655 | 2655 |
*/ |
2656 | 2656 |
return; |
2657 | 2657 |
|
... | ... |
@@ -3003,8 +3004,8 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns |
3003 | 3003 |
{ |
3004 | 3004 |
/* connection is "established" when SSL/TLS key negotiation succeeds |
3005 | 3005 |
* and (if specified) auth user/pass succeeds */ |
3006 |
- if (is_cas_pending(mi->context.c2.tls_multi->multi_state) |
|
3007 |
- && CONNECTION_ESTABLISHED(&mi->context)) |
|
3006 |
+ |
|
3007 |
+ if (is_cas_pending(mi->context.c2.tls_multi->multi_state)) |
|
3008 | 3008 |
{ |
3009 | 3009 |
multi_connection_established(m, mi); |
3010 | 3010 |
} |
... | ... |
@@ -206,7 +206,7 @@ struct context_1 |
206 | 206 |
|
207 | 207 |
|
208 | 208 |
static inline bool |
209 |
-is_cas_pending(enum client_connect_status cas) |
|
209 |
+is_cas_pending(enum multi_status cas) |
|
210 | 210 |
{ |
211 | 211 |
return cas == CAS_PENDING || cas == CAS_PENDING_DEFERRED |
212 | 212 |
|| cas == CAS_PENDING_DEFERRED_PARTIAL; |
... | ... |
@@ -237,6 +237,8 @@ struct context_2 |
237 | 237 |
|
238 | 238 |
struct link_socket *link_socket; /* socket used for TCP/UDP connection to remote */ |
239 | 239 |
bool link_socket_owned; |
240 |
+ |
|
241 |
+ /** This variable is used instead link_socket->info for P2MP UDP childs */ |
|
240 | 242 |
struct link_socket_info *link_socket_info; |
241 | 243 |
const struct link_socket *accept_from; /* possibly do accept() on a parent link_socket */ |
242 | 244 |
|
... | ... |
@@ -867,7 +867,7 @@ process_incoming_push_request(struct context *c) |
867 | 867 |
send_auth_failed(c, client_reason); |
868 | 868 |
ret = PUSH_MSG_AUTH_FAILURE; |
869 | 869 |
} |
870 |
- else if (c->c2.tls_multi->multi_state == CAS_SUCCEEDED) |
|
870 |
+ else if (c->c2.tls_multi->multi_state >= CAS_CONNECT_DONE) |
|
871 | 871 |
{ |
872 | 872 |
time_t now; |
873 | 873 |
|
... | ... |
@@ -2381,19 +2381,22 @@ key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi |
2381 | 2381 |
* If we're a p2mp server to allow NCP, the first key |
2382 | 2382 |
* generation is postponed until after the connect script finished and the |
2383 | 2383 |
* NCP options can be processed. Since that always happens at after connect |
2384 |
- * script options are available the CAS_SUCCEEDED status is identical to |
|
2385 |
- * NCP options are processed and we have no extra state for NCP finished. |
|
2384 |
+ * script options are available the CAS_CONNECT_DONE status is identical to |
|
2385 |
+ * NCP options are processed and do not wait for NCP being finished. |
|
2386 | 2386 |
*/ |
2387 |
- if (session->opt->server && (session->opt->mode != MODE_SERVER |
|
2388 |
- || multi->multi_state == CAS_SUCCEEDED)) |
|
2387 |
+ if (ks->authenticated > KS_AUTH_FALSE && session->opt->server |
|
2388 |
+ && ((session->opt->mode == MODE_SERVER && multi->multi_state >= CAS_CONNECT_DONE) |
|
2389 |
+ || (session->opt->mode == MODE_POINT_TO_POINT && !session->opt->pull))) |
|
2389 | 2390 |
{ |
2390 |
- if (ks->authenticated > KS_AUTH_FALSE) |
|
2391 |
+ /* if key_id >= 1, is a renegotiation, so we use the already established |
|
2392 |
+ * parameters and do not need to delay anything. */ |
|
2393 |
+ |
|
2394 |
+ /* key-id == 0 and multi_state >= CAS_CONNECT_DONE is a special case of |
|
2395 |
+ * the server reusing the session of a reconnecting client. */ |
|
2396 |
+ if (!tls_session_generate_data_channel_keys(session)) |
|
2391 | 2397 |
{ |
2392 |
- if (!tls_session_generate_data_channel_keys(session)) |
|
2393 |
- { |
|
2394 |
- msg(D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed"); |
|
2395 |
- goto error; |
|
2396 |
- } |
|
2398 |
+ msg(D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed"); |
|
2399 |
+ goto error; |
|
2397 | 2400 |
} |
2398 | 2401 |
} |
2399 | 2402 |
|
... | ... |
@@ -2801,6 +2804,21 @@ tls_process(struct tls_multi *multi, |
2801 | 2801 |
/* Set outgoing address for data channel packets */ |
2802 | 2802 |
link_socket_set_outgoing_addr(to_link_socket_info, &ks->remote_addr, session->common_name, session->opt->es); |
2803 | 2803 |
|
2804 |
+ /* Check if we need to advance the tls_multi state machine */ |
|
2805 |
+ if (multi->multi_state == CAS_NOT_CONNECTED) |
|
2806 |
+ { |
|
2807 |
+ if (session->opt->mode == MODE_SERVER) |
|
2808 |
+ { |
|
2809 |
+ /* On a server we continue with running connect scripts next */ |
|
2810 |
+ multi->multi_state = CAS_PENDING; |
|
2811 |
+ } |
|
2812 |
+ else |
|
2813 |
+ { |
|
2814 |
+ /* Skip the connect script related states */ |
|
2815 |
+ multi->multi_state = CAS_CONNECT_DONE; |
|
2816 |
+ } |
|
2817 |
+ } |
|
2818 |
+ |
|
2804 | 2819 |
/* Flush any payload packets that were buffered before our state transitioned to S_ACTIVE */ |
2805 | 2820 |
flush_payload_buffer(ks); |
2806 | 2821 |
|
... | ... |
@@ -504,12 +504,18 @@ struct tls_session |
504 | 504 |
/* client authentication state, CAS_SUCCEEDED must be 0 since |
505 | 505 |
* non multi code path still checks this variable but does not initialise it |
506 | 506 |
* so the code depends on zero initialisation */ |
507 |
-enum client_connect_status { |
|
508 |
- CAS_SUCCEEDED=0, |
|
507 |
+ |
|
508 |
+/* CAS_NOT_CONNECTED is the initial state for every context. When the *first* |
|
509 |
+ * tls_session reaches S_ACTIVE, this state machine moves to CAS_PENDING (server) |
|
510 |
+ * or CAS_CONNECT_DONE (client/p2p) as clients skip the stages associated with |
|
511 |
+ * connect scripts/plugins */ |
|
512 |
+enum multi_status { |
|
513 |
+ CAS_NOT_CONNECTED, |
|
509 | 514 |
CAS_PENDING, |
510 | 515 |
CAS_PENDING_DEFERRED, |
511 | 516 |
CAS_PENDING_DEFERRED_PARTIAL, /**< at least handler succeeded, no result yet*/ |
512 | 517 |
CAS_FAILED, |
518 |
+ CAS_CONNECT_DONE, |
|
513 | 519 |
}; |
514 | 520 |
|
515 | 521 |
|
... | ... |
@@ -548,7 +554,7 @@ struct tls_multi |
548 | 548 |
|
549 | 549 |
int n_sessions; /**< Number of sessions negotiated thus |
550 | 550 |
* far. */ |
551 |
- enum client_connect_status multi_state; |
|
551 |
+ enum multi_status multi_state; |
|
552 | 552 |
|
553 | 553 |
/* |
554 | 554 |
* Number of errors. |