From 2e5f5ab6455ea4f6baf8585bda7bc906294ed2b1 Mon Sep 17 00:00:00 2001 From: John Thacker Date: Sat, 9 Dec 2023 22:05:28 -0500 Subject: [PATCH] HTTP3, QUIC: Desegment HTTP3 QPACK Encoder Streams Return the number of bytes decoded and placed in the tree and set pinfo->desegment_offset and desegment_len so that the QUIC disssector can desegment the HTTP3 Encoder stream. Pass that number of bytes to the nghttp3 decoder so that we don't end up passing the same bytes twice with reassembly. Make it so the QUIC data stream desegmenting code puts a link to the frame data was reassembled in for segments that begin an MSP as well in more cases, as the TCP dissector does. (There are a few more cases TODO to produce results similar to TCP.) Fix #19475 --- epan/dissectors/packet-http3.c | 33 ++++----- epan/dissectors/packet-quic.c | 65 ++++++++++-------- .../http3-qpack-reassembly-anon.pcapng | Bin 0 -> 5988 bytes test/fixtures_ws.py | 1 + test/suite_dissection.py | 19 +++++ 5 files changed, 75 insertions(+), 43 deletions(-) create mode 100644 test/captures/http3-qpack-reassembly-anon.pcapng diff --git a/epan/dissectors/packet-http3.c b/epan/dissectors/packet-http3.c index 6b7a7eafeb..b664436870 100644 --- a/epan/dissectors/packet-http3.c +++ b/epan/dissectors/packet-http3.c @@ -1796,14 +1796,14 @@ dissect_http3_qpack_encoder_stream(tvbuff_t *tvb, packet_info *pinfo _U_, proto_ ENDTRY; } - return offset + decoded; + return decoded; } static gint dissect_http3_qpack_enc(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, int offset, quic_stream_info *stream_info, http3_stream_info_t *http3_stream) { - gint remaining, remaining_captured, retval; //, decoded = 0; + gint remaining, remaining_captured, retval, decoded = 0; proto_item * qpack_update; proto_tree * qpack_update_tree; http3_session_info_t *http3_session; @@ -1821,38 +1821,39 @@ dissect_http3_qpack_enc(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, int */ qpack_update = proto_tree_add_item(tree, hf_http3_qpack_encoder, tvb, offset, remaining, ENC_NA); qpack_update_tree = proto_item_add_subtree(qpack_update, ett_http3_qpack_update); - /* decoded = */ dissect_http3_qpack_encoder_stream(tvb, pinfo, qpack_update_tree, offset, + decoded = dissect_http3_qpack_encoder_stream(tvb, pinfo, qpack_update_tree, offset, http3_stream); + if (decoded < remaining) { + pinfo->desegment_offset = offset + decoded; + pinfo->desegment_len = DESEGMENT_ONE_MORE_SEGMENT; + } + #ifdef HAVE_NGHTTP3 if (remaining > 0) { proto_item * ti; http3_stream_dir packet_direction = http3_packet_get_direction(stream_info); nghttp3_qpack_decoder *decoder = http3_session->qpack_decoder[packet_direction]; - /* XXX: We could use what's in this decoder state in order to help - * defragment encoded QPACK data that is split across multiple - * packets. (The nghttp3_qpack_decoder handles this already; that - * would be for improving what we put in the proto tree.) - */ http3_qpack_encoder_state_t *encoder_state = http3_get_qpack_encoder_state(pinfo, tvb, offset); if (!PINFO_FD_VISITED(pinfo)) { - uint8_t *qpack_buf = (uint8_t *)tvb_memdup(pinfo->pool, tvb, offset, remaining); /* - * Pass the entire stream buffer to the decoder; we stop adding - * instructions that are split across packet boundaries, - * but the nghttp3_qpack_decoder saves states. + * Since we are now defragmenting, pass only the number of bytes + * decoded to the nghttp3_qpack_decoder. Otherwise, we'll end up + * sending the same bytes to the decoder again when the packet + * is defragmented. */ - gint qpack_buf_len = remaining; + uint8_t *qpack_buf = (uint8_t *)tvb_memdup(pinfo->pool, tvb, offset, decoded); + gint qpack_buf_len = decoded; /* * Get the instr count prior to processing the data. */ uint64_t icnt_before = nghttp3_qpack_decoder_get_icnt(decoder); - HTTP3_DISSECTOR_DPRINTF("decode encoder stream: decoder=%p remaining=%u", decoder, remaining); + HTTP3_DISSECTOR_DPRINTF("decode encoder stream: decoder=%p decoded=%u remaining=%u", decoder, decoded, remaining); encoder_state->nread = nghttp3_qpack_decoder_read_encoder(decoder, qpack_buf, qpack_buf_len); encoder_state->icnt = nghttp3_qpack_decoder_get_icnt(decoder); @@ -1867,7 +1868,7 @@ dissect_http3_qpack_enc(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, int quic_cid_t quic_cid = {.len = 0}; gboolean initial_cid_found = quic_conn_data_get_conn_client_dcid_initial(pinfo, &quic_cid); proto_tree_add_expert_format( - tree, pinfo, &ei_http3_qpack_failed, tvb, offset, 0, "QPAC decoder %p DCID %s [found=%d] error %d (%s)", + tree, pinfo, &ei_http3_qpack_failed, tvb, offset, 0, "QPACK decoder %p DCID %s [found=%d] error %d (%s)", decoder, cid_to_string(&quic_cid, pinfo->pool), initial_cid_found, (int)encoder_state->nread, nghttp3_strerror((int)encoder_state->nread)); } @@ -1884,7 +1885,7 @@ dissect_http3_qpack_enc(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, int #else (void)stream_info; (void)qpack_update; - /*(void)decoded;*/ + (void)decoded; #endif /* HAVE_NGHTTP3 */ return retval; diff --git a/epan/dissectors/packet-quic.c b/epan/dissectors/packet-quic.c index fbb1a8a1cf..a8406e3d1d 100644 --- a/epan/dissectors/packet-quic.c +++ b/epan/dissectors/packet-quic.c @@ -1626,13 +1626,11 @@ again: /* Did the subdissector ask us to desegment some more data * before it could handle the packet? - * If so we have to create some structures in our table but - * this is something we only do the first time we see this - * packet. + * If so we'll have to handle that later. */ if (pinfo->desegment_len) { + must_desegment = TRUE; if (!PINFO_FD_VISITED(pinfo)) { - must_desegment = TRUE; if (msp) msp->flags &= ~MSP_FLAGS_GOT_ALL_SEGMENTS; } @@ -1753,33 +1751,42 @@ again: } } - if (must_desegment && !PINFO_FD_VISITED(pinfo)) { - // TODO handle DESEGMENT_UNTIL_FIN if needed, maybe use the FIN bit? + if (must_desegment) { guint32 deseg_seq = seq + (deseg_offset - offset); - if (((nxtseq - deseg_seq) <= 1024*1024) - && (!PINFO_FD_VISITED(pinfo))) { - if(pinfo->desegment_len == DESEGMENT_ONE_MORE_SEGMENT) { - /* The subdissector asked to reassemble using the - * entire next segment. - * Just ask reassembly for one more byte - * but set this msp flag so we can pick it up - * above. - */ - msp = pdu_store_sequencenumber_of_next_pdu(pinfo, deseg_seq, - nxtseq+1, stream->multisegment_pdus); - msp->flags |= MSP_FLAGS_REASSEMBLE_ENTIRE_SEGMENT; - } else { - msp = pdu_store_sequencenumber_of_next_pdu(pinfo, - deseg_seq, nxtseq+pinfo->desegment_len, stream->multisegment_pdus); - } + if (!PINFO_FD_VISITED(pinfo)) { + // TODO handle DESEGMENT_UNTIL_FIN if needed, maybe use the FIN bit? + if ((nxtseq - deseg_seq) <= 1024*1024) { + if(pinfo->desegment_len == DESEGMENT_ONE_MORE_SEGMENT) { + /* The subdissector asked to reassemble using the + * entire next segment. + * Just ask reassembly for one more byte + * but set this msp flag so we can pick it up + * above. + */ + msp = pdu_store_sequencenumber_of_next_pdu(pinfo, deseg_seq, + nxtseq+1, stream->multisegment_pdus); + msp->flags |= MSP_FLAGS_REASSEMBLE_ENTIRE_SEGMENT; + } else { + msp = pdu_store_sequencenumber_of_next_pdu(pinfo, + deseg_seq, nxtseq+pinfo->desegment_len, stream->multisegment_pdus); + } - /* add this segment as the first one for this new pdu */ - fragment_add(&quic_reassembly_table, tvb, deseg_offset, - pinfo, reassembly_id, NULL, - 0, nxtseq - deseg_seq, - nxtseq < msp->nxtpdu); + /* add this segment as the first one for this new pdu */ + fragment_add(&quic_reassembly_table, tvb, deseg_offset, + pinfo, reassembly_id, NULL, + 0, nxtseq - deseg_seq, + nxtseq < msp->nxtpdu); + } + } else { + /* If this is not the first time we have seen the packet, then + * the MSP should already be created. Retrieve it to see if we + * know what later frame the PDU is reassembled in. + */ + if ((msp = (struct tcp_multisegment_pdu *)wmem_tree_lookup32(stream->multisegment_pdus, deseg_seq))) { + fh = fragment_get(&quic_reassembly_table, pinfo, reassembly_id, NULL); + } } } @@ -1794,6 +1801,10 @@ again: 0, fh->reassembled_in); proto_item_set_generated(item); } + + /* TODO: Show what's left in the packet as a raw QUIC "segment", like + * packet-tcp.c does here. + */ } pinfo->can_desegment = 0; pinfo->desegment_offset = 0; diff --git a/test/captures/http3-qpack-reassembly-anon.pcapng b/test/captures/http3-qpack-reassembly-anon.pcapng new file mode 100644 index 0000000000000000000000000000000000000000..4a7e02d2ea3e1b5b7cf7a90f6aeb27513cdf2cae GIT binary patch literal 5988 zcmcIoXINCp(jKyaWF&(q0s;b)193(eK!=<}P;jCmNl7B0AQ{O>4iY5iBuGXSMY04* z3Mvu=K~PjOcV>5ecK81M?pM!Kr%rdzsq=Q#^jqDBje~GZEju%;mMPX;5=2JA zp+q1IK|~JMMz+@l^h@yJLgW*UNkOG1bpkx#UK>;HXPzn)6gyRul zA_{_su>FNj4`ZlpriZ~{|Mv)qUA10f&;lnetw$wUf;NCpxi z2qG040!SuNP-G~IjHFHugoKdb5H>TcsU_Cb?*9)V6i%YVMGWKibq1Bcsv|VMo~woNKs%CEvi8LZ-oEOtsU_Ho?QwYK?G5tBoG+{ zM8aWs5P^gMks(kZ0t%sK6@dUFkT4LL8airJArK%8PoM^oj6fj>)Q6y?-`V}&BZE<6 z27)4>co-al0E6)m5(NRH1_%NK10g6V7z~CYkVGUA2ql4`NFV}_A`w6!0*DMJg5e}Q zp1MeAYlNA)RwS9Im%l_nDueZa5dfem7QlEg8H5I)d0tzy0qD8>sLL%km9Cj_nZ@lX z_4jl~!Y7aw0-!N~B+IAzIZ4L4)}yj7gbK2YJ-dN6Df^m5&tKIWe&}esbajo1 zx#)n+T%qn6nwPS@|)mppKyd1*W*XZ zV%?Mi;8&(5u^NNx6|%5^JV%)aTT1imq)YBrXBl8d8Up|}c zMG?}_bGb>Ad;7 zT&p2J#31WdJc5h&JmKujV7Dsy522s)&)#XZ8B^Uj2>9v!c)ZnT-J$IX#ky5L-1?;_ z_pYai;7bxR(`Dg)w)g$@+kHjC2`y4k=W9ir-3y4)|?I z%0VcrZtI&aH&=sJHF(+4gXO++%}?~7{nGn2e59G zfj&ET`%G6&T1wXzAHS>m^uR7xSMa)Uy_T;tr+2;Uy3g(d>7(4+&0mrn-;;sI*FV+1 zJvN8F8ukj`BN6lxcXl%Z?Gj29*kM4{xNF*<1^#u7HbAw_v|LS1>MpyB8RpAZu|pH2uQ% z;%M-i8mDlGym`vH!``S*SoETrxU->!@Sq;zXV9nZs;X}LGbWyL6T05(7Az_U-RChM zM>=rcoad1CBb>SC{2rdUD5LREijUFcZiCI;Ff+rY)}@h*v9S&VGyFnJzP6jPSBE4( zJKR{jdJ)QU<-OUuTmoNkUl^bT;LR;}V6H}t;Hy-4?)`cAWZkDUeVcRRLS?+Db?Qx zE-!4>py}TY%0%vueGij02xXd?8kuW*@0@%jex~&C^`b@aT!OEQanrIyqjxr}U!+_k z+%TXvGjn0_{BY=*fj~2LJ;BAY`BnM%i*~2|e~FGKteLo1kbKSO3PEZv7Rj~Y1*1-P zL}M}H6{}#O*N>d0$|&>zek)}a!2DSmQQ!Ma!EbLpPl0XKw5xh?d>pbAL*&bwI*d%q zHAE~W)rWz91vlsw5DdI$pzNk2k~d)PaZYMXnU{V0RJA4ESsKr+R`Uy{bSQ1?7CLH~ zSs#^Meu7Gl$vZljspalcFW4e|oPc2ngO)e+GW+T#*k%5}+gA8`8h!aTIbr2LDOOOd zV~#kLyRgGFBmx}MCU`l^77`pvO@Hwio*1^p)H`1BE={9n7-R}(9!iem-FN!I)1V5~ zl)W2bcD3+k2y2n)3G0cXa(=Zf+3>4!Cmm-O-uZJkY<-+iIyI8!lCr4MYh5kU_+?#j zbnOum$4tywuI@psVy|XOG5T9U=)7kpS_2+%YPf<*8l0WXjLm+w$uKI(wwuDlwIlf& zQ)NdXhm$rud)cDCHj(YvR#yNjYQ_9rmhN@cusX5^s>66>?Dg7P^3hDa= zoNADz%fjWR2+@uQuTL zVdDLydBN4=rpo%Ee}CUsi%KrO#5lo|0gk%$pDZI|$_?cbWI488Oa-^IQT*e?j-3rt z8{s|7xvxs}bRB6>1)o1hn!od7Ff?@S!E(qdTzt#S@^jIHjaRoHCw|sSY4IDntZ(pk z-s$kTRruN$&}1u1$bPPRvRRVD(3_y`qwxuW5r=xs`fZ8HmjpWV9$z~m1bkDL#rlw; zo6)t^$Tw$$?RcXvE}C~Gm(_<@6j(tYAD97&NjI_w;T{dP`_w8O$8D;LKBKRzNxnlu z3s$IhQ#dD`|_bcYAr<}h#9ERq~uEMO?1tnTAr$t?`@tJh1@L^q9QPytnZ@nYU5z7Z{++s0Q4sCkhNhcOQr(Bb7wx{#{$GFR6g_|FTJ@+AC0@6or0P^lH}vO^E?Q*YSx?D|dBW-M$fSiGTEB(zHFcB_hfP1hw7#cfFqanMd*{;4sJ?@} zZ3?KXe*E}s#^wIy)LC_($VYB#BVCdZ4iov52fu z-ffPZD9ce#s=mWN^Z}39l^3g$B89z`RF+?UZE$6Y9zSa+HTS4|Ct+-{3HRX$>Kubl z!*$K{)zl4-BE6w+(-(?Hu0N8$X~5Drc2rJ(o`)qmSdqMP#}G9K^i+ik z7$EvOcjLW6x4o0R)on_oDLZF6GT+nD-#rm7vu-{b#}{mLCCqy4@xiU-V=wEZ_i)zw zT#DE7m7smo9d+7Cc4m%`=w8I?G%`~(T@)@>sJTxEAJhsn$0}!sTK0TBm=oqqtQU*! zW$YNR7-$eXzpz~wWyos2Tm@=lY0$A9pVCg5aje@ENRLV^8vjmNnl3DBkEna^!0jlz zpyJ?PtkfwYad;!_a$*PJ)P!b@*=4^C*ZR}ui5Vk?U5WV>@ol5`F!k<^`zKt53+#jd z;lJo2@fQrnNH<;U2M0I6@GE{zN${empv_GW(N|q6rCIStiZ6p(e2!Fv`OWL5v$h&C z>B32m^tM16HzMV~Ho)2@*tDMqO%);K@l5k26{LKk(vH&4+ydXDy}S1^`xM-9`T+tu zvx3d1nV&StW-+^7{xjn1E z?@`#GnMdUKcp)?H?k2w+vhH&Ese~qJOGEK8L0R0t-NX8w{JtuNWCU+xh1$}8e zxwP2o(EGiwhbQJ)!<3`PbFJwjTbo>sv2{Sa7~pDrp7HAenocu5{1py~@nkwT=Z|t1P|4d3K?s z=(O!X8dpzX(NF-ht0)5F@nKjz2i&8`uvPnat++(>`&%5TSX8MEno_Z-PKMI3yb<{q z7TLxk?C&b!q_6ZP_!{Gj^Yp`I6LN6%Mxo_^Pws%+~#vW(OP>(s4W!5H^whhW3n>pHLlvyoX9zDL;gbmMO5IUU-$ zyLAe*+XM<=_!n--al`Ipa9XDEL0$2;)&nr*7vb6C$GNZgxT<^%z#Wg|N-pZOS?D-; zktHBg8=s&kU)uPQC#l|GAaL;6>n)s)*d=K|2Jz>m6N3-+hKN0$^;<#PSk%5X!c#^Y zt+&yu{Is6l=thXv(B#vWgSu%i({6ZH?kb>RIMvf@NHwtGC4bzSlxc&R(=)C)NorUeI7}vuR8PAvc z;i)G*vIlvH5Ay@#Oq-xFExql!fFAWkR~1f8cbVzfd+B}y*fnzQ`5j4r3x&f9o2TY6 zRh^YPeLr!YeKI40A0L=XpOY@g!j8BACwA(6^fuROB^Hlb;go0lA926vq`9J;PthxnydELb-<4;%(yPBy@ayBShfdZGZ}F)+G8)>~nuD2FYyz0Lagw9- zH!5c<+9D-f_FZQBP6UW??>bt}Y(?R%y@PbKGfY~Kul&reEE?4(w$dwjb1zlCf%tP^RwrLa17NFn-+*&!v)?K?ftc)?k~X~ARNItR8ZoW2@rXo-*q_Ioog z4V!L4Q#V#3RKvdOJ&owOa6O{P+&1=z`ME=}ZZEf=VK?liWnv86OK1B&+rBGqsbk;A zh($;tP6>X*s|$0X3spV*>5jEwV^7LGJh)=tU&Dv%JcXMuGRvbgcz-_q#3{ecdvkA> zdstVZjj#R+t=@a8P>XAE<0+e70!5L`H22U=jL(~+b+n`B0-c$6nOIu!!`ZVw9an$0 zH!%jH(thS&t}=t*S@ft=SUm=mN!F)o0N`?;bo9ylwAV z7yYmaA-#6d6EX!algNr)IUeDXb$g%lAT3)(YX83Gmj=HoPWqp4_k1;Juf0MWN#_oY z>dB7iyxkxGUqtMLyjxsOJe%RgGcZ%SR^!X`f+hYydz_N@gYN>{okSBfgD7Zu)KB?g zT%Ny)c*=Uzy3@-6D}8|}yF;?=xcv>AJN(i?qf?R)1do&QLs4PgonpnIgsUJ!?r!5P z%vA^TC*D|DBf1KV&j)?>V(lm1QDMgYCtsU{+BtO@pKO+R^nRzo{0_-`IYN|Knz-Dd zFFLs)Iz$){TNs!s7C08iU10N}!tsW{X^V-@!0(lBBFv(3w!aGIG*RLAU%Q#4cMFtH z@HuDGf2fM9SgE;a#q%k$u#J$2M!X}@kAflf*U7gRXaNGz$S z*v%K;yV$%^RHWFhp+&MY5ZNpDgxSY6a(<7}bzD^nZRg3Df0D@iqtSM#wsq>Ym{sX= zJ0@3aqb+fj-xC%4ZLGjRtC;6QbFazdo0Rt}S}zMZIX^%yeS+(kNB;D{i+(ZQdMfm| zIy*kwD{96qv^207F}4!2qp6WBu4_uj?aUEDBWjJVf7c1fbe*Ub_zwQ|MAfJNoAjS~ z`XgVzlSQ>Ivl*;0R7+E8os5a)0nqjg>i^m^_%zvDo1f>2)qSosyooN}S21KdR$PVZ zYBTTo^|;KFY~5|mJCq5Fi5`BSq;&UU{WRQ#UD!~d9U0Jfqs#V$?b5RrhqQ3Uh&K)6 z`Eb7oO)0>4zs|>lVSO3fpD|Bw=kR#N