Release 2.8.4

Dmitri Tikhonov · Dmitri Tikhonov · commit 747be414e210 · 2020-01-06T00:47:12.000-05:00
- [HTTP3] Verify number of bytes in incoming DATA frames against
  content-length.
- [HTTP3] Stop issuing streams credits if peer stops opening QPACK
  decoder window.  This addresses a potential attack whereby client
  can cause the server to keep allocating memory.  See Security
  Considerations in the QPACK draft.
- [BUGFIX] Mini conn: don't shorten max packet size for Q050 and later.
- [BUGFIX] Init IETF connection flow controller using correct setting.
- Code cleanup and minor fixes.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,15 @@
+2020-01-06
+    - 2.8.4
+    - [HTTP3] Verify number of bytes in incoming DATA frames against
+      content-length.
+    - [HTTP3] Stop issuing streams credits if peer stops opening QPACK
+      decoder window.  This addresses a potential attack whereby client
+      can cause the server to keep allocating memory.  See Security
+      Considerations in the QPACK draft.
+    - [BUGFIX] Mini conn: don't shorten max packet size for Q050 and later.
+    - [BUGFIX] Init IETF connection flow controller using correct setting.
+    - Code cleanup and minor fixes.
+
 2019-12-30
     - 2.8.1
     - [FEATURE] Use occasional packet number gaps to detect optimistic
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -196,7 +196,10 @@ ENDIF()
 IF (CMAKE_SYSTEM_NAME STREQUAL Windows)
     FIND_LIBRARY(EVENT_LIB event)
 ELSE()
-    FIND_LIBRARY(EVENT_LIB libevent.a libevent.so)
+    FIND_LIBRARY(EVENT_LIB libevent.a)
+    IF(NOT EVENT_LIB)
+        FIND_LIBRARY(EVENT_LIB libevent.so)
+    ENDIF()
 ENDIF()
 IF(EVENT_LIB)
     MESSAGE(STATUS "Found event: ${EVENT_LIB}")
diff --git a/include/lsquic.h b/include/lsquic.h
@@ -25,7 +25,7 @@ extern "C" {
 
 #define LSQUIC_MAJOR_VERSION 2
 #define LSQUIC_MINOR_VERSION 8
-#define LSQUIC_PATCH_VERSION 1
+#define LSQUIC_PATCH_VERSION 4
 
 /**
  * Engine flags:
diff --git a/src/liblsquic/lsquic_enc_sess_ietf.c b/src/liblsquic/lsquic_enc_sess_ietf.c
@@ -1518,6 +1518,7 @@ iquic_esfi_handshake (struct enc_sess_iquic *enc_sess)
             LSQ_DEBUG("early data rejected");
             hsk_status = LSQ_HSK_0RTT_FAIL;
             goto err;
+            /* fall through */
         default:
             LSQ_DEBUG("handshake: %s", ERR_error_string(err, errbuf));
             hsk_status = LSQ_HSK_FAIL;
diff --git a/src/liblsquic/lsquic_full_conn_ietf.c b/src/liblsquic/lsquic_full_conn_ietf.c
@@ -294,6 +294,7 @@ struct ietf_full_conn
     uint64_t                    ifc_max_stream_data_uni;
     enum ifull_conn_flags       ifc_flags;
     enum send_flags             ifc_send_flags;
+    enum send_flags             ifc_delayed_send;
     struct {
         uint64_t    streams_blocked[N_SDS];
     }                           ifc_send;
@@ -920,7 +921,7 @@ ietf_full_conn_init (struct ietf_full_conn *conn,
         flags & IFC_SERVER ? &server_ver_neg : &conn->ifc_u.cli.ifcli_ver_neg,
         &conn->ifc_pub, SC_IETF|SC_NSTP|(ecn ? SC_ECN : 0));
     lsquic_cfcw_init(&conn->ifc_pub.cfcw, &conn->ifc_pub,
-                                                conn->ifc_settings->es_cfcw);
+                                        conn->ifc_settings->es_init_max_data);
     conn->ifc_pub.all_streams = lsquic_hash_create();
     if (!conn->ifc_pub.all_streams)
         return -1;
