BUG/MEDIUM: quic: fix unsuccessful handshakes on ncb_advance error

QUIC handshakes were frequently in error due to haproxy misuse of
ncbuf. This resulted in one of the following scenario :
- handshake rejected with CONNECTION_CLOSE due to overlapping data
  rejected
- CRYPTO data fully received by haproxy but handshake completion signal
  not reported causing the client to emit PING repeatedly before timeout

This was produced because ncb_advance() result was not checked after
providing crypto data to the SSL stack in qc_provide_cdata(). However,
this operation can fail if a too small gap is formed. In the meantime,
quic_enc_level offset was always incremented. In some cases, this caused
next ncb_add() to report rejected overlapping data. In other cases, no
error was reported but SSL stack never received the end of CRYPTO data.

Change slightly the handling of new CRYPTO frames to avoid this bug :
when receiving a CRYPTO frame for the current offset, handle them
directly as previously done only if quic_enc_level ncbuf is empty. In
the other case, copy them to the buffer before treating contiguous data
via qc_treat_rx_crypto_frms().

This change ensures that ncb_advance() operation is now conducted only
in a data block : thus this is guaranteed to never fail.

This bug was easily reproduced with chromium as it fragments CRYPTO
frames randomly in several frames out of order.

This commit has two drawbacks :
- it is slightly less worst on performance because as sometimes even
  data at the current offset will be memcpy
- if a client uses too many fragmented CRYPTO frames, this can cause
  repeated ncb_add() error on gap size. This can be reproduced with
  chrome, albeit with a slighly less frequent rate than the fixed issue.

This change should fix in part github issue #1903.

This must be backported up to 2.6.

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 832886260146f2f97108dc1095c6a9e86e6d924b..7fc2ecd4318ca61f41b8a8abea3ffd200977ffaa 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -2682,7 +2682,7 @@ static int qc_handle_crypto_frm(struct quic_conn *qc,
                frm->offset = cstream->rx.offset;
        }
 
-       if (frm->offset == cstream->rx.offset) {
+       if (frm->offset == cstream->rx.offset && ncb_is_empty(ncbuf)) {
                if (!qc_provide_cdata(qel, qc->xprt_ctx, frm->data, frm->len,
                                      pkt, &cfdebug)) {
                        // trace already emitted by function above