Skip to content

Commit

Permalink
QUIC: Miscellaneous error handling updates
Browse files Browse the repository at this point in the history 
Raise errors when appropriate.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21700)
  • Loading branch information
@t8m
t8m committed Aug 23, 2023
1 parent cb19528 commit 9601484
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 34 deletions.
30 changes: 19 additions & 11 deletions ssl/quic/quic_channel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ static int gen_rand_conn_id(OSSL_LIB_CTX *libctx, size_t len, QUIC_CONN_ID *cid)
cid->id_len = (unsigned char)len;

if (RAND_bytes_ex(libctx, cid->id, len, len * 8) != 1) {
ERR_raise(ERR_LIB_SSL, ERR_R_RAND_LIB);
cid->id_len = 0;
return 0;
}
Expand Down Expand Up @@ -1264,8 +1265,11 @@ static int ch_on_transport_params(const unsigned char *params,
if (ch->got_remote_transport_params)
goto malformed;

if (!PACKET_buf_init(&pkt, params, params_len))
if (!PACKET_buf_init(&pkt, params, params_len)) {
ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_INTERNAL_ERROR, 0,
"internal error (packet buf init)");
return 0;
}

while (PACKET_remaining(&pkt) > 0) {
if (!ossl_quic_wire_peek_transport_param(&pkt, &id))
Expand Down Expand Up @@ -1447,7 +1451,7 @@ static int ch_on_transport_params(const unsigned char *params,
if (got_max_ack_delay) {
/* must not appear more than once */
reason = TP_REASON_DUP("MAX_ACK_DELAY");
return 0;
goto malformed;
}

if (!ossl_quic_wire_decode_transport_param_int(&pkt, &id, &v)
Expand All @@ -1467,7 +1471,7 @@ static int ch_on_transport_params(const unsigned char *params,
if (got_initial_max_streams_bidi) {
/* must not appear more than once */
reason = TP_REASON_DUP("INITIAL_MAX_STREAMS_BIDI");
return 0;
goto malformed;
}

if (!ossl_quic_wire_decode_transport_param_int(&pkt, &id, &v)
Expand Down Expand Up @@ -2202,9 +2206,11 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch)
/* Malformed retry packet, ignore. */
return;

ch_retry(ch, ch->qrx_pkt->hdr->data,
ch->qrx_pkt->hdr->len - QUIC_RETRY_INTEGRITY_TAG_LEN,
&ch->qrx_pkt->hdr->src_conn_id);
if (!ch_retry(ch, ch->qrx_pkt->hdr->data,
ch->qrx_pkt->hdr->len - QUIC_RETRY_INTEGRITY_TAG_LEN,
&ch->qrx_pkt->hdr->src_conn_id))
ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_INTERNAL_ERROR,
0, "handling retry packet");
break;

case QUIC_PKT_TYPE_0RTT:
Expand Down Expand Up @@ -2449,7 +2455,7 @@ static int ch_tx(QUIC_CHANNEL *ch)
* be transmitted for this reason.
*/
ossl_quic_channel_raise_protocol_error(ch, QUIC_ERR_INTERNAL_ERROR, 0,
"internal error");
"internal error (txp generate)");
break; /* Internal failure (e.g. allocation, assertion) */
}

Expand Down Expand Up @@ -2532,8 +2538,10 @@ static OSSL_TIME ch_determine_next_tick_deadline(QUIC_CHANNEL *ch)
/* Determines whether we can support a given poll descriptor. */
static int validate_poll_descriptor(const BIO_POLL_DESCRIPTOR *d)
{
if (d->type == BIO_POLL_DESCRIPTOR_TYPE_SOCK_FD && d->value.fd < 0)
if (d->type == BIO_POLL_DESCRIPTOR_TYPE_SOCK_FD && d->value.fd < 0) {
ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
return 0;
}

return 1;
}
Expand Down Expand Up @@ -2673,7 +2681,7 @@ static int ch_retry(QUIC_CHANNEL *ch,
* a SCID field that is identical to the DCID field of its initial packet."
*/
if (ossl_quic_conn_id_eq(&ch->init_dcid, retry_scid))
return 0;
return 1;

/* We change to using the SCID in the Retry packet as the DCID. */
if (!ossl_quic_tx_packetiser_set_cur_dcid(ch->txp, retry_scid))
Expand Down Expand Up @@ -2964,14 +2972,14 @@ static void free_frame_data(unsigned char *buf, size_t buf_len, void *arg)

static int ch_enqueue_retire_conn_id(QUIC_CHANNEL *ch, uint64_t seq_num)
{
BUF_MEM *buf_mem;
BUF_MEM *buf_mem = NULL;
WPACKET wpkt;
size_t l;

chan_remove_reset_token(ch, seq_num);

if ((buf_mem = BUF_MEM_new()) == NULL)
return 0;
goto err;

if (!WPACKET_init(&wpkt, buf_mem))
goto err;
Expand Down
29 changes: 21 additions & 8 deletions ssl/quic/quic_impl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ int ossl_quic_reset(SSL *s)
if (!expect_quic(s, &ctx))
return 0;

/* Not supported. */
ERR_raise(ERR_LIB_SSL, ERR_R_UNSUPPORTED);
return 0;
}

Expand Down Expand Up @@ -1052,8 +1052,10 @@ int ossl_quic_get_rpoll_descriptor(SSL *s, BIO_POLL_DESCRIPTOR *desc)
if (!expect_quic(s, &ctx))
return 0;

if (desc == NULL || ctx.qc->net_rbio == NULL)
if (desc == NULL || ctx.qc->net_rbio == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
return 0;
}

return BIO_get_rpoll_descriptor(ctx.qc->net_rbio, desc);
}
Expand All @@ -1066,8 +1068,10 @@ int ossl_quic_get_wpoll_descriptor(SSL *s, BIO_POLL_DESCRIPTOR *desc)
if (!expect_quic(s, &ctx))
return 0;

if (desc == NULL || ctx.qc->net_wbio == NULL)
if (desc == NULL || ctx.qc->net_wbio == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
return 0;
}

return BIO_get_wpoll_descriptor(ctx.qc->net_wbio, desc);
}
Expand Down Expand Up @@ -1708,12 +1712,16 @@ static SSL *quic_conn_stream_new(QCTX *ctx, uint64_t flags, int need_lock)
}

qs = ossl_quic_channel_new_stream_local(qc->ch, is_uni);
if (qs == NULL)
if (qs == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
goto err;
}

xso = create_xso_from_stream(qc, qs);
if (xso == NULL)
if (xso == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
goto err;
}

qc_touch_default_xso(qc); /* inhibits default XSO */
if (need_lock)
Expand Down Expand Up @@ -2719,6 +2727,7 @@ int ossl_quic_set_incoming_stream_policy(SSL *s, int policy,
break;

default:
ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
ret = 0;
break;
}
Expand Down Expand Up @@ -3018,9 +3027,11 @@ int ossl_quic_set_write_buffer_size(SSL *ssl, size_t size)
if (!expect_quic_with_stream_lock(ssl, /*remote_init=*/-1, &ctx))
return 0;

if (!ossl_quic_stream_has_send(ctx.xso->stream))
if (!ossl_quic_stream_has_send(ctx.xso->stream)) {
/* Called on a unidirectional receive-only stream - error. */
ERR_raise(ERR_LIB_SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
goto out;
}

if (!ossl_quic_stream_has_send_buffer(ctx.xso->stream)) {
/*
Expand All @@ -3031,8 +3042,10 @@ int ossl_quic_set_write_buffer_size(SSL *ssl, size_t size)
goto out;
}

if (!ossl_quic_sstream_set_buffer_size(ctx.xso->stream->sstream, size))
if (!ossl_quic_sstream_set_buffer_size(ctx.xso->stream->sstream, size)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
goto out;
}

ret = 1;

Expand Down Expand Up @@ -3088,7 +3101,7 @@ int ossl_quic_key_update(SSL *ssl, int update_type)
break;

default:
/* Unknown type - error. */
ERR_raise(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT);
return 0;
}

Expand Down
45 changes: 36 additions & 9 deletions ssl/quic/quic_rx_depack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,13 @@ static int depack_do_frame_crypto(PACKET *pkt, QUIC_CHANNEL *ch,
}

if (!ossl_quic_rstream_queue_data(rstream, parent_pkt,
f.offset, f.data, f.len, 0))
f.offset, f.data, f.len, 0)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_INTERNAL_ERROR,
OSSL_QUIC_FRAME_TYPE_CRYPTO,
"internal error (rstream queue)");
return 0;
}

*datalen = f.len;

