QUIC CONFORMANCE: Enforce minimal frame type encoding

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21135)

11 files changed, 142 insertions, 17 deletions

diff --git a/include/internal/packet_quic.h b/include/internal/packet_quic.h
index 7784e62c16..e75b81e422 100644
--- a/include/internal/packet_quic.h
+++ b/include/internal/packet_quic.h
@@ -40,10 +40,12 @@ __owur static ossl_inline int PACKET_get_quic_vlint(PACKET *pkt,
 /*
  * Decodes a QUIC variable-length integer in |pkt| and stores the result in
  * |data|. Unlike PACKET_get_quic_vlint, this does not advance the current
- * position.
+ * position. If was_minimal is non-NULL, *was_minimal is set to 1 if the integer
+ * was encoded using the minimal possible number of bytes and 0 otherwise.
  */
-__owur static ossl_inline int PACKET_peek_quic_vlint(PACKET *pkt,
-                                                     uint64_t *data)
+__owur static ossl_inline int PACKET_peek_quic_vlint_ex(PACKET *pkt,
+                                                        uint64_t *data,
+                                                        int *was_minimal)
 {
     size_t enclen;
 
@@ -56,9 +58,19 @@ __owur static ossl_inline int PACKET_peek_quic_vlint(PACKET *pkt,
         return 0;
 
     *data = ossl_quic_vlint_decode_unchecked(pkt->curr);
+
+    if (was_minimal != NULL)
+        *was_minimal = (enclen == ossl_quic_vlint_encode_len(*data));
+
     return 1;
 }
 
+__owur static ossl_inline int PACKET_peek_quic_vlint(PACKET *pkt,
+                                                     uint64_t *data)
+{
+    return PACKET_peek_quic_vlint_ex(pkt, data, NULL);
+}
+
 /*
  * Skips over a QUIC variable-length integer in |pkt| without decoding it.
  */
diff --git a/include/internal/quic_wire.h b/include/internal/quic_wire.h
index e9ff8e6f35..8a1ef34ead 100644
--- a/include/internal/quic_wire.h
+++ b/include/internal/quic_wire.h
@@ -493,7 +493,8 @@ int ossl_quic_wire_encode_transport_param_cid(WPACKET *wpkt,
  * position). This can be used to determine the frame type and determine which
  * frame decoding function to call.
  */
-int ossl_quic_wire_peek_frame_header(PACKET *pkt, uint64_t *type);
+int ossl_quic_wire_peek_frame_header(PACKET *pkt, uint64_t *type,
+                                     int *was_minimal);
 
 /*
  * Like ossl_quic_wire_peek_frame_header, but advances the current position
diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c
index 88a893cf24..6e2067f451 100644
--- a/ssl/quic/quic_rx_depack.c
+++ b/ssl/quic/quic_rx_depack.c
@@ -928,6 +928,7 @@ static int depack_process_frames(QUIC_CHANNEL *ch, PACKET *pkt,
     }
 
     while (PACKET_remaining(pkt) > 0) {
+        int was_minimal;
         uint64_t frame_type;
         const unsigned char *sof = NULL;
         uint64_t datalen = 0;
@@ -935,8 +936,21 @@ static int depack_process_frames(QUIC_CHANNEL *ch, PACKET *pkt,
         if (ch->msg_callback != NULL)
             sof = PACKET_data(pkt);
 
-        if (!ossl_quic_wire_peek_frame_header(pkt, &frame_type))
+        if (!ossl_quic_wire_peek_frame_header(pkt, &frame_type, &was_minimal)) {
+            ossl_quic_channel_raise_protocol_error(ch,
+                                                   QUIC_ERR_PROTOCOL_VIOLATION,
+                                                   0,
+                                                   "malformed frame header");
+            return 0;
+        }
+
+        if (!was_minimal) {
+            ossl_quic_channel_raise_protocol_error(ch,
+                                                   QUIC_ERR_PROTOCOL_VIOLATION,
+                                                   frame_type,
+                                                   "non-minimal frame type encoding");
             return 0;
+        }
 
         switch (frame_type) {
         case OSSL_QUIC_FRAME_TYPE_PING:
@@ -1211,7 +1225,7 @@ static int depack_process_frames(QUIC_CHANNEL *ch, PACKET *pkt,
             /* Unknown frame type */
             ackm_data->is_ack_eliciting = 1;
             ossl_quic_channel_raise_protocol_error(ch,
-                                                   QUIC_ERR_PROTOCOL_VIOLATION,
+                                                   QUIC_ERR_FRAME_ENCODING_ERROR,
                                                    frame_type,
                                                    "Unknown frame type received");
             return 0;
diff --git a/ssl/quic/quic_trace.c b/ssl/quic/quic_trace.c
index ed25e05ee3..5a57e675f9 100644
--- a/ssl/quic/quic_trace.c
+++ b/ssl/quic/quic_trace.c
@@ -392,7 +392,7 @@ static int trace_frame_data(BIO *bio, PACKET *pkt)
 {
     uint64_t frame_type;
 
-    if (!ossl_quic_wire_peek_frame_header(pkt, &frame_type))
+    if (!ossl_quic_wire_peek_frame_header(pkt, &frame_type, NULL))
         return 0;
 
     switch (frame_type) {
diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c
index 42329b42de..18e0c507ba 100644
--- a/ssl/quic/quic_txp.c
+++ b/ssl/quic/quic_txp.c
@@ -318,7 +318,7 @@ static int tx_helper_commit(struct tx_helper *h)
         PACKET pkt;
 
         if (!PACKET_buf_init(&pkt, h->txn.data, l)
-                || !ossl_quic_wire_peek_frame_header(&pkt, &ftype)) {
+                || !ossl_quic_wire_peek_frame_header(&pkt, &ftype, NULL)) {
             tx_helper_end(h, 0);
             return 0;
         }
diff --git a/ssl/quic/quic_wire.c b/ssl/quic/quic_wire.c
index 937d16e1c8..0545b8f956 100644
--- a/ssl/quic/quic_wire.c
+++ b/ssl/quic/quic_wire.c
@@ -439,9 +439,10 @@ int ossl_quic_wire_encode_transport_param_cid(WPACKET *wpkt,
  * QUIC Wire Format Decoding
  * =========================
  */
-int ossl_quic_wire_peek_frame_header(PACKET *pkt, uint64_t *type)
+int ossl_quic_wire_peek_frame_header(PACKET *pkt, uint64_t *type,
+                                     int *was_minimal)
 {
-    return PACKET_peek_quic_vlint(pkt, type);
+    return PACKET_peek_quic_vlint_ex(pkt, type, was_minimal);
 }
 
 int ossl_quic_wire_skip_frame_header(PACKET *pkt, uint64_t *type)
diff --git a/test/helpers/quictestlib.c b/test/helpers/quictestlib.c
index 1053a102de..fcff11727a 100644
--- a/test/helpers/quictestlib.c
+++ b/test/helpers/quictestlib.c
@@ -347,6 +347,7 @@ int qtest_check_server_transport_err(QUIC_TSERVER *qtserv, uint64_t code)
     cause = ossl_quic_tserver_get_terminate_cause(qtserv);
     if  (!TEST_ptr(cause)
             || !TEST_true(cause->remote)
+            || !TEST_false(cause->app)
             || !TEST_uint64_t_eq(cause->error_code, code))
         return 0;
 
@@ -358,6 +359,11 @@ int qtest_check_server_protocol_err(QUIC_TSERVER *qtserv)
     return qtest_check_server_transport_err(qtserv, QUIC_ERR_PROTOCOL_VIOLATION);
 }
 
+int qtest_check_server_frame_encoding_err(QUIC_TSERVER *qtserv)
+{
+    return qtest_check_server_transport_err(qtserv, QUIC_ERR_FRAME_ENCODING_ERROR);
+}
+
 void qtest_fault_free(QTEST_FAULT *fault)
 {
     if (fault == NULL)
diff --git a/test/helpers/quictestlib.h b/test/helpers/quictestlib.h
index 90e1b68c1f..8b24ab6009 100644
--- a/test/helpers/quictestlib.h
+++ b/test/helpers/quictestlib.h
@@ -75,6 +75,12 @@ int qtest_check_server_transport_err(QUIC_TSERVER *qtserv, uint64_t code);
 int qtest_check_server_protocol_err(QUIC_TSERVER *qtserv);
 
 /*
+ * Confirm the server has received a frame encoding error. Equivalent to calling
+ * qtest_check_server_transport_err with a code of QUIC_ERR_FRAME_ENCODING_ERROR
+ */
+int qtest_check_server_frame_encoding_err(QUIC_TSERVER *qtserv);
+
+/*
  * Enable tests to listen for pre-encryption QUIC packets being sent
  */
 typedef int (*qtest_fault_on_packet_plain_cb)(QTEST_FAULT *fault,
diff --git a/test/quic_txp_test.c b/test/quic_txp_test.c
index b733dcaf1c..52026d3569 100644
--- a/test/quic_txp_test.c
+++ b/test/quic_txp_test.c
@@ -1217,7 +1217,7 @@ static void skip_padding(struct helper *h)
 {
     uint64_t frame_type;
 
-    if (!ossl_quic_wire_peek_frame_header(&h->pkt, &frame_type))
+    if (!ossl_quic_wire_peek_frame_header(&h->pkt, &frame_type, NULL))
         return; /* EOF */
 
     if (frame_type == OSSL_QUIC_FRAME_TYPE_PADDING)
@@ -1296,7 +1296,7 @@ static int run_script(const struct script_op *script)
             break;
         case OPK_NEXT_FRAME:
             skip_padding(&h);
-            if (!ossl_quic_wire_peek_frame_header(&h.pkt, &h.frame_type)) {
+            if (!ossl_quic_wire_peek_frame_header(&h.pkt, &h.frame_type, NULL)) {
                 h.frame_type = UINT64_MAX;
                 break;
             }
diff --git a/test/quic_wire_test.c b/test/quic_wire_test.c
index e3cd218e27..c307643c1b 100644
--- a/test/quic_wire_test.c
+++ b/test/quic_wire_test.c
@@ -533,20 +533,29 @@ static int encode_case_12_dec(PACKET *pkt, ossl_ssize_t fail)
 {
     uint64_t max_streams_1 = 0, max_streams_2 = 0,
             frame_type_1 = 0, frame_type_2 = 0;
+    int is_minimal;
 
-    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_1),
+    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_1,
+                                                      &is_minimal),
                      fail < 0 || fail >= 1))
         return 0;
 
+    if (!TEST_true(is_minimal))
+        return 0;
+
     if (!TEST_int_eq(ossl_quic_wire_decode_frame_max_streams(pkt,
                                                              &max_streams_1),
                      fail < 0 || fail >= 3))
         return 0;
 
-    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_2),
+    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_2,
+                                                      &is_minimal),
                      fail < 0 || fail >= 4))
         return 0;
 
+    if (!TEST_true(is_minimal))
+        return 0;
+
     if (!TEST_int_eq(ossl_quic_wire_decode_frame_max_streams(pkt,
                                                              &max_streams_2),
                      fail < 0))
@@ -663,20 +672,29 @@ static int encode_case_15_dec(PACKET *pkt, ossl_ssize_t fail)
 {
     uint64_t max_streams_1 = 0, max_streams_2 = 0,
             frame_type_1 = 0, frame_type_2 = 0;
+    int is_minimal;
 
-    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_1),
+    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_1,
+                                                      &is_minimal),
                      fail < 0 || fail >= 1))
         return 0;
 
