Skip to content

Fix handling of PPP data packets concatenated-within-1-TLS record and split-across-multiple-TLS-records

Daniel Lenski requested to merge fix_incomplete_Fortinet_packets into master

  • Fix handling of concatenated PPP data packets (#252 (closed))

    This 'continue' prevented us from ever actually processing a concatenated packet if the first one was a data packet. Dan only tested this for concatenation of config packets in the early stages of the connection. Bad Dan.

  • Temporary fix for Fortinet packets split across TLS records (with hard-coded delay)

    In addition to concatenating multiple PPP packets within a single TLS record (which is fine), Fortinet will sometimes split PPP packets across multiple TLS records, which is horrid. Sadly, it's not unexpected, because oNCP does similar terrible things.

    If we don't handle this case correctly, then we lose synchronization to the expected encapsulation header when we read the next TLS record as well, causing us to drop the encapsulated packet(s) therein as well. Under heavy load from the remote end, this will cause us to drop all incoming packets until the remote end completely stops sending new packets.

  • Fix Fortinet packets split across TLS records

    This replicates the behavior of oncp_record_read(): if a Fortinet (D)TLS record ends with an incomplete PPP packet, we save the read-so-far length as vpninfo->oncp_rec_size, and continue reading at that point on the next iteration.

    This poorly-layered packetisiation behavior may exist for other encapsulations as well, but we haven't yet observed it.

    This works as expected to prevent loss of synchronization to the encapsulation header. Here's an example showing receipt of incomplete Fortinet/PPP packets, during a high rate of incoming packets generated with 'iperf3 -c $IP_ON_REMOTE_NETWORK -R', followed by recovery on subsequent mainloop iterations:

      Received IPv4 data packet of 1183 bytes over TLS
  Fortinet packet is incomplete (837 bytes on wire but header payload_len is 1185). Waiting for more.
  Packet contains 539 bytes after payload. Assuming concatenated packet.
  Received IPv4 data packet of 1183 bytes over TLS
  Fortinet packet is incomplete (539 bytes on wire but header payload_len is 1185). Waiting for more.
  No work to do; sleeping for 10000 ms...
  No work to do; sleeping for 10000 ms...
  No work to do; sleeping for 10000 ms...
  No work to do; sleeping for 10000 ms...
  Packet contains 13101 bytes after payload. Assuming concatenated packet.
  Received IPv4 data packet of 1183 bytes over TLS
  Packet contains 11910 bytes after payload. Assuming concatenated packet.
  Received IPv4 data packet of 1183 bytes over TLS
  Packet contains 10719 bytes after payload. Assuming concatenated packet.
  Received IPv4 data packet of 1183 bytes over TLS
  Packet contains 9528 bytes after payload. Assuming concatenated packet.

  • Rename oncp_rec_size → partial_rec_size

    This variable is used to track the size of partially-received tunnel packets across mainloop invocations.

    It was intially used by the oNCP protocol, where tunneled packets can be split across TLS records. We're now using it to deal with an extremely similar mis-layering of packetisation for PPP-based protocols as well.

    Renaming it to partial_rec_size to emphasize its cross-protocol nature (cf. !151).

  • Assume that other PPP-based protocols can also send PPP packets across (D)TLS records

    Any protocol with a payload length in the encapsulation header (Fortinet or F5-non-HDLC) which potentially splits PPP packets across TLS records can be handled in the same way as we're already doing for Fortinet.

Edited by Daniel Lenski

Merge request reports