Re: [Outreachy] Multiple-line parsing of packets dissected over HTTP

[Pascal Quantin <pascal () wireshark org>]

Hi Joey,

Le mer. 20 janv. 2021 à 20:15, Joey Salazar <jgsal () protonmail com> a écrit :

> Hi Pascal,
>
> On Wednesday, January 20, 2021 4:23 AM, Pascal Quantin wrote:
>
> Hi Joey,
>
> Le mar. 19 janv. 2021 à 23:35, Joey Salazar <jgsal () protonmail com> a
> écrit :
>
> > On Tuesday, January 19, 2021 4:20 PM, Pascal Quantin wrote:
> >
> > Le mar. 19 janv. 2021 à 23:09, Joey Salazar <jgsal () protonmail com> a
> > écrit :
> >
> > > Hi Pascal,
> > > On Tuesday, January 19, 2021 11:19 AM, Pascal Quantin wrote:
> > >
> > > Hi Joey,
> > >
> > > Le mar. 19 janv. 2021 à 17:45, Joey Salazar via Wireshark-dev <
> > > wireshark-dev () wireshark org> a écrit :
> > >
> > > > Hi all,
> > > >
> > > > In commit 33af2649 [1] we can keep dissecting the contents of the req,
> > > > adv, and res packets by setting
> > > >  while (plen > 0) { }
> > > > either in `dissect_git_pdu()` or in `dissect_one_pkt_line()`, but for
> > > > now in `dissect_git_pdu()` it'd be a bit messy, so wanted to ask for your
> > > > feedback for getting `dissect_one_pkt_line()` to work properly first.
> > > >
> > > > As you can see in pcap 169 [2], it correctly parses the length of the
> > > > first line as 0x0014 (20 bytes) until `0x0a`, then it's supposed to get the
> > > > length of the next line by the first 4 hex bytes in that line, but instead
> > > > of reading the length as 0x0018 (24 bytes) it's reading it as 0x0010 (16
> > > > bytes), and anyways, this particular line's length actually is 59 bytes.
> > > >
> > > > Suggestions on how to approach this?
> > >
> > > So what is the code leading to this dissection? It does not seem to be
> > > gitlab.com/joeysal/wireshark/-/commit/33af2649927cb5660d4aeb64b9a9e9a58a1823aa
> > > as dissect_one_pkt_line() seem to read only one line
> > >
> > > Yes, the code on that commit is what gives the parsing of the screenshot.
> >
> > So what mechanism is used to call dissect_one_plt_line() a second time?
> > With only screenshots and no pcap / code to look at, we can hardly help.
> >
> > The code has already been provided. I confirm again that there hasn't
> > been other lines added other than what's in that commit.
> >
> > Does it mean that packet-http.c calls your dissector per line? Please
> > provide more info, or even better share the pcap if you want us to provide
> > some help.
> >
> > Please find attached the pcap I'm using with the patch from the commit.
> > As you can see, the way 167 and 255 are parsed is similar, but I'm
> > referring specifically to 169 for now ("To-do" in line 121 will be for the
> > cases where there's a 0000 terminator packet like the end of the first-line
> > in 167) .
>
> Unfortunately you did not share the associated TLS secret (or I missed it)
> that would allow me to decrypt the session and test your dissector. Could
> you send it?
>
> My big apologies, I haven't worked with TLS certificates in the past and
> completely missed to send the secret. Apologies for taking your time.
> Please let me know if I'm missing anything else.

The use of a debugger clearly shows what the issue is:
- dissect_one_pkt_line() gets the length of the first line only with
get_packet_length(). So the while loop after should be useless as you will
consume the full line anyway as I stated previously
- dissect_one_pkt_line() is in fact intended to decode all lines, but you
are not updating plen after adding the hf_git_packet_data item to the tree
while incrementing offset. So you reuse the previous value of plen (that
has been decremented by 4 after putting the hf_git_packet_len item), thus
the value 0x0010 you get.
Your code should be instead something like this:

  total_len = tvb_reported_length(tvb);
  while (offset < total_len) {
    if (!get_packet_length(tvb, offset, &plen)) {
      /* XXX display expert info error? */
      return tvb_captured_length(tvb);
    }
    proto_tree_add_uint(git_tree, hf_git_packet_len, tvb, offset, 4, plen);
    offset += 4;
    plen -= 4;

    proto_tree_add_item(git_tree, hf_git_packet_data, tvb, offset, plen,
ENC_NA);
    offset += plen;
    // To-do: add lines for parsing of terminator packet 0000
  }
  if (plen == 0) {
    proto_tree_add_uint(git_tree, hf_git_packet_terminator, tvb, offset,
                        4, plen);
  }
  return tvb_captured_length(tvb);

Note that in packet 169 there seems to be an issue with the fourth line
that has a length of 1 only while you expect 4 at a minimum in your code.
It needs to be properly handled.

Hope this helps,
Pascal.

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe