Re: [bug report 5/8] Perform initial AEAD validation before creating a channel

[Alexandr Nedvedicky]

Hello Dan,

thank you for taking a look.

On Fri, Apr 10, 2026 at 08:14:50PM +0300, Dan Carpenter wrote:
> Hello Alexandr Nedvedicky,
>
> Commit c14ae0461352 ("Perform initial AEAD validation before creating
> a channel") from Feb 18, 2025 (linux-next), leads to the following
> Smatch static checker warning:
>
> ssl/quic/quic_port.c:1666 port_default_packet_handler()
> error: we previously assumed 'dcid' could be null (see line 1585)
>
> ssl/quic/quic_port.c
> 1563 static void port_default_packet_handler(QUIC_URXE *e, void *arg,
> 1564 const QUIC_CONN_ID *dcid)
> 1565 {
> 1566 QUIC_PORT *port = arg;
> 1567 PACKET pkt;
> 1568 QUIC_PKT_HDR hdr;
> 1569 QUIC_CHANNEL *ch = NULL, *new_ch = NULL;
> 1570 QUIC_CONN_ID odcid;
> 1571 uint8_t gen_new_token = 0;
> 1572 OSSL_QRX *qrx = NULL;
> 1573 OSSL_QRX *qrx_src = NULL;
> 1574 OSSL_QRX_ARGS qrx_args = { 0 };
> 1575 uint64_t cause_flags = 0;
> 1576 OSSL_QRX_PKT *qrx_pkt = NULL;
> 1577
> 1578 /* Don't handle anything if we are no longer running. */
> 1579 if (!ossl_quic_port_is_running(port))
> 1580 goto undesirable;
> 1581
> 1582 if (port_try_handle_stateless_reset(port, e))
> 1583 goto undesirable;
> 1584
> 1585 if (dcid != NULL
> ^^^^^^^^^^^^
> The old code assumed dcid could be NULL

and yes it can still be in case the caller (demux_process_pending_urxe())
found malformed header and failed to extract dcid from packet.

>
> 1586 && ossl_quic_lcidm_lookup(port->lcidm, dcid, NULL,
> 1587 (void **)&ch)) {
> 1588 assert(ch != NULL);
> 1589 ossl_quic_channel_inject(ch, e);
> 1590 return;
> 1591 }
</snip>
> 1610 /*
> 1611 * We set short_conn_id_len to SIZE_MAX here which will cause the decode
> 1612 * operation to fail if we get a 1-RTT packet. This is fine since we only
> 1613 * care about Initial packets.
> 1614 */

if the packet is malformed the dcid is NULL here. And we know it but the code
as you are pointing out deliberately ignores that warning sign and is going
to parse the malformed packet.

> 1615 if (!ossl_quic_wire_decode_pkt_hdr(&pkt, SIZE_MAX, 1, 0, &hdr, NULL,
> 1616 &cause_flags)) {
> 1617 /*
> 1618 * If we fail due to a bad version, we know the packet up to the version
> 1619 * number was decoded, and we use it below to send a version
> 1620 * negotiation packet
> 1621 */
> 1622 if ((cause_flags & QUIC_PKT_HDR_DECODE_BAD_VERSION) == 0)
> 1623 goto undesirable;

the goto at line 1623 avoids that NULL pointer dereference, however I agree we
could let function to exit way earlier saving CPU cycles on futile processing
of malformed packet.

> 1624 }
> 1625
> 1626 switch (hdr.version) {
> 1627 case QUIC_VERSION_1:
> 1628 break;
> 1629

</snip>
> 1664 qrx_args.libctx = port->engine->libctx;
> 1665 qrx_args.demux = port->demux;
> --> 1666 qrx_args.short_conn_id_len = dcid->id_len;
> ^^^^^^^^^^^^
> but the patch adds an unchecked dereference

fortunately the code just burns CPU cycles.


I think adding a dcid == NULL check somewhere as we enter the function
and let function to fail when dcid is not found is worth thing to do.

Are you interested to submit PR? or shall I proceed with PR?

thanks and
regards
sashan

[Alexandr Nedvedicky]

Hello Dan,

On Mon, Apr 13, 2026 at 01:07:47PM +0300, Dan Carpenter wrote:
</snip>

> >
> > if the packet is malformed the dcid is NULL here. And we know it but the code
> > as you are pointing out deliberately ignores that warning sign and is going
> > to parse the malformed packet.
> >
> > > 1615 if (!ossl_quic_wire_decode_pkt_hdr(&pkt, SIZE_MAX, 1, 0, &hdr, NULL,
> > > 1616 &cause_flags)) {
> > > 1617 /*
> > > 1618 * If we fail due to a bad version, we know the packet up to the version
> > > 1619 * number was decoded, and we use it below to send a version
> > > 1620 * negotiation packet
> > > 1621 */
> > > 1622 if ((cause_flags & QUIC_PKT_HDR_DECODE_BAD_VERSION) == 0)
> > > 1623 goto undesirable;
> >
> > the goto at line 1623 avoids that NULL pointer dereference, however I agree we
> > could let function to exit way earlier saving CPU cycles on futile processing
> > of malformed packet.
>

> Huh. It's a bit complicated for me to change the static checker to
> connect the dots between a NULL dcid and the goto on line 1623...
> Thanks for taking a look!

your static analyzer is doing a good job. it identified the
place with suspicious smell in code. what else one can ask for.

>
> >
> > > 1624 }
> > > 1625
> > > 1626 switch (hdr.version) {
> > > 1627 case QUIC_VERSION_1:
> > > 1628 break;
> > > 1629
> >
> > </snip>
> > > 1664 qrx_args.libctx = port->engine->libctx;
> > > 1665 qrx_args.demux = port->demux;
> > > --> 1666 qrx_args.short_conn_id_len = dcid->id_len;
> > > ^^^^^^^^^^^^
> > > but the patch adds an unchecked dereference
> >
> > fortunately the code just burns CPU cycles.
> >
> >
> > I think adding a dcid == NULL check somewhere as we enter the function
> > and let function to fail when dcid is not found is worth thing to do.
> >
> > Are you interested to submit PR? or shall I proceed with PR?
>

> I'm trying to get out of the business of patching the code myself.
> I'd like to extend static analysis to all the C programs in Debian
> but I'm having a hard time scaling up the bug reporting process...
>

I will submit PR then with all the credit you deserve.

thanks and
regards
sashan

[Alexandr Nedvedicky]

Hello,

just to wrap things up, here is PR:
https://github.com/openssl/openssl/pull/30795

thanks and
regards
sashan