Expand Down Expand Up @@ -594,18 +599,28 @@ static int depack_do_frame_stream(PACKET *pkt, QUIC_CHANNEL *ch,
frame_data.offset,
frame_data.data,
frame_data.len,
frame_data.is_fin))
frame_data.is_fin)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_INTERNAL_ERROR,
frame_type,
"internal error (rstream queue)");
return 0;
}

/*
* rs_fin will be 1 only if we can read all data up to and including the FIN
* without any gaps before it; this implies we have received all data. Avoid
* calling ossl_quic_rstream_available() where it is not necessary as it is
* more expensive.
*/
if (stream->recv_state == QUIC_RSTREAM_STATE_SIZE_KNOWN)
if (!ossl_quic_rstream_available(stream->rstream, &rs_avail, &rs_fin))
return 0;
if (stream->recv_state == QUIC_RSTREAM_STATE_SIZE_KNOWN
&& !ossl_quic_rstream_available(stream->rstream, &rs_avail, &rs_fin)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_INTERNAL_ERROR,
frame_type,
"internal error (rstream available)");
return 0;
}

if (rs_fin)
ossl_quic_stream_map_notify_totally_received(&ch->qsm, stream);
Expand Down Expand Up @@ -982,12 +997,18 @@ static int depack_do_frame_path_response(PACKET *pkt,
return 1;
}

static int depack_do_frame_conn_close(PACKET *pkt, QUIC_CHANNEL *ch)
static int depack_do_frame_conn_close(PACKET *pkt, QUIC_CHANNEL *ch,
uint64_t frame_type)
{
OSSL_QUIC_FRAME_CONN_CLOSE frame_data;

if (!ossl_quic_wire_decode_frame_conn_close(pkt, &frame_data))
if (!ossl_quic_wire_decode_frame_conn_close(pkt, &frame_data)) {
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_FRAME_ENCODING_ERROR,
frame_type,
"decode error");
return 0;
}

ossl_quic_channel_on_remote_conn_close(ch, &frame_data);
return 1;
Expand All @@ -997,8 +1018,14 @@ static int depack_do_frame_handshake_done(PACKET *pkt,
QUIC_CHANNEL *ch,
OSSL_ACKM_RX_PKT *ackm_data)
{
if (!ossl_quic_wire_decode_frame_handshake_done(pkt))
if (!ossl_quic_wire_decode_frame_handshake_done(pkt)) {
/* This can fail only with an internal error. */
ossl_quic_channel_raise_protocol_error(ch,
QUIC_ERR_INTERNAL_ERROR,
OSSL_QUIC_FRAME_TYPE_HANDSHAKE_DONE,
"internal error (decode frame handshake done)");
return 0;
}

ossl_quic_channel_on_handshake_confirmed(ch);
return 1;
Expand Down Expand Up @@ -1321,7 +1348,7 @@ static int depack_process_frames(QUIC_CHANNEL *ch, PACKET *pkt,
/* FALLTHRU */
case OSSL_QUIC_FRAME_TYPE_CONN_CLOSE_TRANSPORT:
/* CONN_CLOSE_TRANSPORT frames are valid in all packets */
if (!depack_do_frame_conn_close(pkt, ch))
if (!depack_do_frame_conn_close(pkt, ch, frame_type))
return 0;
break;

Expand Down
8 changes: 2 additions & 6 deletions ssl/quic/quic_txp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp,
&& pkt[QUIC_ENC_LEVEL_HANDSHAKE].h.bytes_appended > 0);

/* Flush & Cleanup */
res = TX_PACKETISER_RES_NO_PKT;
res = pkts_done > 0 ? TX_PACKETISER_RES_SENT_PKT : TX_PACKETISER_RES_NO_PKT;
out:
ossl_qtx_finish_dgram(txp->args.qtx);

Expand All @@ -842,11 +842,7 @@ int ossl_quic_tx_packetiser_generate(OSSL_QUIC_TX_PACKETISER *txp,
++enc_level)
txp_pkt_cleanup(&pkt[enc_level], txp);

/*
* If we already successfully did at least one, make sure we report this via
* the return code.
*/
return pkts_done > 0 ? TX_PACKETISER_RES_SENT_PKT : res;
return res;
}

static const struct archetype_data archetypes[QUIC_ENC_LEVEL_NUM][TX_PACKETISER_ARCHETYPE_NUM] = {
Expand Down

0 comments on commit 9601484

Please sign in to comment.