Release 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.
This commit is contained in:
Dmitri Tikhonov 2020-01-06 00:47:12 -05:00
parent 3f2ab3517e
commit 747be414e2
15 changed files with 286 additions and 59 deletions

View file

@ -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

View file

@ -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}")

View file

@ -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:

View file

@ -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;

View file

@ -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",

View file

@ -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)

View file

@ -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
{

View file

@ -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,

View file

@ -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;
}

View file

@ -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;
}
}

View file

@ -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;

View file

@ -1240,6 +1240,26 @@ read_uh (struct lsquic_stream *stream,
}
static void
verify_cl_on_fin (struct lsquic_stream *stream)
{
struct lsquic_conn *lconn;
/* The rules in RFC7230, Section 3.3.2 are a bit too intricate. We take
* a simple approach and verify content-length only when there was any
* payload at all.
*/
if (stream->sm_data_in != 0 && stream->sm_cont_len != stream->sm_data_in)
{
lconn = stream->conn_pub->lconn;
lconn->cn_if->ci_abort_error(lconn, 1, HEC_GENERAL_PROTOCOL_ERROR,
"number of bytes in DATA frames of stream %"PRIu64" is %llu, "
"while content-length specified of %llu", stream->id,
stream->sm_data_in, stream->sm_cont_len);
}
}
static void
stream_consumed_bytes (struct lsquic_stream *stream)
{
@ -1320,6 +1340,8 @@ read_data_frames (struct lsquic_stream *stream, int do_filtering,
if (fin)
{
stream->stream_flags |= STREAM_FIN_REACHED;
if (stream->sm_bflags & SMBF_VERIFY_CL)
verify_cl_on_fin(stream);
goto end_while;
}
}
@ -3169,7 +3191,8 @@ update_buffered_hq_frames (struct lsquic_stream *stream, size_t len,
{
assert(avail >= 3);
shf = stream_activate_hq_frame(stream, cur_off, HQFT_DATA, 0, len);
/* XXX malloc can fail */
if (!shf)
return 0;
if (len > stream_hq_frame_end(shf) - cur_off)
len = stream_hq_frame_end(shf) - cur_off;
extendable = 0;
@ -4118,6 +4141,23 @@ lsquic_stream_header_is_trailer (const struct lsquic_stream *stream)
}
static void
verify_cl_on_new_data_frame (struct lsquic_stream *stream,
struct hq_filter *filter)
{
struct lsquic_conn *lconn;
stream->sm_data_in += filter->hqfi_left;
if (stream->sm_data_in > stream->sm_cont_len)
{
lconn = stream->conn_pub->lconn;
lconn->cn_if->ci_abort_error(lconn, 1, HEC_GENERAL_PROTOCOL_ERROR,
"number of bytes in DATA frames of stream %"PRIu64" exceeds "
"content-length limit of %llu", stream->id, stream->sm_cont_len);
}
}
static size_t
hq_read (void *ctx, const unsigned char *buf, size_t sz, int fin)
{
@ -4160,7 +4200,11 @@ hq_read (void *ctx, const unsigned char *buf, size_t sz, int fin)
if (filter->hqfi_left > 0)
{
if (filter->hqfi_type == HQFT_DATA)
{
if (stream->sm_bflags & SMBF_VERIFY_CL)
verify_cl_on_new_data_frame(stream, filter);
goto end;
}
else if (filter->hqfi_type == HQFT_PUSH_PROMISE)
{
if (stream->sm_bflags & SMBF_SERVER)
@ -4716,6 +4760,7 @@ pp_reader_size (void *lsqr_ctx)
case PPWS_ID6:
case PPWS_ID7:
size += 8 - promise->pp_write_state;
/* fall-through */
case PPWS_PFX0:
++size;
/* fall-through */
@ -4836,3 +4881,21 @@ lsquic_stream_push_promise (struct lsquic_stream *stream,
return -1;
}
}
int
lsquic_stream_verify_len (struct lsquic_stream *stream,
unsigned long long cont_len)
{
if ((stream->sm_bflags & (SMBF_IETF|SMBF_USE_HEADERS))
== (SMBF_IETF|SMBF_USE_HEADERS))
{
stream->sm_cont_len = cont_len;
stream->sm_bflags |= SMBF_VERIFY_CL;
LSQ_DEBUG("will verify that incoming DATA frames have %llu bytes",
cont_len);
return 0;
}
else
return -1;
}

View file

@ -181,7 +181,8 @@ enum stream_b_flags
SMBF_RW_ONCE = 1 << 6, /* When set, read/write events are dispatched once per call */
SMBF_CONN_LIMITED = 1 << 7,
SMBF_HEADERS = 1 << 8, /* Headers stream */
#define N_SMBF_FLAGS 9
SMBF_VERIFY_CL = 1 << 9, /* Verify content-length (stored in sm_cont_len) */
#define N_SMBF_FLAGS 10
};
@ -314,6 +315,13 @@ struct lsquic_stream
uint64_t sm_last_frame_off;
/* Content length specified in incoming `content-length' header field.
* Used to verify size of DATA frames.
*/
unsigned long long sm_cont_len;
/* Sum of bytes in all incoming DATA frames. Used for verification. */
unsigned long long sm_data_in;
/* How much data there is in sm_header_block and how much of it has been
* sent:
*/
@ -599,4 +607,7 @@ lsquic_stream_header_is_pp (const struct lsquic_stream *);
int
lsquic_stream_header_is_trailer (const struct lsquic_stream *);
int
lsquic_stream_verify_len (struct lsquic_stream *, unsigned long long);
#endif

View file

@ -74,8 +74,9 @@ lsquic_varint_read_nb (const unsigned char **pp, const unsigned char *end,
return 0;
case 1:
state->val = (*p++ & VINT_MASK) << 8;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1000; break; }
/* fall through */
case 1000:
state->val |= *p++;
*pp = p;
return 0;
@ -91,14 +92,17 @@ lsquic_varint_read_nb (const unsigned char **pp, const unsigned char *end,
return 0;
}
state->val = (*p++ & VINT_MASK) << 24;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1001; break; }
/* fall through */
case 1001:
state->val |= *p++ << 16;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1002; break; }
/* fall through */
case 1002:
state->val |= *p++ << 8;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1003; break; }
/* fall through */
case 1003:
state->val |= *p++;
*pp = p;
return 0;
@ -114,26 +118,33 @@ lsquic_varint_read_nb (const unsigned char **pp, const unsigned char *end,
return 0;
}
state->val = (uint64_t) (*p++ & VINT_MASK) << 56;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1004; break; }
/* fall through */
case 1004:
state->val |= (uint64_t) *p++ << 48;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1005; break; }
/* fall through */
case 1005:
state->val |= (uint64_t) *p++ << 40;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1006; break; }
/* fall through */
case 1006:
state->val |= (uint64_t) *p++ << 32;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1007; break; }
/* fall through */
case 1007:
state->val |= (uint64_t) *p++ << 24;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1008; break; }
/* fall through */
case 1008:
state->val |= (uint64_t) *p++ << 16;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1009; break; }
/* fall through */
case 1009:
state->val |= (uint64_t) *p++ << 8;
if (p >= end) { state->pos = __LINE__ + 1; break; }
case __LINE__:
if (p >= end) { state->pos = 1010; break; }
/* fall through */
case 1010:
state->val |= *p++;
*pp = p;
return 0;

View file

@ -111,9 +111,11 @@ load_cert (struct lsquic_hash *certs, const char *optarg)
const int was = SSL_CTX_set_session_cache_mode(cert->ce_ssl_ctx, 1);
LSQ_DEBUG("set SSL session cache mode to 1 (was: %d)", was);
lsquic_hash_insert(certs, cert->ce_sni, strlen(cert->ce_sni), cert,
&cert->ce_hash_el);
rv = 0;
if (lsquic_hash_insert(certs, cert->ce_sni, strlen(cert->ce_sni), cert,
&cert->ce_hash_el))
rv = 0;
else
LSQ_WARN("cannot insert cert for %s into hash table", cert->ce_sni);
end:
free(sni);