Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 16 Feb 2022 16:52:44 GMT
From:      Mark Johnston <markj@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: e3b852f99bc5 - stable/13 - ktls: Disallow transmitting empty frames outside of TLS 1.0/CBC mode
Message-ID:  <202202161652.21GGqi6j068526@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=e3b852f99bc5f51e99f8fda67e71ab3639f60f4e

commit e3b852f99bc5f51e99f8fda67e71ab3639f60f4e
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2022-02-08 17:36:41 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-02-16 16:52:31 +0000

    ktls: Disallow transmitting empty frames outside of TLS 1.0/CBC mode
    
    There was nothing preventing one from sending an empty fragment on an
    arbitrary KTLS TX-enabled socket, but ktls_frame() asserts that this
    could not happen.  Though the transmit path handles this case for TLS
    1.0 with AES-CBC, we should be strict and allow empty fragments only in
    modes where it is explicitly allowed.
    
    Modify sosend_generic() to reject writes to a KTLS-enabled socket if the
    number of data bytes is zero, so that userspace cannot trigger the
    aforementioned assertion.
    
    Add regression tests to exercise this case.
    
    Reported by:    syzkaller
    Reviewed by:    gallatin, jhb
    Sponsored by:   The FreeBSD Foundation
    
    (cherry picked from commit 5de79eeddb9de079d108d1312148bcbefce45c27)
---
 sys/kern/uipc_ktls.c       | 18 ++++++++++++------
 sys/kern/uipc_socket.c     |  5 +++++
 sys/sys/ktls.h             |  1 +
 tests/sys/kern/ktls_test.c | 31 +++++++++++++++++++++++--------
 4 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/sys/kern/uipc_ktls.c b/sys/kern/uipc_ktls.c
index db4ab69e06cf..50bd0a0df21e 100644
--- a/sys/kern/uipc_ktls.c
+++ b/sys/kern/uipc_ktls.c
@@ -1568,19 +1568,18 @@ ktls_frame(struct mbuf *top, struct ktls_session *tls, int *enq_cnt,
 		 * All mbufs in the chain should be TLS records whose
 		 * payload does not exceed the maximum frame length.
 		 *
-		 * Empty TLS records are permitted when using CBC.
+		 * Empty TLS 1.0 records are permitted when using CBC.
 		 */
-		KASSERT(m->m_len <= maxlen &&
-		    (tls->params.cipher_algorithm == CRYPTO_AES_CBC ?
-		    m->m_len >= 0 : m->m_len > 0),
-		    ("ktls_frame: m %p len %d\n", m, m->m_len));
+		KASSERT(m->m_len <= maxlen && m->m_len >= 0 &&
+		    (m->m_len > 0 || ktls_permit_empty_frames(tls)),
+		    ("ktls_frame: m %p len %d", m, m->m_len));
 
 		/*
 		 * TLS frames require unmapped mbufs to store session
 		 * info.
 		 */
 		KASSERT((m->m_flags & M_EXTPG) != 0,
-		    ("ktls_frame: mapped mbuf %p (top = %p)\n", m, top));
+		    ("ktls_frame: mapped mbuf %p (top = %p)", m, top));
 
 		tls_len = m->m_len;
 
@@ -1674,6 +1673,13 @@ ktls_frame(struct mbuf *top, struct ktls_session *tls, int *enq_cnt,
 	}
 }
 
+bool
+ktls_permit_empty_frames(struct ktls_session *tls)
+{
+	return (tls->params.cipher_algorithm == CRYPTO_AES_CBC &&
+	    tls->params.tls_vminor == TLS_MINOR_VER_ZERO);
+}
+
 void
 ktls_check_rx(struct sockbuf *sb)
 {
diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c
index 1fd63640988b..5a3748eb8cf5 100644
--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -1604,6 +1604,11 @@ sosend_generic(struct socket *so, struct sockaddr *addr, struct uio *uio,
 				atomic = 1;
 			}
 		}
+
+		if (resid == 0 && !ktls_permit_empty_frames(tls)) {
+			error = EINVAL;
+			goto release;
+		}
 	}
 #endif
 
diff --git a/sys/sys/ktls.h b/sys/sys/ktls.h
index e9cae34035c6..0e16fd5d9d1e 100644
--- a/sys/sys/ktls.h
+++ b/sys/sys/ktls.h
@@ -223,6 +223,7 @@ int ktls_enable_tx(struct socket *so, struct tls_enable *en);
 void ktls_destroy(struct ktls_session *tls);
 void ktls_frame(struct mbuf *m, struct ktls_session *tls, int *enqueue_cnt,
     uint8_t record_type);
+bool ktls_permit_empty_frames(struct ktls_session *tls);
 void ktls_seq(struct sockbuf *sb, struct mbuf *m);
 void ktls_enqueue(struct mbuf *m, struct socket *so, int page_count);
 void ktls_enqueue_to_free(struct mbuf *m);
