MaxF has uploaded this change for review. ( https://gerrit.openvpn.net/c/openvpn/+/1985?usp=email )
Change subject: Move check_session_buf_not_used method into the method that free the buffer ...................................................................... Move check_session_buf_not_used method into the method that free the buffer This cleans the code up a bit and ensure that we do not miss an invocation of a problematic code path. This is still a band-aid fix and a real fix requires more refactoring and changing the logic. v2: sprinkle some "const" over check_keystate_buf_not_used() args v3: add missing doxygen for to_link parameter to tls_session_free() CVE: 2026-84471 Reported-By: Andreas Gabriel Berbescu <[email protected]> Tested-By: Andreas Gabriel Berbescu <[email protected]> Github: openvpn/openvpn-private-issues#157 Reported-By: Haruki Oyama (Waseda University) Github: openvpn/openvpn-private-issues#132 Change-Id: I64920ed9f714803604d76c6df6b38cf20ce7626d Signed-off-by: Arne Schwabe <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1893 Acked-by: MaxF <[email protected]> Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg38913.html Signed-off-by: Gert Doering <[email protected]> (cherry picked from commit 2a881d737b2562ee28247148160cfb60f0ba4645) --- M src/openvpn/ssl.c 1 file changed, 102 insertions(+), 63 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/85/1985/1 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 9b6cfa0..cedefa2 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -93,6 +93,14 @@ #endif /* ifdef MEASURE_TLS_HANDSHAKE_STATS */ +/* forward decleration since tls_process needs this function prototype */ +static void +check_session_buf_not_used(struct buffer *to_link, struct tls_session *session); + +static void +check_keystate_buf_not_used(struct buffer *to_link, const struct key_state *ks); + + /** * SSL/TLS Cipher suite name translation table */ @@ -989,10 +997,15 @@ * cleaned up. * @param clear - Whether the memory allocated for the \a ks object * should be overwritten with 0s. + * + * @param to_link - if not NULL, check that the buffer does not contain + * any pointer to one of the internal structs of ks */ static void -key_state_free(struct key_state *ks, bool clear) +key_state_free(struct key_state *ks, bool clear, struct buffer *to_link) { + check_keystate_buf_not_used(to_link, ks); + ks->state = S_UNDEF; key_state_ssl_free(&ks->ks_ssl); @@ -1143,16 +1156,25 @@ * @param session - A pointer to the \c tls_session structure to be * cleaned up. * @param clear - Whether the memory allocated for the \a session - * object should be overwritten with 0s. + * object should be overwritten with 0s. This + * implicitly sets many states to 0/false, + * e.g. the validity of the keys in the structure + * @param to_link - if not NULL, check that the buffer does not contain + * any pointer to one of the internal structs of ks + * */ static void -tls_session_free(struct tls_session *session, bool clear) +tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link) { + check_session_buf_not_used(to_link, session); tls_wrap_free(&session->tls_wrap); for (size_t i = 0; i < KS_SIZE; ++i) { - key_state_free(&session->key[i], false); + /* we don't need clear=true for this call since + * the structs are part of session and get cleared + * as part of session */ + key_state_free(&session->key[i], false, to_link); } if (session->common_name) @@ -1174,8 +1196,10 @@ static void -move_session(struct tls_multi *multi, int dest, int src, bool reinit_src) +move_session(struct tls_multi *multi, int dest, int src, bool reinit_src, + struct buffer *to_link) { + check_session_buf_not_used(to_link, &multi->session[dest]); msg(D_TLS_DEBUG_LOW, "TLS: move_session: dest=%s src=%s reinit_src=%d", session_index_name(dest), session_index_name(src), @@ -1183,7 +1207,7 @@ ASSERT(src != dest); ASSERT(src >= 0 && src < TM_SIZE); ASSERT(dest >= 0 && dest < TM_SIZE); - tls_session_free(&multi->session[dest], false); + tls_session_free(&multi->session[dest], false, to_link); multi->session[dest] = multi->session[src]; if (reinit_src) @@ -1199,9 +1223,9 @@ } static void -reset_session(struct tls_multi *multi, struct tls_session *session) +reset_session(struct tls_multi *multi, struct tls_session *session, struct buffer *to_link) { - tls_session_free(session, false); + tls_session_free(session, false, to_link); tls_session_init(multi, session); } @@ -1369,7 +1393,7 @@ for (int i = 0; i < TM_SIZE; ++i) { - tls_session_free(&multi->session[i], false); + tls_session_free(&multi->session[i], false, NULL); } if (clear) @@ -2125,13 +2149,13 @@ * active key. */ static void -key_state_soft_reset(struct tls_session *session) +key_state_soft_reset(struct tls_session *session, struct buffer *to_link) { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ struct key_state *ks_lame = &session->key[KS_LAME_DUCK]; /* retiring key */ ks->must_die = now + session->opt->transition_window; /* remaining lifetime of old key */ - key_state_free(ks_lame, false); + key_state_free(ks_lame, false, to_link); *ks_lame = *ks; key_state_init(session, ks); @@ -2249,7 +2273,7 @@ * push request, also signal that the client wants * to get push-reply messages without without requiring a round * trip for a push request message*/ - if(session->opt->pull) + if (session->opt->pull) { iv_proto |= IV_PROTO_REQUEST_PUSH; } @@ -2426,7 +2450,7 @@ * NCP options are processed and we have no extra state for NCP finished. */ if (session->opt->server && (session->opt->mode != MODE_SERVER - || multi->multi_state == CAS_SUCCEEDED)) + || multi->multi_state == CAS_SUCCEEDED)) { if (ks->authenticated > KS_AUTH_FALSE) { @@ -2679,13 +2703,13 @@ (int) (now - ks->established), session->opt->renegotiate_seconds, ks->n_bytes, session->opt->renegotiate_bytes, ks->n_packets, session->opt->renegotiate_packets); - key_state_soft_reset(session); + key_state_soft_reset(session, to_link); } /* Kill lame duck key transition_window seconds after primary key negotiation */ if (lame_duck_must_die(session, wakeup)) { - key_state_free(ks_lame, true); + key_state_free(ks_lame, true, to_link); msg(D_TLS_DEBUG_LOW, "TLS: tls_process: killed expiring key"); } @@ -3009,6 +3033,56 @@ return false; } +static void +check_keystate_buf_not_used(struct buffer *to_link, const struct key_state *ks) +{ + if (ks->state == S_UNDEF || !to_link || !to_link->data) + { + return; + } + + uint8_t *dataptr = to_link->data; + + /* we don't expect send_reliable to be NULL when state is + * not S_UNDEF, but people have reported crashes nonetheless, + * therefore we better catch this event, report and exit. + */ + if (!ks->send_reliable) + { + msg(M_FATAL, + "ERROR: ks.send_reliable (key-id %d), is NULL " + "while key state is %s. Exiting.", + ks->key_id, state_name(ks->state)); + } + + for (int j = 0; j < ks->send_reliable->size; j++) + { + if (ks->send_reliable->array[j].buf.data == dataptr) + { + msg(M_INFO, + "Warning buffer of freed TLS session is still in" + " use (key-id %d, ks.send_reliable->array[%d])", + ks->key_id, j); + + goto used; + } + } + + if (ks->ack_write_buf.data == dataptr) + { + msg(M_INFO, "Warning buffer of freed TLS session is still in use " + "(ks.ack_write_buf, key-id %d)", + ks->key_id); + + goto used; + } + return; + +used: + to_link->len = 0; + to_link->data = 0; +} + /** * This is a safe guard function to double check that a buffer from a session is * not used in a session to avoid a use after free. @@ -3019,58 +3093,24 @@ static void check_session_buf_not_used(struct buffer *to_link, struct tls_session *session) { - uint8_t *dataptr = to_link->data; - if (!dataptr) + if (!to_link || !to_link->data) { return; } + const uint8_t *dataptr = to_link->data; /* Checks buffers in tls_wrap */ if (session->tls_wrap.work.data == dataptr) { msg(M_INFO, "Warning buffer of freed TLS session is " - "still in use (tls_wrap.work.data)"); + "still in use (tls_wrap.work.data)"); goto used; } for (int i = 0; i < KS_SIZE; i++) { - struct key_state *ks = &session->key[i]; - if (ks->state == S_UNDEF) - { - continue; - } - - /* we don't expect send_reliable to be NULL when state is - * not S_UNDEF, but people have reported crashes nonetheless, - * therefore we better catch this event, report and exit. - */ - if (!ks->send_reliable) - { - msg(M_FATAL, - "ERROR: session->key[%d]->send_reliable is NULL " - "while key state is %s. Exiting.", - i, state_name(ks->state)); - } - - for (int j = 0; j < ks->send_reliable->size; j++) - { - if (ks->send_reliable->array[j].buf.data == dataptr) - { - msg(M_INFO, - "Warning buffer of freed TLS session is still in" - " use (session->key[%d].send_reliable->array[%d])", - i, j); - - goto used; - } - } - if (ks->ack_write_buf.data == dataptr) - { - msg(M_INFO, "Warning buffer of freed TLS session is still in use (session->key[%d].ack_write_buf)", i); - - goto used; - } + const struct key_state *ks = &session->key[i]; + check_keystate_buf_not_used(to_link, ks); } return; @@ -3170,13 +3210,11 @@ && ks_lame->state >= S_ACTIVE && !multi->opt.single_session) { - check_session_buf_not_used(to_link, session); - move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true); + move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true, to_link); } else { - check_session_buf_not_used(to_link, session); - reset_session(multi, session); + reset_session(multi, session, to_link); } } } @@ -3191,7 +3229,7 @@ */ if (lame_duck_must_die(&multi->session[TM_LAME_DUCK], wakeup)) { - tls_session_free(&multi->session[TM_LAME_DUCK], true); + tls_session_free(&multi->session[TM_LAME_DUCK], true, to_link); msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: killed expiring key"); } @@ -3206,9 +3244,10 @@ */ if (DECRYPT_KEY_ENABLED(multi, &multi->session[TM_UNTRUSTED].key[KS_PRIMARY])) { - check_session_buf_not_used(to_link, &multi->session[TM_ACTIVE]); - move_session(multi, TM_ACTIVE, TM_UNTRUSTED, true); - msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: untrusted session promoted to %strusted", + move_session(multi, TM_ACTIVE, TM_UNTRUSTED, true, to_link); + tas = tls_authentication_status(multi, TLS_MULTI_AUTH_STATUS_INTERVAL); + msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: initial untrusted " + "session promoted to %strusted", tas == TLS_AUTHENTICATION_SUCCEEDED ? "" : "semi-"); } @@ -3622,7 +3661,7 @@ goto error; } - key_state_soft_reset(session); + key_state_soft_reset(session, NULL); dmsg(D_TLS_DEBUG, "TLS: received P_CONTROL_SOFT_RESET_V1 s=%d sid=%s", -- To view, visit https://gerrit.openvpn.net/c/openvpn/+/1985?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: openvpn Gerrit-Branch: release/2.5 Gerrit-Change-Id: I64920ed9f714803604d76c6df6b38cf20ce7626d Gerrit-Change-Number: 1985 Gerrit-PatchSet: 1 Gerrit-Owner: MaxF <[email protected]> Gerrit-CC: Arne Schwabe <[email protected]> Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
