QUIC: Miscellaneous error handling updates

Raise errors when appropriate.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21700)

4 files changed, 78 insertions, 34 deletions

diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index 2d20f3743e..2163990ae7 100644
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -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;
     }
@@ -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))
@@ -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)
@@ -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)
@@ -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:
@@ -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) */
     }
 
@@ -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;
 }
@@ -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))
@@ -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;
diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index b0d613299f..919affc0ae 100644
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -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;
 }
 
@@ -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);
 }
@@ -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);
 }
@@ -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)
@@ -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;
     }
@@ -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)) {
         /*
@@ -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;
 
@@ -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;
     }
 
diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c
index f7d0406e2b..3ef4798111 100644
--- a/ssl/quic/quic_rx_depack.c
+++ b/ssl/quic/quic_rx_depack.c
@@ -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;
 
@@ -594,8 +599,13 @@ 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
@@ -603,9 +613,14 @@ static int depack_do_frame_stream(PACKET *pkt, QUIC_CHANNEL *ch,
      * 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);
@@ -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;
@@ -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;
@@ -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;
 
diff --git a/ssl/quic/quic_txp.c b/ssl/quic/quic_txp.c
index 0f6fe140ec..f0c6d96fff 100644
--- a/ssl/quic/quic_txp.c
+++ b/ssl/quic/quic_txp.c
@@ -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);
 
@@ -842,11 +842,7 @@ out:
          ++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] = {