From 76a42bef05244d0023b27b26ad9f24b0d2901df9 Mon Sep 17 00:00:00 2001 From: Daniel Lenski Date: Mon, 7 Jun 2021 15:46:23 -0700 Subject: [PATCH] Fix PPP packets split across TLS records This replicates the behavior of oncp_record_read(): if a (D)TLS record ends with an incomplete PPP packet, we save the read-so-far length as vpninfo->partial_rec_size, and continue reading at that point on the next iteration. This poorly-layered packetisation behavior was observed on Fortinet and may exist for other encapsulations as well. So we cope with it for PPP_ENCAP_F5 because it's trivial, but not yet PPP_ENCAP_F5_HDLC which would be harder; we'll wait until we actually see it there before delving into that complexity. 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. Signed-off-by: Daniel Lenski Signed-off-by: David Woodhouse --- ppp.c | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/ppp.c b/ppp.c index cb67ee87..05e88a28 100644 --- a/ppp.c +++ b/ppp.c @@ -890,6 +890,7 @@ static int handle_state_transition(struct openconnect_info *vpninfo, int dtls, * we need to reconfigure before we can send data packets. */ free(vpninfo->current_ssl_pkt); vpninfo->current_ssl_pkt = NULL; + vpninfo->partial_rec_size = 0; ppp->ppp_state = PPPS_ESTABLISH; /* fall through */ @@ -1101,13 +1102,28 @@ static int ppp_mainloop(struct openconnect_info *vpninfo, int dtls, * the payload later. */ rsv_hdr_size = ppp->encap_len + ppp->exp_ppp_hdr_size; - /* Load the encap header to end up with the payload where we expect it */ + /* This should never happen here as we should complain about it + * before setting ->partial_rec_size in the first place, but be + * paranoid. This would mean we just read zero (or negative!) */ + if (vpninfo->partial_rec_size >= receive_mtu + rsv_hdr_size) { + vpninfo->quit_reason = "Payload exceeds MTU"; + vpn_progress(vpninfo, PRG_ERR, + _("PPP payload exceeds receive buffer\n")); + return -EINVAL; + } + + /* Load the encap header to end up with the payload where we expect it. Also, + * if our previous (D)TLS read contained an incomplete PPP packet, we need + * to append to it. */ eh = this->data - rsv_hdr_size; - len = ssl_nonblock_read(vpninfo, dtls, eh, receive_mtu + rsv_hdr_size); + len = ssl_nonblock_read(vpninfo, dtls, eh + vpninfo->partial_rec_size, + receive_mtu + rsv_hdr_size - vpninfo->partial_rec_size); if (!len) break; if (len < 0) goto do_reconnect; + len += vpninfo->partial_rec_size; + vpninfo->partial_rec_size = 0; /* XX: Some protocols require us to check for an HTTP response in place * of the first packet @@ -1129,11 +1145,11 @@ static int ppp_mainloop(struct openconnect_info *vpninfo, int dtls, * len: number of bytes-from-the-wire */ - if (len < 8) { + if (len < (ppp->encap_len ? : 8)) { short_pkt: - vpn_progress(vpninfo, PRG_ERR, _("Short packet received (%d bytes)\n"), len); - vpninfo->quit_reason = "Short packet received"; - return 1; + vpn_progress(vpninfo, PRG_DEBUG, _("Short packet received (%d bytes). Waiting for more.\n"), len); + vpninfo->partial_rec_size = len; + continue; } if (vpninfo->dump_http_traffic) @@ -1157,10 +1173,19 @@ static int ppp_mainloop(struct openconnect_info *vpninfo, int dtls, if (len < 4 + payload_len) { incomplete_pkt: - vpn_progress(vpninfo, PRG_ERR, - _("Packet is incomplete. Received %d bytes on wire (includes %d encap) but header payload_len is %d\n"), + /* We've read a partial PPP packet. Save the offset for our next read. */ + vpninfo->partial_rec_size = len; + if (vpninfo->partial_rec_size >= receive_mtu + rsv_hdr_size) { + vpninfo->quit_reason = "Payload exceeds MTU"; + vpn_progress(vpninfo, PRG_ERR, + _("PPP payload len %d exceeds receive buffer %d\n"), + payload_len, receive_mtu + rsv_hdr_size); + dump_buf_hex(vpninfo, PRG_ERR, '<', eh, len); + return -EINVAL; + } + vpn_progress(vpninfo, PRG_DEBUG, + _("PPP packet is incomplete. Received %d bytes on wire (includes %d encap) but header payload_len is %d. Waiting for more.\n"), len, ppp->encap_len, payload_len); - dump_buf_hex(vpninfo, PRG_ERR, '<', eh, len); continue; } break; -- 2.49.0