Subject: Re: [PATCH] TCP: add option for silent port knocking with integrity protection

From: Christian Grothoff <[email-masked]>
Date: Thu, 12 Dec 2013 16:07:17 +0100

On 12/12/2013 03:34 PM, Eric Dumazet wrote:
> On Thu, 2013-12-12 at 12:43 +0100, Christian Grothoff wrote:
>> On 12/12/2013 11:19 AM, Jacob Appelbaum wrote:
>>> I think that generally, I would prefer if the code didn't use MD5 but
>>> otherwise, I don't see any real risk of adding an exploitable hole. It
>>> seems silly to disable it by default though - ideally, I'd like a sysctl
>>> to ensure that Tor could use this without making the user recompile
>>> their kernel. That is more of a pain than running a userspace helper, I
>>> think.
>>>
>>> All the best,
>>> Jacob
>>
>> Given that the output is truncated to 32 bits and that performance (SYN
>> flood) is also a concern, AND that the original TCP SQN generation is
>> also MD5-based (and we want to look the same), what disadvantage do you
>> see over MD5?  Given the truncation to 32 bits, I don't think a stronger
>> hash would do anything for us.
>>
>> As for it being disabled by default, we did this with respect to
>> kernel submission guidelines which we understood said that features
>> should _initially_ always be submitted with disabled-by-default
>> (presumably so that until they have stabilized, nobody is harmed
>> unless they explicitly activate the code).
>>
>> I don't see the point in having a sysctl, as applications have to
>> explicitly request it anyway.
> 
> Wait a minute.
> 
> Your implementation looks like another TCP MD5 thing, but with a single
> secret. Check at git history to discover how many bugs we had to fix.
> Ask yourself how wrong was TCP MD5 being implemented in the kernel.
> 
> Very soon you'll need to support different secrets. You do not want all
> clients share a common secret, do you ? How can a server change its
> secret without disrupting clients ?

By doing another setsockopt() on the listen socket, which doesn't affect
existing connections.

> How having a constant initial sequence number can even be valid?

(1) technically, a constant ISN is 'valid', it is just not a good idea
    for various reasons which I suspect we don't need to discuss here.
(2) however, our ISNs are not necessarily constant, as
    (a) clients/servers may change the secret (see above), for example
        SilentKnock suggested using one-time secrets; our implementation
        does not preclude this;
    (b) alternatively, the scheme optionally includes TCP payload in
        what is being hashed; assuming a sane TCP protocol is used
	where the payload is reasonably random, this will result in
	a reasonably random ISN

Still, your concern that ISNs may not be sufficiently random for certain
applications using a global secret and no payload protection is valid.
There is an easy fix if TCP timestamps are available (throw the
timestamp into the MD5 hash operation), but we did not implement this
(yet). However, this might be a good idea.

> What about TCP timestamps being not available at all?

We're currently not using TCP timestamps; if we assumed that they were
always available, we would be able to better address your ISN issue
above, even for applications that use no payload protection and do not
change the secret.  That should be a minor modification to the existing
patch, but I read your point as you not wanting to use timestamps.

> How typical servers can be behind a load balancer?
> Or am I missing something?

I already agreed earlier that setups using a DNAT that changes SQNs for
load balancing won't work with this (and this is now documented on the
website).  Note that the patch requires the server to explicitly enable
this option, so in setups where it fundamentally doesn't work, the
simple answer is to not use it and the patch will do no harm.

Also, I'm not aware of any Tor bridges or GNUnet peers behind a load
balancer. The only SSH servers that are behind a load balancer that I'm
aware of are for HPC systems, and those have stronger protections from
Internet visibility than what Knock would possibly offer.  So the
use-cases I had in mind are not affected by the DNAT limitation.

> With various proposals (like TCP minion), maybe its time to be able to
> implement part of TCP stack in user land (Keep the mux inside the
> kernel, and forward raw incoming packets to user land where all the
> crazy things can be done without kernel patching.)

Sounds much like the GNU Hurd to me, which is a nice design. Still, I'm
not sure this (relatively tiny) patch really warrants moving TCP into
user land.

> To me your idea is very close from TCP fast open. The only difference
> would be for the server to not send its cookie in SYNACK ?

I'm not sure I see the relationship to TCP fast open.

> Sorry, you seem to hurry to get this facility, I do not see how it can
> really save the world. If it does, you definitely should get feedback
> from TCP community at IETF.

I'm already having fun with IETF and pTLDs right now, one war at a time
;-).  I also figured it might be easier to have a reasonable working
reference implementation first and then standardize.  After all, with my
recent draft some people at IETF suggested I should get 1,000,000+ users
first and then ask again.

Happy hacking!

-Christian