Properly handling stream/crypto frames while tracing

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20914)

diff --git a/include/internal/quic_wire.h b/include/internal/quic_wire.h
index a80ab6bbfdcc840de6c0cf7176f1c684593503d7..a514d08d3dd0a6c2c218fd4c0fb1572b81693488 100644 (file)
--- a/include/internal/quic_wire.h
+++ b/include/internal/quic_wire.h
@@ -557,9 +557,11 @@ int ossl_quic_wire_decode_frame_stop_sending(PACKET *pkt,
  * Decodes a QUIC CRYPTO frame.
  *
  * f->data is set to point inside the packet buffer inside the PACKET, therefore
- * it is safe to access for as long as the packet buffer exists.
+ * it is safe to access for as long as the packet buffer exists. If nodata is
+ * set to 1 then reading the PACKET stops after the frame header and f->data is
+ * set to NULL.
  */
-int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
+int ossl_quic_wire_decode_frame_crypto(PACKET *pkt, int nodata,
                                        OSSL_QUIC_FRAME_CRYPTO *f);
 
 /*
@@ -573,6 +575,10 @@ int ossl_quic_wire_decode_frame_new_token(PACKET               *pkt,
 /*
  * Decodes a QUIC STREAM frame.
  *
+ * If nodata is set to 1 then reading the PACKET stops after the frame header
+ * and f->data is set to NULL. In this case f->len will also be 0 in the event
+ * that "has_explicit_len" is 0.
+ *
  * If the frame did not contain an offset field, f->offset is set to 0, as the
  * absence of an offset field is equivalent to an offset of 0.
  *
@@ -595,7 +601,7 @@ int ossl_quic_wire_decode_frame_new_token(PACKET               *pkt,
  * f->is_fin is set according to whether the frame was marked as ending the
  * stream.
  */
-int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
+int ossl_quic_wire_decode_frame_stream(PACKET *pkt, int nodata,
                                        OSSL_QUIC_FRAME_STREAM *f);
 
 /*

diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c
index 996de79bdfc79c5febbba5e8a52a0bca7f39a62c..740f95f23a670980c032b9808d4e03b6534ffd1a 100644 (file)
--- a/ssl/quic/quic_rx_depack.c
+++ b/ssl/quic/quic_rx_depack.c
@@ -196,7 +196,7 @@ static int depack_do_frame_crypto(PACKET *pkt, QUIC_CHANNEL *ch,
 
     *datalen = 0;
 
-    if (!ossl_quic_wire_decode_frame_crypto(pkt, &f)) {
+    if (!ossl_quic_wire_decode_frame_crypto(pkt, 0, &f)) {
         ossl_quic_channel_raise_protocol_error(ch,
                                                QUIC_ERR_FRAME_ENCODING_ERROR,
                                                OSSL_QUIC_FRAME_TYPE_CRYPTO,
@@ -395,7 +395,7 @@ static int depack_do_frame_stream(PACKET *pkt, QUIC_CHANNEL *ch,
 
     *datalen = 0;
 
-    if (!ossl_quic_wire_decode_frame_stream(pkt, &frame_data)) {
+    if (!ossl_quic_wire_decode_frame_stream(pkt, 0, &frame_data)) {
         ossl_quic_channel_raise_protocol_error(ch,
                                                QUIC_ERR_FRAME_ENCODING_ERROR,
                                                frame_type,

diff --git a/ssl/quic/quic_trace.c b/ssl/quic/quic_trace.c
index aa25e8f028823d98ed0c38eaf6fdbff3702da612..d84a2f65c67ea989823ac012e1bd510b78ffc488 100644 (file)
--- a/ssl/quic/quic_trace.c
+++ b/ssl/quic/quic_trace.c
@@ -110,7 +110,7 @@ static int frame_crypto(BIO *bio, PACKET *pkt)
 {
     OSSL_QUIC_FRAME_CRYPTO frame_data;
 
-    if (!ossl_quic_wire_decode_frame_crypto(pkt, &frame_data))
+    if (!ossl_quic_wire_decode_frame_crypto(pkt, 1, &frame_data))
         return 0;
 
     BIO_printf(bio, "    Offset: %lu\n", frame_data.offset);
@@ -173,12 +173,19 @@ static int frame_stream(BIO *bio, PACKET *pkt, uint64_t frame_type)
         return 0;
     }
 
-    if (!ossl_quic_wire_decode_frame_stream(pkt, &frame_data))
+    if (!ossl_quic_wire_decode_frame_stream(pkt, 1, &frame_data))
         return 0;
 
     BIO_printf(bio, "    Stream id: %lu\n", frame_data.stream_id);
     BIO_printf(bio, "    Offset: %lu\n", frame_data.offset);
-    BIO_printf(bio, "    Len: %lu\n", frame_data.len);
+    /*
+     * It would be nice to find a way of passing the implicit length through
+     * to the msg_callback. But this is not currently possible.
+     */
+    if (frame_data.has_explicit_len)
+        BIO_printf(bio, "    Len: %lu\n", frame_data.len);
+    else
+        BIO_puts(bio, "    Len: <implicit length>\n");
 
     return 1;
 }
