[PATCH 3/3] Drop packets that are too large without dropping connection

[Daniel Lenski]

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