@@ -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,8 +3244,7 @@
*/
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);
+ move_session(multi, TM_ACTIVE, TM_UNTRUSTED, true, to_link);
msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: untrusted session promoted to %strusted",
tas == TLS_AUTHENTICATION_SUCCEEDED ? "" : "semi-");
}
@@ -3622,7 +3659,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",