+    if (!TEST_true(is_minimal))
+        return 0;
+
     if (!TEST_int_eq(ossl_quic_wire_decode_frame_streams_blocked(pkt,
                                                                  &max_streams_1),
                      fail < 0 || fail >= 3))
         return 0;
 
-    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_2),
+    if (!TEST_int_eq(ossl_quic_wire_peek_frame_header(pkt, &frame_type_2,
+                                                      &is_minimal),
                      fail < 0 || fail >= 4))
         return 0;
 
+    if (!TEST_true(is_minimal))
+        return 0;
+
     if (!TEST_int_eq(ossl_quic_wire_decode_frame_streams_blocked(pkt,
                                                                  &max_streams_2),
                      fail < 0 || fail >= 8))
@@ -1539,11 +1557,78 @@ err:
     return testresult;
 }
 
+/* is_minimal=0 test */
+static const unsigned char non_minimal_1[] = {
+    0x40, 0x00,
+};
+
+static const unsigned char non_minimal_2[] = {
+    0x40, 0x3F,
+};
+
+static const unsigned char non_minimal_3[] = {
+    0x80, 0x00, 0x00, 0x00,
+};
+
+static const unsigned char non_minimal_4[] = {
+    0x80, 0x00, 0x3F, 0xFF,
+};
+
+static const unsigned char non_minimal_5[] = {
+    0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+
+static const unsigned char non_minimal_6[] = {
+    0xC0, 0x00, 0x00, 0x00, 0x3F, 0xFF, 0xFF, 0xFF
+};
+
+static const unsigned char *const non_minimal[] = {
+    non_minimal_1,
+    non_minimal_2,
+    non_minimal_3,
+    non_minimal_4,
+    non_minimal_5,
+    non_minimal_6,
+};
+
+static const size_t non_minimal_len[] = {
+    OSSL_NELEM(non_minimal_1),
+    OSSL_NELEM(non_minimal_2),
+    OSSL_NELEM(non_minimal_3),
+    OSSL_NELEM(non_minimal_4),
+    OSSL_NELEM(non_minimal_5),
+    OSSL_NELEM(non_minimal_6),
+};
+
+static int test_wire_minimal(int idx)
+{
+    int testresult = 0;
+    int is_minimal;
+    uint64_t frame_type;
+    PACKET pkt;
+
+    if (!TEST_true(PACKET_buf_init(&pkt, non_minimal[idx],
+                                   non_minimal_len[idx])))
+        goto err;
+
+    if (!TEST_true(ossl_quic_wire_peek_frame_header(&pkt, &frame_type,
+                                                    &is_minimal)))
+        goto err;
+
+    if (!TEST_false(is_minimal))
+        goto err;
+
+    testresult = 1;
+err:
+    return testresult;
+}
+
 int setup_tests(void)
 {
     ADD_ALL_TESTS(test_wire_encode,     OSSL_NELEM(encode_cases));
     ADD_ALL_TESTS(test_wire_ack,        OSSL_NELEM(ack_cases));
     ADD_ALL_TESTS(test_wire_pkt_hdr_pn, OSSL_NELEM(pn_tests));
     ADD_TEST(test_wire_retry_integrity_tag);
+    ADD_ALL_TESTS(test_wire_minimal,    OSSL_NELEM(non_minimal_len));
     return 1;
 }
diff --git a/test/quicfaultstest.c b/test/quicfaultstest.c
index 4d1cae01e4..406b09a9ea 100644
--- a/test/quicfaultstest.c
+++ b/test/quicfaultstest.c
@@ -155,7 +155,7 @@ static int test_unknown_frame(void)
         goto err;
 #endif
 
-    if (!TEST_true(qtest_check_server_protocol_err(qtserv)))
+    if (!TEST_true(qtest_check_server_frame_encoding_err(qtserv)))
         goto err;
 
     testresult = 1;