diff --git a/tests/sys/kern/ktls_test.c b/tests/sys/kern/ktls_test.c
index b2c722914854..a994f675711b 100644
--- a/tests/sys/kern/ktls_test.c
+++ b/tests/sys/kern/ktls_test.c
@@ -1057,9 +1057,19 @@ test_ktls_transmit_empty_fragment(struct tls_enable *en, uint64_t seqno)
 	fd_set_blocking(sockets[0]);
 	fd_set_blocking(sockets[1]);
 
-	/* A write of zero bytes should send an empty fragment. */
+	/*
+	 * A write of zero bytes should send an empty fragment only for
+	 * TLS 1.0, otherwise an error should be raised.
+	 */
 	rv = write(sockets[1], NULL, 0);
-	ATF_REQUIRE(rv == 0);
+	if (rv == 0) {
+		ATF_REQUIRE(en->cipher_algorithm == CRYPTO_AES_CBC);
+		ATF_REQUIRE(en->tls_vminor == TLS_MINOR_VER_ZERO);
+	} else {
+		ATF_REQUIRE(rv == -1);
+		ATF_REQUIRE(errno == EINVAL);
+		goto out;
+	}
 
 	/*
 	 * First read the header to determine how much additional data
@@ -1081,6 +1091,7 @@ test_ktls_transmit_empty_fragment(struct tls_enable *en, uint64_t seqno)
 	    "read %zd decrypted bytes for an empty fragment", rv);
 	ATF_REQUIRE(record_type == TLS_RLTYPE_APP);
 
+out:
 	free(outbuf);
 
 	ATF_REQUIRE(close(sockets[1]) == 0);
@@ -1324,7 +1335,7 @@ ATF_TC_BODY(ktls_transmit_##cipher_name##_##name, tc)			\
 	ATF_TP_ADD_TC(tp, ktls_transmit_##cipher_name##_##name);
 
 #define GEN_TRANSMIT_EMPTY_FRAGMENT_TEST(cipher_name, cipher_alg,	\
-	    key_size, auth_alg)						\
+	    key_size, auth_alg, minor)					\
 ATF_TC_WITHOUT_HEAD(ktls_transmit_##cipher_name##_empty_fragment);	\
 ATF_TC_BODY(ktls_transmit_##cipher_name##_empty_fragment, tc)		\
 {									\
@@ -1333,14 +1344,14 @@ ATF_TC_BODY(ktls_transmit_##cipher_name##_empty_fragment, tc)		\
 									\
 	ATF_REQUIRE_KTLS();						\
 	seqno = random();						\
-	build_tls_enable(cipher_alg, key_size, auth_alg,		\
-	    TLS_MINOR_VER_ZERO,	seqno, &en);				\
+	build_tls_enable(cipher_alg, key_size, auth_alg, minor, seqno,	\
+	    &en);							\
 	test_ktls_transmit_empty_fragment(&en, seqno);			\
 	free_tls_enable(&en);						\
 }
 
 #define ADD_TRANSMIT_EMPTY_FRAGMENT_TEST(cipher_name, cipher_alg,	\
-	    key_size, auth_alg)						\
+	    key_size, auth_alg, minor)					\
 	ATF_TP_ADD_TC(tp, ktls_transmit_##cipher_name##_empty_fragment);
 
 #define GEN_TRANSMIT_TESTS(cipher_name, cipher_alg, key_size, auth_alg,	\
@@ -1461,7 +1472,9 @@ AES_CBC_TESTS(GEN_TRANSMIT_PADDING_TESTS);
  * Test "empty fragments" which are TLS records with no payload that
  * OpenSSL can send for TLS 1.0 connections.
  */
-TLS_10_TESTS(GEN_TRANSMIT_EMPTY_FRAGMENT_TEST);
+AES_CBC_TESTS(GEN_TRANSMIT_EMPTY_FRAGMENT_TEST);
+AES_GCM_TESTS(GEN_TRANSMIT_EMPTY_FRAGMENT_TEST);
+CHACHA20_TESTS(GEN_TRANSMIT_EMPTY_FRAGMENT_TEST);
 
 static void
 test_ktls_invalid_transmit_cipher_suite(struct tls_enable *en)
@@ -1708,7 +1721,9 @@ ATF_TP_ADD_TCS(tp)
 	AES_GCM_TESTS(ADD_TRANSMIT_TESTS);
 	CHACHA20_TESTS(ADD_TRANSMIT_TESTS);
 	AES_CBC_TESTS(ADD_TRANSMIT_PADDING_TESTS);
-	TLS_10_TESTS(ADD_TRANSMIT_EMPTY_FRAGMENT_TEST);
+	AES_CBC_TESTS(ADD_TRANSMIT_EMPTY_FRAGMENT_TEST);
+	AES_GCM_TESTS(ADD_TRANSMIT_EMPTY_FRAGMENT_TEST);
+	CHACHA20_TESTS(ADD_TRANSMIT_EMPTY_FRAGMENT_TEST);
 	INVALID_CIPHER_SUITES(ADD_INVALID_TRANSMIT_TEST);
 
 	/* Receive tests */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202202161652.21GGqi6j068526>