@@ -1979,34 +1980,44 @@ is_peer_initiated (const struct ietf_full_conn *conn,
 }
 
 
-#if 0
-/* XXX seems we don't need this? */
-static unsigned
-count_streams (const struct ietf_full_conn *conn, enum stream_id_type sit)
+static void
+sched_max_bidi_streams (void *conn_p)
 {
-    const struct lsquic_stream *stream;
-    struct lsquic_hash_elem *el;
-    unsigned count;
-    int peer;
+    struct ietf_full_conn *conn = conn_p;
 
-    peer = is_peer_initiated(conn, sit);
-    for (el = lsquic_hash_first(conn->ifc_pub.all_streams); el;
-                             el = lsquic_hash_next(conn->ifc_pub.all_streams))
-    {
-        stream = lsquic_hashelem_getdata(el);
-        count += (stream->id & SIT_MASK) == sit
-              && !lsquic_stream_is_closed(stream)
-                 /* When counting peer-initiated streams, do not include those
-                  * that have been reset:
-                  */
-              && !(peer && lsquic_stream_is_reset(stream));
-    }
-
-    return count;
+    conn->ifc_send_flags |= SF_SEND_MAX_STREAMS_BIDI;
+    conn->ifc_delayed_send &= ~SF_SEND_MAX_STREAMS_BIDI;
+    LSQ_DEBUG("schedule MAX_STREAMS frame for bidirectional streams (was "
+        "delayed)");
 }
 
 
-#endif
+/* Do not allow peer to open more streams while QPACK decoder stream has
+ * unsent data.
+ */
+static int
+can_give_peer_streams_credit (struct ietf_full_conn *conn, enum stream_dir sd)
+{
+    /* This logic only applies to HTTP servers. */
+    if ((conn->ifc_flags & (IFC_SERVER|IFC_HTTP)) != (IFC_SERVER|IFC_HTTP))
+        return 1;
+    /* HTTP client does not open unidirectional streams (other than the
+     * standard three), not applicable.
+     */
+    if (SD_UNI == sd)
+        return 1;
+    if (conn->ifc_delayed_send & (SF_SEND_MAX_STREAMS << sd))
+        return 0;
+    if (lsquic_qdh_arm_if_unsent(&conn->ifc_qdh, sched_max_bidi_streams, conn))
+    {
+        LSQ_DEBUG("delay sending more streams credit to peer until QPACK "
+            "decoder sends unsent data");
+        conn->ifc_delayed_send |= SF_SEND_MAX_STREAMS << sd;
+        return 0;
+    }
+    else
+        return 1;
+}
 
 
 /* Because stream IDs are distributed unevenly, it is more efficient to
@@ -2034,7 +2045,7 @@ conn_mark_stream_closed (struct ietf_full_conn *conn,
             max_allowed = conn->ifc_max_allowed_stream_id[idx] >> SIT_SHIFT;
             thresh = conn->ifc_closed_peer_streams[sd]
                                             + conn->ifc_max_streams_in[sd] / 2;
-            if (thresh >= max_allowed)
+            if (thresh >= max_allowed && can_give_peer_streams_credit(conn, sd))
             {
                 LSQ_DEBUG("closed incoming %sdirectional streams reached "
                     "%"PRIu64", scheduled MAX_STREAMS frame",
diff --git a/src/liblsquic/lsquic_handshake.c b/src/liblsquic/lsquic_handshake.c
@@ -4216,7 +4216,7 @@ gquic2_esf_decrypt_packet (enc_session_t *enc_session_p,
         goto err;
     }
 
-    /* Bits 2 and 3 are not set and don't need to be check in gQUIC */
+    /* Bits 2 and 3 are not set and don't need to be checked in gQUIC */
 
     packet_in->pi_data_sz = packet_in->pi_header_sz + out_sz;
     if (packet_in->pi_flags & PI_OWN_DATA)
diff --git a/src/liblsquic/lsquic_mini_conn.c b/src/liblsquic/lsquic_mini_conn.c
@@ -546,8 +546,12 @@ process_stream_frame (struct mini_conn *mc, lsquic_packet_in_t *packet_in,
             mc->mc_flags |= MC_HAVE_NEW_HSK;
             MCHIST_APPEND(mc, MCHE_NEW_HSK);
             if (0 == stream_frame.data_frame.df_offset)
+            {
                 /* First CHLO message: update maximum packet size */
                 mc->mc_path.np_pack_size = packet_in->pi_data_sz;
+                LSQ_DEBUG("update packet size to %hu",
+                                                    mc->mc_path.np_pack_size);
+            }
         }
         else
         {
@@ -577,7 +581,7 @@ process_crypto_frame (struct mini_conn *mc, struct lsquic_packet_in *packet_in,
     {   /* This is not supported for simplicity: assume a single CRYPTO frame
          * per packet.  If this changes, we can revisit this code.
          */
-        LSQ_INFO("two handshake stream frames in single incoming packet");
+        LSQ_INFO("two CRYPTO frames in single incoming packet");
         MCHIST_APPEND(mc, MCHE_2HSK_1STREAM);
         return 0;
     }
@@ -588,8 +592,15 @@ process_crypto_frame (struct mini_conn *mc, struct lsquic_packet_in *packet_in,
         mc->mc_flags |= MC_HAVE_NEW_HSK;
         MCHIST_APPEND(mc, MCHE_NEW_HSK);
         if (0 == stream_frame.data_frame.df_offset)
+        {
             /* First CHLO message: update maximum packet size */
-            mc->mc_path.np_pack_size = packet_in->pi_data_sz;
+            mc->mc_path.np_pack_size = packet_in->pi_data_sz
+                /* Q050 and later adjust pi_data_sz of Initial packets during
+                 * decryption, here we have to add the tag length back:
+                 */
+                                        + mc->mc_conn.cn_esf_c->esf_tag_len;
+            LSQ_DEBUG("update packet size to %hu", mc->mc_path.np_pack_size);
+        }
     }
     else
     {
diff --git a/src/liblsquic/lsquic_mini_conn_ietf.c b/src/liblsquic/lsquic_mini_conn_ietf.c
@@ -1030,7 +1030,7 @@ static unsigned (*const imico_process_frames[N_QUIC_FRAMES])
     [QUIC_FRAME_ACK]                =  imico_process_ack_frame,
     [QUIC_FRAME_PING]               =  imico_process_ping_frame,
     [QUIC_FRAME_CONNECTION_CLOSE]   =  imico_process_connection_close_frame,
-    /* XXX: Some of them are invalid, while others are unexpected.  We treat
+    /* Some of them are invalid, while others are unexpected.  We treat
      * them the same: handshake cannot proceed.
      */
     [QUIC_FRAME_RST_STREAM]         =  imico_process_invalid_frame,
diff --git a/src/liblsquic/lsquic_parse_ietf_v1.c b/src/liblsquic/lsquic_parse_ietf_v1.c
@@ -172,10 +172,13 @@ write_packno (unsigned char *p, lsquic_packno_t packno,
     {
     case IQUIC_PACKNO_LEN_4:
         *p++ = packno >> 24;
+        /* fall through */
     case IQUIC_PACKNO_LEN_3:
         *p++ = packno >> 16;
+        /* fall through */
     case IQUIC_PACKNO_LEN_2:
         *p++ = packno >> 8;
+        /* fall through */
     default:
         *p++ = packno;
     }
diff --git a/src/liblsquic/lsquic_qdec_hdl.c b/src/liblsquic/lsquic_qdec_hdl.c
@@ -223,7 +223,16 @@ qdh_out_on_write (struct lsquic_stream *stream, lsquic_stream_ctx_t *ctx)
         LSQ_DEBUG("wrote %zd bytes to stream", nw);
         (void) lsquic_stream_flush(stream);
         if (lsquic_frab_list_empty(&qdh->qdh_fral))
+        {
             lsquic_stream_wantwrite(stream, 0);
+            if (qdh->qdh_on_dec_sent_func)
+            {
+                LSQ_DEBUG("buffered data written: call callback");
+                qdh->qdh_on_dec_sent_func(qdh->qdh_on_dec_sent_ctx);
+                qdh->qdh_on_dec_sent_func = NULL;
+                qdh->qdh_on_dec_sent_ctx = NULL;
+            }
+        }
     }
     else
     {
@@ -294,7 +303,7 @@ qdh_read_encoder_stream (void *ctx, const unsigned char *buf, size_t sz,
     s = lsqpack_dec_enc_in(&qdh->qdh_decoder, buf, sz);
     if (s != 0)
     {
-        LSQ_INFO("error reading decoder stream");
+        LSQ_INFO("error reading encoder stream");
         qerr = lsqpack_dec_get_err_info(&qdh->qdh_decoder);
         qdh->qdh_conn->cn_if->ci_abort_error(qdh->qdh_conn, 1,
             HEC_QPACK_DECODER_STREAM_ERROR, "Error interpreting QPACK encoder "
@@ -376,6 +385,63 @@ qdh_hblock_unblocked (void *stream_p)
 }
 
 
+struct cont_len
+{
+    unsigned long long      value;
+    int                     has;    /* 1: set, 0: not set, -1: invalid */
+};
+
+
+static void
+process_content_length (const struct qpack_dec_hdl *qdh /* for logging */,
+            struct cont_len *cl, const char *val /* not NUL-terminated */,
+                                                                unsigned len)
+{
+    char *endcl, cont_len_buf[30];
+
+    if (0 == cl->has)
+    {
+        if (len >= sizeof(cont_len_buf))
+        {
+            LSQ_DEBUG("content-length has invalid value `%.*s'",
+                                                            (int) len, val);
+            cl->has = -1;
+            return;
+        }
+        memcpy(cont_len_buf, val, len);
+        cont_len_buf[len] = '\0';
+        cl->value = strtoull(cont_len_buf, &endcl, 10);
+        if (*endcl == '\0' && !(ULLONG_MAX == cl->value && ERANGE == errno))
+        {
+            cl->has = 1;
+            LSQ_DEBUG("content length is %llu", cl->value);
+        }
+        else
+        {
+            cl->has = -1;
+            LSQ_DEBUG("content-length has invalid value `%.*s'",
+                (int) len, val);
+        }
+    }
+    else if (cl->has > 0)
+    {
+        LSQ_DEBUG("header set has two content-length: ambiguous, "
+            "turn off checking");
+        cl->has = -1;
+    }
+}
+
+
+static int
+is_content_length (const struct lsqpack_header *header)
+{
+    return ((header->qh_flags & QH_ID_SET) && header->qh_static_id == 4)
+        || (header->qh_name_len == 14 && header->qh_name[0] == 'c'
+                    && 0 == memcmp(header->qh_name + 1, "ontent-length", 13))
+        ;
+}
+
+
 static int
 qdh_supply_hset_to_stream (struct qpack_dec_hdl *qdh,
             struct lsquic_stream *stream, struct lsqpack_header_list *qlist)
@@ -387,6 +453,7 @@ qdh_supply_hset_to_stream (struct qpack_dec_hdl *qdh,
     int push_promise;
     unsigned i;
     void *hset;
+    struct cont_len cl;
 
     push_promise = lsquic_stream_header_is_pp(stream);
     hset = hset_if->hsi_create_header_set(qdh->qdh_hsi_ctx, push_promise);
@@ -398,6 +465,7 @@ qdh_supply_hset_to_stream (struct qpack_dec_hdl *qdh,
 
     LSQ_DEBUG("got header set for stream %"PRIu64, stream->id);
 
+    cl.has = 0;
     for (i = 0; i < qlist->qhl_count; ++i)
     {
         header = qlist->qhl_headers[i];
@@ -412,6 +480,9 @@ qdh_supply_hset_to_stream (struct qpack_dec_hdl *qdh,
             LSQ_INFO("header process returned non-OK code %u", (unsigned) st);
             goto err;
         }
+        if (is_content_length(header))
+            process_content_length(qdh, &cl, header->qh_value,
+                                                        header->qh_value_len);
     }
 
     lsqpack_dec_destroy_header_list(qlist);
@@ -434,6 +505,8 @@ qdh_supply_hset_to_stream (struct qpack_dec_hdl *qdh,
         goto err;
     LSQ_DEBUG("converted qlist to hset and gave it to stream %"PRIu64,
                                                                 stream->id);
+    if (cl.has > 0)
+        (void) lsquic_stream_verify_len(stream, cl.value);
     return 0;
 
   err:
@@ -590,3 +663,24 @@ lsquic_qdh_cancel_stream (struct qpack_dec_hdl *qdh,
         lsquic_qdh_unref_stream(qdh, stream);
     }
 }
+
+
+int
+lsquic_qdh_arm_if_unsent (struct qpack_dec_hdl *qdh, void (*func)(void *),
+                                                                    void *ctx)
+{
+    size_t bytes;
+
+    /* Use size of a single frab list buffer as an arbitrary threshold */
+    bytes = lsquic_frab_list_size(&qdh->qdh_fral);
+    if (bytes <= qdh->qdh_fral.fl_buf_size)
+        return 0;
+    else
+    {
+        LSQ_DEBUG("have %zu bytes of unsent QPACK decoder stream data: set "
+            "up callback", bytes);
+        qdh->qdh_on_dec_sent_func = func;
+        qdh->qdh_on_dec_sent_ctx  = ctx;
+        return 1;
+    }
+}
diff --git a/src/liblsquic/lsquic_qdec_hdl.h b/src/liblsquic/lsquic_qdec_hdl.h
@@ -31,6 +31,8 @@ struct qpack_dec_hdl
                             *qdh_enpub;
     struct http1x_ctor_ctx   qdh_h1x_ctor_ctx;
     void                    *qdh_hsi_ctx;
+    void                   (*qdh_on_dec_sent_func)(void *);
+    void                    *qdh_on_dec_sent_ctx;
 };
 
 int
@@ -67,6 +69,9 @@ void
 lsquic_qdh_cancel_stream (struct qpack_dec_hdl *,
                                                 struct lsquic_stream *);
 
+int
+lsquic_qdh_arm_if_unsent (struct qpack_dec_hdl *, void (*)(void *), void *);
+
 extern const struct lsquic_stream_if *const lsquic_qdh_enc_sm_in_if;
 extern const struct lsquic_stream_if *const lsquic_qdh_dec_sm_out_if;
 
diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c
diff --git a/src/liblsquic/lsquic_stream.h b/src/liblsquic/lsquic_stream.h
diff --git a/src/liblsquic/lsquic_varint.c b/src/liblsquic/lsquic_varint.c
diff --git a/test/test_cert.c b/test/test_cert.c

Original file line number	Diff line number	Diff line change
`@@ -4216,7 +4216,7 @@ gquic2_esf_decrypt_packet (enc_session_t *enc_session_p,`
`4216`	`4216`	`goto err;`
`4217`	`4217`	`}`
`4218`	`4218`
`4219`		`- /* Bits 2 and 3 are not set and don't need to be check in gQUIC */`
	`4219`	`+ /* Bits 2 and 3 are not set and don't need to be checked in gQUIC */`
`4220`	`4220`
`4221`	`4221`	`packet_in->pi_data_sz = packet_in->pi_header_sz + out_sz;`
`4222`	`4222`	`if (packet_in->pi_flags & PI_OWN_DATA)`
Original file line number	Diff line number	Diff line change
`@@ -172,10 +172,13 @@ write_packno (unsigned char *p, lsquic_packno_t packno,`
`172`	`172`	`{`
`173`	`173`	`case IQUIC_PACKNO_LEN_4:`
`174`	`174`	`*p++ = packno >> 24;`
	`175`	`+ /* fall through */`
`175`	`176`	`case IQUIC_PACKNO_LEN_3:`
`176`	`177`	`*p++ = packno >> 16;`
	`178`	`+ /* fall through */`
`177`	`179`	`case IQUIC_PACKNO_LEN_2:`
`178`	`180`	`*p++ = packno >> 8;`
	`181`	`+ /* fall through */`
`179`	`182`	`default:`
`180`	`183`	`*p++ = packno;`
`181`	`184`	`}`