On Fri, May 12, 2017 at 3:15 PM, David Woodhouse <dwmw2 at infradead.org> wrote: > On Thu, 2017-05-11 at 10:03 -0700, Daniel Lenski wrote: >> On Thu, May 11, 2017 at 9:27 AM, Nikolay Martynov wrote: >> In the GlobalProtect mainloop >> (https://github.com/dlenski/openconnect/blob/globalprotect/gpst.c#L615) >> I don't pay attention to the size of the packet at all as long as it's >> well-formed. > > We tend to allocate receive buffers big enough for the negotiated MTU, > so I hope you're paying *some* attention to how much data you're > receiving :) That's a good point. Currently, all three protocols (including GP here :-D) allocate their SSL receive buffers based on the mtu. Although both CSTP and GPST pad them up to a minimum size of 16384, which is probably why we never see buffer overflows. cstp.c ------- int len = MAX(16384, vpninfo->deflate_pkt_size ? : vpninfo->ip_info.mtu); if (!vpninfo->cstp_pkt) { vpninfo->cstp_pkt = malloc(sizeof(struct pkt) + len); oncp.c ------ int len, kmp, kmplen, iplen; len = vpninfo->ip_info.mtu + vpninfo->pkt_trailer; if (!vpninfo->cstp_pkt) { vpninfo->cstp_pkt = malloc(sizeof(struct pkt) + len); gpst.c ----- int len = MAX(16384, vpninfo->ip_info.mtu); if (!vpninfo->cstp_pkt) { vpninfo->cstp_pkt = malloc(sizeof(struct pkt) + len); > >> There is no mechanism whatsoever to request or advertise the MTU of >> the HTTPS tunnel, so I don't really have another choice. (Clearly, >> this is a poor design? but I didn't design it.) > > And it looks like Juniper is also sending 1500-byte packets after > negotiating an MTU of 1400. Not negotiating is bad design. Negotiating > and then violating the agreement is worse :) Agreed. >> I'm not either. Perhaps David Woodhouse can weigh in on why he decided >> to drop the connection when Juniper packets exceed the MTU (this was >> added back in a47d69d3544e8d067c08aeb82e770daf8f635348). > > Because it was (supposedly!) a 'can never happen' condition. > > If they're actually going to send larger packets then ? as long as we > make bloody sure we're not going to overflow our allocations ? I > suspect we're better off actually receiving them. If they made them > through, why drop? I agree. "Be liberal in what you accept and conservative in what you transmit." In order to do this, I think it'd be good to make the packet-size-allocation consistent across all supported protocols. Perhaps by allocating at least a certain amount of headroom above the negotiated/estimated MTU, say 1024 bytes? I can submit a patch if that's desirable. -Dan