@@ -532,6 +539,7 @@ int ossl_quic_trace(int write_p, int version, int content_type,
 
     case SSL3_RT_QUIC_FRAME_PADDING:
     case SSL3_RT_QUIC_FRAME_FULL:
+    case SSL3_RT_QUIC_FRAME_HEADER:
         {
             BIO_puts(bio, write_p ? "Sent" : "Received");
             BIO_puts(bio, " Frame: ");
@@ -545,16 +553,6 @@ int ossl_quic_trace(int write_p, int version, int content_type,
         }
         break;
 
-    case SSL3_RT_QUIC_FRAME_HEADER:
-        {
-            BIO_puts(bio, write_p ? "Sent" : "Received");
-            BIO_puts(bio, " Frame Data\n");
-
-            /* TODO(QUIC): Implement me */
-            BIO_puts(bio, "  <content skipped>\n");
-        }
-        break;
-
     default:
         /* Unrecognised content_type. We defer to SSL_trace */
         return 0;

diff --git a/ssl/quic/quic_wire.c b/ssl/quic/quic_wire.c
index 5d35c98b67d11323308633c83231576824b5fdcf..7df32c77b21f2e952b04fdb7cde3c45eb04f45e0 100644 (file)
--- a/ssl/quic/quic_wire.c
+++ b/ssl/quic/quic_wire.c
@@ -591,6 +591,7 @@ int ossl_quic_wire_decode_frame_stop_sending(PACKET *pkt,
 }
 
 int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
+                                       int nodata,
                                        OSSL_QUIC_FRAME_CRYPTO *f)
 {
     if (!expect_frame_header(pkt, OSSL_QUIC_FRAME_TYPE_CRYPTO)
@@ -599,13 +600,17 @@ int ossl_quic_wire_decode_frame_crypto(PACKET *pkt,
             || f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */)
         return 0;
 
-    if (PACKET_remaining(pkt) < f->len)
-        return 0;
+    if (nodata) {
+        f->data = NULL;
+    } else {
+        if (PACKET_remaining(pkt) < f->len)
+            return 0;
 
-    f->data = PACKET_data(pkt);
+        f->data = PACKET_data(pkt);
 
-    if (!PACKET_forward(pkt, (size_t)f->len))
-        return 0;
+        if (!PACKET_forward(pkt, (size_t)f->len))
+            return 0;
+    }
 
     return 1;
 }
@@ -633,6 +638,7 @@ int ossl_quic_wire_decode_frame_new_token(PACKET               *pkt,
 }
 
 int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
+                                       int nodata,
                                        OSSL_QUIC_FRAME_STREAM *f)
 {
     uint64_t frame_type;
@@ -658,14 +664,21 @@ int ossl_quic_wire_decode_frame_stream(PACKET *pkt,
         if (!PACKET_get_quic_vlint(pkt, &f->len))
             return 0;
     } else {
-        f->len = PACKET_remaining(pkt);
+        if (nodata)
+            f->len = 0;
+        else
+            f->len = PACKET_remaining(pkt);
     }
 
-    f->data = PACKET_data(pkt);
+    if (nodata) {
+        f->data = NULL;
+    } else {
+        f->data = PACKET_data(pkt);
 
-    if (f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */
-        || !PACKET_forward(pkt, (size_t)f->len))
-        return 0;
+        if (f->len > SIZE_MAX /* sizeof(uint64_t) > sizeof(size_t)? */
+            || !PACKET_forward(pkt, (size_t)f->len))
+            return 0;
+    }
 
     return 1;
 }

diff --git a/test/quic_txp_test.c b/test/quic_txp_test.c
index 7e5e0edc7a748df268af581f40a18818c71147fb..7842486a3f0a8bb0b044b5311627b1d2299caa5d 100644 (file)
--- a/test/quic_txp_test.c
+++ b/test/quic_txp_test.c
@@ -1337,7 +1337,7 @@ static int run_script(const struct script_op *script)
                     goto err;
                 break;
             case OSSL_QUIC_FRAME_TYPE_CRYPTO:
-                if (!TEST_true(ossl_quic_wire_decode_frame_crypto(&h.pkt, &h.frame.crypto)))
+                if (!TEST_true(ossl_quic_wire_decode_frame_crypto(&h.pkt, 0, &h.frame.crypto)))
                     goto err;
                 break;
 
@@ -1349,7 +1349,7 @@ static int run_script(const struct script_op *script)
             case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_FIN:
             case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_LEN:
             case OSSL_QUIC_FRAME_TYPE_STREAM_OFF_LEN_FIN:
-                if (!TEST_true(ossl_quic_wire_decode_frame_stream(&h.pkt, &h.frame.stream)))
+                if (!TEST_true(ossl_quic_wire_decode_frame_stream(&h.pkt, 0, &h.frame.stream)))
                     goto err;
                 break;
 

diff --git a/test/quic_wire_test.c b/test/quic_wire_test.c
index 04e287fbf7591ade9866f98fbf20d8f4b6f3d8c3..e3cd218e27543eeaf9f4b6edd7945c7fe3303870 100644 (file)
--- a/test/quic_wire_test.c
+++ b/test/quic_wire_test.c
@@ -261,7 +261,7 @@ static int encode_case_6_dec(PACKET *pkt, ossl_ssize_t fail)
 {
     OSSL_QUIC_FRAME_CRYPTO f = {0};
 
-    if (!TEST_int_eq(ossl_quic_wire_decode_frame_crypto(pkt, &f), fail < 0))
+    if (!TEST_int_eq(ossl_quic_wire_decode_frame_crypto(pkt, 0, &f), fail < 0))
         return 0;
 
     if (fail >= 0)
@@ -358,7 +358,7 @@ static int encode_case_8_dec(PACKET *pkt, ossl_ssize_t fail)
          */
         return 1;
 
-    if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, &f), fail < 0))
+    if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, 0, &f), fail < 0))
         return 0;
 
     if (fail >= 0)
@@ -413,7 +413,7 @@ static int encode_case_9_dec(PACKET *pkt, ossl_ssize_t fail)
 {
     OSSL_QUIC_FRAME_STREAM f = {0};
 
-    if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, &f), fail < 0))
+    if (!TEST_int_eq(ossl_quic_wire_decode_frame_stream(pkt, 0, &f), fail < 0))
         return 0;
 
     if (fail >= 0)