plaisthos has uploaded this change for review. (
http://gerrit.openvpn.net/c/openvpn/+/1762?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.
Change-Id: Id9781a21a21637a12488fab857bc10aacc4a73c0
---
M src/openvpn/ssl.c
1 file changed, 86 insertions(+), 58 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/62/1762/1
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 60df7ce..5535207 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -95,6 +95,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, struct key_state *ks);
+
+
/**
* Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
*
@@ -902,10 +910,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);
@@ -1052,8 +1065,9 @@
*
*/
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);
tls_wrap_free(&session->tls_wrap_reneg);
@@ -1062,7 +1076,7 @@
/* 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);
+ key_state_free(&session->key[i], false, to_link);
}
free(session->common_name);
@@ -1081,14 +1095,16 @@
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), reinit_src);
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)
@@ -1104,9 +1120,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);
}
@@ -1259,7 +1275,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)
@@ -1754,13 +1770,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);
@@ -1771,7 +1787,7 @@
void
tls_session_soft_reset(struct tls_multi *tls_multi)
{
- key_state_soft_reset(&tls_multi->session[TM_ACTIVE]);
+ key_state_soft_reset(&tls_multi->session[TM_ACTIVE], NULL);
}
/*
@@ -3041,13 +3057,13 @@
session->opt->aead_usage_limit,
ks->crypto_options.key_ctx_bi.decrypt.plaintext_blocks +
ks->n_packets,
session->opt->aead_usage_limit);
- 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");
}
@@ -3140,6 +3156,55 @@
return false;
}
+static void
+check_keystate_buf_not_used(struct buffer *to_link, 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
@@ -3151,11 +3216,11 @@
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;
}
+ uint8_t *dataptr = to_link->data;
/* Checks buffers in tls_wrap */
if (session->tls_wrap.work.data == dataptr)
@@ -3174,41 +3239,7 @@
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;
- }
+ check_keystate_buf_not_used(to_link, ks);
}
return;
@@ -3302,13 +3333,11 @@
if (i == TM_ACTIVE && ks_lame->state >= S_GENERATED_KEYS
&& !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);
}
}
}
@@ -3361,7 +3390,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");
}
@@ -3376,8 +3405,7 @@
*/
if (TLS_AUTHENTICATED(multi, &multi->session[TM_INITIAL].key[KS_PRIMARY]))
{
- check_session_buf_not_used(to_link, &multi->session[TM_ACTIVE]);
- move_session(multi, TM_ACTIVE, TM_INITIAL, true);
+ move_session(multi, TM_ACTIVE, TM_INITIAL, true, to_link);
tas = tls_authentication_status(multi);
msg(D_TLS_DEBUG_LOW,
"TLS: tls_multi_process: initial untrusted "
@@ -3779,7 +3807,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", i,
session_id_print(&sid, &gc));
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1762?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Id9781a21a21637a12488fab857bc10aacc4a73c0
Gerrit-Change-Number: 1762
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
