@@ -96,14 +96,6 @@ show_tls_performance_stats(void)
#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);
-
-
/**
* Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
*
@@ -904,15 +896,10 @@ key_state_init(struct tls_session *session, struct key_state *ks)
* 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, struct buffer *to_link)
+key_state_free(struct key_state *ks, bool clear)
{
- check_keystate_buf_not_used(to_link, ks);
-
ks->state = S_UNDEF;
key_state_ssl_free(&ks->ks_ssl);
@@ -1056,14 +1043,10 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
* 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, struct buffer *to_link)
+tls_session_free(struct tls_session *session, bool clear)
{
- check_session_buf_not_used(to_link, session);
tls_wrap_free(&session->tls_wrap);
tls_wrap_free(&session->tls_wrap_reneg);
@@ -1072,7 +1055,7 @@ tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link
/* 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);
+ key_state_free(&session->key[i], false);
}
free(session->common_name);
@@ -1091,16 +1074,14 @@ tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link
static void
-move_session(struct tls_multi *multi, int dest, int src, bool reinit_src,
- struct buffer *to_link)
+move_session(struct tls_multi *multi, int dest, int src, bool reinit_src)
{
- 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, to_link);
+ tls_session_free(&multi->session[dest], false);
multi->session[dest] = multi->session[src];
if (reinit_src)
@@ -1116,9 +1097,9 @@ move_session(struct tls_multi *multi, int dest, int src, bool reinit_src,
}
static void
-reset_session(struct tls_multi *multi, struct tls_session *session, struct buffer *to_link)
+reset_session(struct tls_multi *multi, struct tls_session *session)
{
- tls_session_free(session, false, to_link);
+ tls_session_free(session, false);
tls_session_init(multi, session);
}
@@ -1192,6 +1173,8 @@ void
tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu)
{
tls_init_control_channel_frame_parameters(&multi->opt.frame, tls_mtu);
+ multi->to_link_buf = alloc_buf(BUF_SIZE(&multi->opt.frame));
+
/* initialize the active and untrusted sessions */
tls_session_init(multi, &multi->session[TM_ACTIVE]);
@@ -1283,9 +1266,11 @@ tls_multi_free(struct tls_multi *multi, bool clear)
for (int i = 0; i < TM_SIZE; ++i)
{
- tls_session_free(&multi->session[i], false, NULL);
+ tls_session_free(&multi->session[i], false);
}
+ free_buf(&multi->to_link_buf);
+
if (clear)
{
secure_memzero(multi, sizeof(*multi));
@@ -1816,13 +1801,13 @@ flush_payload_buffer(struct key_state *ks)
* active key.
*/
static void
-key_state_soft_reset(struct tls_session *session, struct buffer *to_link)
+key_state_soft_reset(struct tls_session *session)
{
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, to_link);
+ key_state_free(ks_lame, false);
*ks_lame = *ks;
key_state_init(session, ks);
@@ -1833,7 +1818,7 @@ key_state_soft_reset(struct tls_session *session, struct buffer *to_link)
void
tls_session_soft_reset(struct tls_multi *tls_multi)
{
- key_state_soft_reset(&tls_multi->session[TM_ACTIVE], NULL);
+ key_state_soft_reset(&tls_multi->session[TM_ACTIVE]);
}
/*
@@ -2914,18 +2899,15 @@ tls_process_state(struct tls_multi *multi, struct tls_session *session, struct b
const struct buffer *buf = reliable_send(ks->send_reliable, &opcode);
ASSERT(buf);
- struct buffer b = *buf;
INCR_SENT;
- write_control_auth(session, ks, &b, to_link_addr, opcode, CONTROL_SEND_ACK_MAX, true);
- *to_link = b;
+ write_control_auth(session, ks, buf, &multi->to_link_buf, to_link_addr, opcode,
+ CONTROL_SEND_ACK_MAX, true);
+ *to_link = multi->to_link_buf;
dmsg(D_TLS_DEBUG, "Reliable -> TCP/UDP");
- /* This changed the state of the outgoing buffer. In order to avoid
- * running this function again/further and invalidating the key_state
- * buffer and accessing the buffer that is now in to_link after it being
- * freed for a potential error, we shortcircuit exiting of the outer
- * process here. */
+ /* This changed the state of the outgoing buffer. Stop this
+ * processing pass so that the event loop can send the packet. */
return false;
}
@@ -3165,13 +3147,13 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer
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, to_link);
+ key_state_soft_reset(session);
}
/* Kill lame duck key transition_window seconds after primary key negotiation */
if (lame_duck_must_die(session, wakeup))
{
- key_state_free(ks_lame, true, to_link);
+ key_state_free(ks_lame, true);
msg(D_TLS_DEBUG_LOW, "TLS: tls_process: killed expiring key");
}
@@ -3220,8 +3202,9 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer
{
struct buffer buf = ks->ack_write_buf;
ASSERT(buf_init(&buf, multi->opt.frame.buf.headroom));
- write_control_auth(session, ks, &buf, to_link_addr, P_ACK_V1, RELIABLE_ACK_SIZE, false);
- *to_link = buf;
+ write_control_auth(session, ks, &buf, &multi->to_link_buf, to_link_addr, P_ACK_V1,
+ RELIABLE_ACK_SIZE, false);
+ *to_link = multi->to_link_buf;
dmsg(D_TLS_DEBUG, "Dedicated ACK -> TCP/UDP");
}
}
@@ -3264,98 +3247,6 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer
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;
- }
-
- const 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.
- *
- * @param to_link
- * @param session
- */
-static void
-check_session_buf_not_used(struct buffer *to_link, struct tls_session *session)
-{
- 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)");
- goto used;
- }
- if (session->tls_wrap_reneg.work.data == dataptr)
- {
- msg(M_INFO, "Warning buffer of freed TLS session is "
- "still in use (tls_wrap_reneg.work.data)");
- goto used;
- }
-
- for (int i = 0; i < KS_SIZE; i++)
- {
- const struct key_state *ks = &session->key[i];
- check_keystate_buf_not_used(to_link, ks);
- }
- return;
-
-used:
- to_link->len = 0;
- to_link->data = 0;
- /* for debugging, you can add an ASSERT(0); here to trigger an abort */
-}
/*
* Called by the top-level event loop.
*
@@ -3444,11 +3335,11 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link,
if (i == TM_ACTIVE && ks_lame->state >= S_GENERATED_KEYS
&& !multi->opt.single_session)
{
- move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true, to_link);
+ move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true);
}
else
{
- reset_session(multi, session, to_link);
+ reset_session(multi, session);
}
}
}
@@ -3511,7 +3402,7 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link,
*/
if (lame_duck_must_die(&multi->session[TM_LAME_DUCK], wakeup))
{
- tls_session_free(&multi->session[TM_LAME_DUCK], true, to_link);
+ tls_session_free(&multi->session[TM_LAME_DUCK], true);
msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: killed expiring key");
}
@@ -3526,7 +3417,7 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link,
*/
if (TLS_AUTHENTICATED(multi, &multi->session[TM_INITIAL].key[KS_PRIMARY]))
{
- move_session(multi, TM_ACTIVE, TM_INITIAL, true, to_link);
+ move_session(multi, TM_ACTIVE, TM_INITIAL, true);
tas = tls_authentication_status(multi);
msg(D_TLS_DEBUG_LOW,
"TLS: tls_multi_process: initial untrusted "
@@ -3938,7 +3829,7 @@ tls_pre_decrypt(struct tls_multi *multi, const struct link_socket_actual *from,
goto error;
}
- key_state_soft_reset(session, NULL);
+ key_state_soft_reset(session);
dmsg(D_TLS_DEBUG, "TLS: received P_CONTROL_SOFT_RESET_V1 s=%d sid=%s", i,
session_id_print(&sid, &gc));
@@ -627,6 +627,14 @@ struct tls_multi
*/
struct link_socket_actual to_link_addr;
+ /**
+ * Holds the outgoing control packet returned by tls_multi_process.
+ * It belongs to the tls_multi and not to a session or key state, so
+ * it stays valid when these are freed or moved before the packet is
+ * written to the link.
+ */
+ struct buffer to_link_buf;
+
int n_sessions; /**< Number of sessions negotiated thus
* far. */
enum multi_status multi_state;
@@ -163,10 +163,14 @@ tls_wrap_control(struct tls_wrap_ctx *ctx, uint8_t header, struct buffer *buf,
}
void
-write_control_auth(struct tls_session *session, struct key_state *ks, struct buffer *buf,
- struct link_socket_actual **to_link_addr, int opcode, int max_ack,
- bool prepend_ack)
+write_control_auth(struct tls_session *session, struct key_state *ks, const struct buffer *payload,
+ struct buffer *out, struct link_socket_actual **to_link_addr, int opcode,
+ int max_ack, bool prepend_ack)
{
+ /* Work on a copy so that tls_wrap_control can point it at another
+ * buffer without modifying the caller's buffer */
+ struct buffer buf = *payload;
+
ASSERT(ks->key_id >= 0 && ks->key_id <= P_KEY_ID_MASK);
ASSERT(opcode >= 0 && opcode <= P_LAST_OPCODE);
uint8_t header = (uint8_t)(ks->key_id | (opcode << P_OPCODE_SHIFT));
@@ -181,14 +185,20 @@ write_control_auth(struct tls_session *session, struct key_state *ks, struct buf
}
ASSERT(link_socket_actual_defined(&ks->remote_addr));
- ASSERT(reliable_ack_write(ks->rec_ack, ks->lru_acks, buf, &ks->session_id_remote, max_ack,
+ ASSERT(reliable_ack_write(ks->rec_ack, ks->lru_acks, &buf, &ks->session_id_remote, max_ack,
prepend_ack));
msg(D_TLS_DEBUG, "%s(): %s", __func__, packet_opcode_name(opcode));
- tls_wrap_control(tls_session_get_tls_wrap(session, ks->key_id), header, buf,
+ tls_wrap_control(tls_session_get_tls_wrap(session, ks->key_id), header, &buf,
&session->session_id);
+ /* buf now points into memory owned by ks or session (the payload's
+ * buffer or the tls_wrap work buffer). Either can be freed before
+ * the packet is written to the link, so copy the packet to out. */
+ ASSERT(buf_init(out, buf.offset));
+ ASSERT(buf_copy(out, &buf));
+
*to_link_addr = &ks->remote_addr;
}
@@ -185,10 +185,29 @@ struct session_id calculate_session_id_hmac(struct session_id client_sid,
bool check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state, const struct openvpn_sockaddr *from,
const uint8_t *key, int handwindow, bool pkt_is_ack);
-/*
- * Write a control channel authentication record.
+/**
+ * Write a control channel packet: add the ACKs, the opcode and session id
+ * header and the tls-auth/tls-crypt wrapping to \c payload.
+ *
+ * @param session the TLS session the packet belongs to
+ * @param ks the key state the packet belongs to
+ * @param payload the packet payload. Its data is left
+ * unchanged, but the space around it in its
+ * buffer is used to build the packet.
+ * @param out buffer the finished packet is written to, at
+ * the offset it was built at. It must be owned
+ * by the caller, not share memory with \c payload
+ * or the tls_wrap work buffers, and have room for
+ * the packet at that offset (a buffer of
+ * BUF_SIZE() of the session's frame does).
+ * @param to_link_addr set to the address the packet is sent to
+ * @param opcode opcode of the packet
+ * @param max_ack maximum number of ACKs to add
+ * @param prepend_ack add the ACKs in front of the payload instead
+ * of writing them into it
*/
-void write_control_auth(struct tls_session *session, struct key_state *ks, struct buffer *buf,
+void write_control_auth(struct tls_session *session, struct key_state *ks,
+ const struct buffer *payload, struct buffer *out,
struct link_socket_actual **to_link_addr, int opcode, int max_ack,
bool prepend_ack);
@@ -741,6 +741,108 @@ test_wkc_not_appended_without_key(void **ut_state)
free_tas(&tas);
}
+/* write_control_auth() writes the finished packet to the out buffer of the
+ * caller, so the packet stays valid when the buffers of the session and key
+ * state are reused or freed before it is sent. */
+static void
+check_write_control_auth_out_buf(struct tls_wrap_ctx *wrap, struct tls_wrap_ctx *peer_wrap)
+{
+ struct tls_options opt = { 0 };
+ opt.server = true;
+
+ struct tls_session session = { 0 };
+ session.opt = &opt;
+ session.tls_wrap = *wrap;
+ session.session_id = (struct session_id){ { 1, 2, 3, 4, 5, 6, 7, 8 } };
+
+ struct reliable_ack rec_ack = { .len = 1, .packet_id = { 7 } };
+ struct reliable_ack lru_acks = { 0 };
+ struct key_state ks = { 0 };
+ ks.rec_ack = &rec_ack;
+ ks.lru_acks = &lru_acks;
+ ks.session_id_remote = (struct session_id){ { 8, 7, 6, 5, 4, 3, 2, 1 } };
+ ks.remote_addr.dest.addr.sa.sa_family = AF_INET;
+ ks.remote_addr.dest.addr.in4.sin_addr.s_addr = htonl(0x01020304);
+
+ const uint8_t data[] = "control channel payload";
+ struct buffer payload = alloc_buf(1600);
+ struct buffer out = alloc_buf(1600);
+
+ /* a control packet with the ACK in front of its payload, as sent from
+ * the reliability layer, and a dedicated ACK packet */
+ for (int i = 0; i < 2; i++)
+ {
+ const bool dedicated_ack = (i == 1);
+ const size_t len = dedicated_ack ? 0 : sizeof(data);
+
+ assert_true(buf_init(&payload, 200));
+ assert_true(buf_write(&payload, data, len));
+
+ struct link_socket_actual *to_link_addr = NULL;
+ write_control_auth(&session, &ks, &payload, &out, &to_link_addr,
+ dedicated_ack ? P_ACK_V1 : P_CONTROL_V1, RELIABLE_ACK_SIZE,
+ !dedicated_ack);
+
+ assert_ptr_equal(to_link_addr, &ks.remote_addr);
+ assert_int_equal(BLEN(&payload), len);
+ assert_memory_equal(BPTR(&payload), data, len);
+ assert_true(BLENZ(&out) > len);
+
+ /* the packet is still intact after the session buffers are overwritten */
+ memset(payload.data, 0x55, payload.capacity);
+ if (session.tls_wrap.work.data)
+ {
+ memset(session.tls_wrap.work.data, 0x55, session.tls_wrap.work.capacity);
+ }
+
+ struct link_socket_actual from = { 0 };
+ assert_true(read_control_auth(&out, peer_wrap, &from, &opt));
+
+ struct reliable_ack ack;
+ struct session_id sid;
+ assert_true(reliable_ack_parse(&out, &ack, &sid));
+ assert_int_equal(ack.len, 1);
+ assert_int_equal(ack.packet_id[0], 7);
+ assert_memory_equal(&sid, &ks.session_id_remote, SID_SIZE);
+ assert_int_equal(BLEN(&out), len);
+ assert_memory_equal(BPTR(&out), data, len);
+ }
+
+ free_buf(&out);
+ free_buf(&payload);
+}
+
+static void
+test_write_control_auth_out_buf(void **ut_state)
+{
+ now = 0x22446688;
+
+ struct tls_wrap_ctx none = { .mode = TLS_WRAP_NONE };
+ check_write_control_auth_out_buf(&none, &none);
+
+ struct tls_auth_standalone tas_client = init_tas_auth(KEY_DIRECTION_INVERSE);
+ struct tls_auth_standalone tas_server = init_tas_auth(KEY_DIRECTION_NORMAL);
+ packet_id_init(&tas_client.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0);
+ packet_id_init(&tas_server.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0);
+ reset_packet_id_send(&tas_client.tls_wrap.opt.packet_id.send);
+ check_write_control_auth_out_buf(&tas_client.tls_wrap, &tas_server.tls_wrap);
+ packet_id_free(&tas_client.tls_wrap.opt.packet_id);
+ packet_id_free(&tas_server.tls_wrap.opt.packet_id);
+ free_tas(&tas_client);
+ free_tas(&tas_server);
+
+ tas_client = init_tas_crypt(false);
+ tas_server = init_tas_crypt(true);
+ packet_id_init(&tas_client.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0);
+ packet_id_init(&tas_server.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0);
+ reset_packet_id_send(&tas_client.tls_wrap.opt.packet_id.send);
+ check_write_control_auth_out_buf(&tas_client.tls_wrap, &tas_server.tls_wrap);
+ packet_id_free(&tas_client.tls_wrap.opt.packet_id);
+ packet_id_free(&tas_server.tls_wrap.opt.packet_id);
+ free_tas(&tas_client);
+ free_tas(&tas_server);
+}
+
static void
test_extract_control_message(void **ut_state)
{
@@ -792,6 +894,7 @@ main(void)
cmocka_unit_test(test_generate_reset_packet_plain),
cmocka_unit_test(test_generate_reset_packet_tls_auth),
cmocka_unit_test(test_wkc_not_appended_without_key),
+ cmocka_unit_test(test_write_control_auth_out_buf),
cmocka_unit_test(test_extract_control_message)
};