Recent kernels enforce system-wide unique L2TPv3 session IDs

[RalfJung]

Recent kernels enforce system-wide unique L2TPv3 session IDs

[kostko]

However, clients actually seem to send some rather arbitrary data alongside that message (UUUUUUUU, to be precise -- wtf?!?), so I am worried that attaching meaningful bytes here will not work very well.

Such padding is used in get cookie (and usage?) protocol messages in order to ensure a specific message length. This is so that the protocol does not enable traffic amplification attacks (e.g. where sending a small (with possibly spoofed source) message would generate a larger message in return).

At least in get cookie messages this was the reasoning. I did not implement the "usage" functionality, so I am not sure how it is used there, but it should be similar.

[RalfJung]

Such padding is used in get cookie (and usage?) protocol messages in order to ensure a specific message length. This is so that the protocol does not enable traffic amplification attacks (e.g. where sending a small (with possibly spoofed source) message would generate a larger message in return).

I see, thanks. However, there also is a "minimum package length" thing going on in context_send_packet. However, that one doesn't increase the length field sent inside the package, so it does not create any issues with possible protocol extensions.
Also, the server doesn't check the msg length for CONTROL_TYPE_USAGE, so this does not even help. (It does for CONTROL_TYPE_COOKIE though.) That's probably a bug then?

[kostko]

Also, the server doesn't check the msg length for CONTROL_TYPE_USAGE, so this does not even help. (It does for CONTROL_TYPE_COOKIE though.) That's probably a bug then?

Yes, I am not sure what the reasoning is with CONTROL_TYPE_USAGE having 8 bytes of padding (which is not even checked), as the response is only two additional bytes. For CONTROL_TYPE_COOKIE the padding length is the same as the cookie length. This probably needs to be fixed.

[RalfJung]

Actually, while the "clients don't retry a broken broker" may still be a good idea, it doesn't help here. The problem is old clients, and obviously patching this now won't help for them.

So, I am still inclined to have a client indicate, in CONTROL_TYPE_USAGE, after the 8 bytes of padding, whether it supports unique session IDs. It's not nice, but it is the only thing I can think of which makes sure that old clients keep working after we have some (but not all) of our servers updated to the new kernel. If we don't do think, it may happen that the machines with new kernels all have lower usage, and then old clients will never even try to connect to the one machine with the old kernel that would still work for them.

[kostko]

So we should probably add some kind of generic "supported features" field (e.g. a 32-bit bitmap field), which would indicate to the broker what kind of features are supported.

The question is where to put this field. If we put it in CONTROL_TYPE_USAGE messages, then there is no way to signal supported features if you don't use the usage-based broker selection. So in this case we would need to put it into CONTROL_TYPE_COOKIE messages as well (in this case, the broker can just reply with an error if the client doesn't support certain features).
Nevermind, I see that usage requests are always sent.

I don't like how this "usage" part was added to the protocol. It would be much better for the "usage" to be indicated as a feature in CONTROL_TYPE_COOKIE message and the broker would then include the usage message. But we can't really change this now without breaking the protocol.

Thinking about it, going forward, we should consider updating the protocol (there is already an 8-bit version field provided for that in all messages) so that the messages are based on a sequence of TLVs instead of fixed fields. This would make extending the protocol much easier in the future.

[RalfJung]

Nevermind, I see that usage requests are always sent.

Right, ever since 88df1c5.

Thinking about it, going forward, we should consider updating the protocol (there is already an 8-bit version field provided for that in all messages) so that the messages are based on a sequence of TLVs instead of fixed fields. This would make extending the protocol much easier in the future.

I don't disagree. But I wouldn't want to do this as part of fixing this problem.

So we should probably add some kind of generic "supported features" field (e.g. a 32-bit bitmap field), which would indicate to the broker what kind of features are supported.

What about the following: For now, we make it an 8-bit field. It is sent both in the COOKIE (after the 8-byte padding) and the PREPARE message (so the server does not need state). Absence of the field means all-0. The first bit (b & 0x1) is "supports unique session IDs". The other ones are not used. (I'd like to enforce this, but it would not be good if old servers would bail on new clients setting more flags.)

I mean, we could use 4 bytes, but that seems wasteful?

[kostko]

I don't disagree. But I wouldn't want to do this as part of fixing this problem.

Yes I agree that we should fix this first.

What about the following: For now, we make it an 8-bit field. It is sent both in the COOKIE (after the 8-byte padding) and the PREPARE message (so the server does not need state). Absence of the field means all-0. The first bit (b & 0x1) is "supports unique session IDs". The other ones are not used. (I'd like to enforce this, but it would not be good if old servers would bail on new clients setting more flags.)

Ok, sounds good.

[mitar]

I mean, we could use 4 bytes, but that seems wasteful?

Aren't these bytes send only in the first message? This is not too much. Maybe making it future proof to have more "supported features" if needed might be a good thing.

[RalfJung]

I don'r really care, so sure. Seems like I will have some fun with endianess then :/

[RalfJung]

I have a fix for this at https://github.com/freifunk-saar/tunneldigger/tree/wlanslovenija. The server side of this is currently already being tested in our network; for the client side I will hopefully soon have an experimental firmware but there are some other issues I want to look at.

Also, it is based on #61, so I am waiting for that to get merged.

[jhpoelen]

Seems like links to github commit with fix to linux-stable is broken.

Here's another reference to the patch https://patchwork.kernel.org/patch/9843481/ .