syncache: accept packets with no SA when TCP_MD5SIG is set

[rew]

Summary

When TCP_MD5SIG is set on a socket, all packets are dropped that don't
contain an MD5 signature. Relax this behavior by allowing sockets set
with TCP_MD5SIG to accept packets from peers that do not have a
security assocation set up.

Packets from peers that have security assocations and did not provide
a signature, will be dropped.

Packets from peers that do not have security associations and do not
provide a signature, will be accepted.

This behavior is consistent with OpenBSD.

tcpmd5: modify tcp_ipsec_input() to handle a NULL buf argument.

This provides a mechanism that will let the caller check if the incoming
packet has a security association, but without computing the signature

[bz]

sys/netinet/tcp_syncache.c

1534

This makes me think is not right. If the socket does not enable TCP-MD5 you never want to call into netkey to check for an SA in first place as otherwise an attacker can simply turn the bit on and make you do a lot of per-packet work which you are trying to avoid in the syncache.

I think you do want to do this:

if (socket has tcpmd5 enabled) {
     if (packet has a signature) {
         if (!TCPMD5_ENABLED || signature validation fails)
             goto done;  // drop packet
     } else {  // no signature
         if (TCPMD5_ENABLED && != ENOENT) {
             goto done;  // tcpmd5 processing is on and we have a SA so drop it.   
     }
} else if (packet has a signature)  // would never validate so drop it.
        goto done;

1535

This line could stay on the same line as the } above.

1541

This is wrong. I've been staring at this for minutes now. You are dropping the packet if there is !TCPMD5_ENABLED() and do not check for a SA anymore; but that is exactly what you do not want, right?
I hope I got it right above.

sys/netipsec/xform_tcp.c

275

Is there a reason to use EPERM here? Do we need to distinguish this case outside this function anyway in a caller to EACCESS?

[rew]

sys/netinet/tcp_syncache.c

1534

This makes me think is not right. If the socket does not enable TCP-MD5 you never want to call into netkey to check for an SA in first place as otherwise an attacker can simply turn the bit on and make you do a lot of per-packet work which you are trying to avoid in the syncache.

This explanation helps, thanks. So if I understand, the expected behavior is:

• If TCP-MD5 is enabled on the socket; signed and non-signed packets are accepted (assuming the non-signed packets don't have a SA).
• If TCP-MD5 is not enabled on the socket, all signed packets are dropped (even if the signed packet has a SA).

Is that correct?

1541

This is wrong. I've been staring at this for minutes now. You are dropping the packet if there is !TCPMD5_ENABLED() and do not check for a SA anymore; but that is exactly what you do not want, right?

Right, yea - I think you got it with TCPMD5_ENABLED() && != ENOENT for the scenario when the socket is set for TCP-MD5 but ipsec isn't enabled..

[bz]

sys/netinet/tcp_syncache.c

1534

That is what I would expect it to be; the socket option is the big ON/OFF switch. Probably should also update tcp(4) man page which describes the old behaviour.

[rew]

address feedback from bz

[rew]

sys/netinet/tcp_syncache.c

1534

Probably should also update tcp(4) man page which describes the old behaviour.

Will do.

This change will partially restore the behavior as it was before fcf596178b5f2be3 was merged.

I'll think of how to word it, but maybe something like (more elaboration?):

A socket enabled with TCP_MD5SIG will only accept non-signed segments for which an SADB entry cannot be found.

sys/netipsec/xform_tcp.c

275

Is there a reason to use EPERM here?

Not specifically, my thinking was to have a different error code for each return.

I've changed it to EACCESS

[bz]

Otherwise I'd be okay with this then (in the end it seems we are changing one if/else clause compared to the latest implementation and the extra "early exit case" in xform.

sys/netinet/tcp_syncache.c

1737

I only just noticed this. I guess I was focused on the above change last time and missed this. Sorry.

Why would you make your own policy processing dependent on what a peer sends you?
That seems wrong and kind-of is hinted in the original RFC to not be the case in the last paragraph on section 2.0 of rfc2385.

[rew]

In D33227#755522, @bz wrote:

Otherwise I'd be okay with this then (in the end it seems we are changing one if/else clause compared to the latest implementation and the extra "early exit case" in xform.

Just to make sure I'm not missing something here, you're talking about returning EACCES vs EPERM?

sys/netinet/tcp_syncache.c

1737

Why would you make your own policy processing dependent on what a peer sends you?

By the time we get here, a packet with a signature has already been verified (or dropped if it's not valid).

This change was for the logic in the original review where I had it set up that non-TCPMD5 socket would accept a packet with a signature provided that a SA existed between sender/receiver.

As the logic is now, I don't think it's wrong per se - but it doesn't add anything useful either - I'll switch this back to how it was before.

[rew]

• set SCF_SIGNATURE when socket is enabled with TCP-MD5

• update tcp(4) to reflect new behavior

[bz]

I am fine with this; I'd hope others too?

[pauamma_gundo.com]

As always, addressing the documentation, not the code or the consistency between them.

share/man/man4/tcp.4

343–346

For concision and taste, and to make sure the exception is more visible, I'd merge this and the paragraph above, perhaps by using "However, " instead of the first sentence.

[ae]

I have no objection. Just a question. What is the reason behind the accepting of non-signed packets on the socket that we explicitly marked that it must use signatures?
I think this will lead to hiding misconfigurations from the user.

[bz]

In D33227#755740, @ae wrote:

I have no objection. Just a question. What is the reason behind the accepting of non-signed packets on the socket that we explicitly marked that it must use signatures?
I think this will lead to hiding misconfigurations from the user.

Because otherwise your listen socket cannot have protected and unprotected connections; 5 BGP speakers do MD5, 7 don't.
The socket option is the "turn the feature on", the SADB is your policy to whom you do MD5 and to whom you don't.

[ae]

In D33227#755751, @bz wrote:

Because otherwise your listen socket cannot have protected and unprotected connections; 5 BGP speakers do MD5, 7 don't.
The socket option is the "turn the feature on", the SADB is your policy to whom you do MD5 and to whom you don't.

Thanks, this sounds reasonable. It would be good to have such description in the commit message :)

[rew]

sys/netinet/tcp_syncache.c

1737

sorry for bringing this back up

SCF_SIGNATURE should be set if the packet is signed.

It's possible we might be receiving a non-signed packet on a TCPMD5 enabled socket. And if we set based on the socket, we will respond with a signed packet instead of a non-signed packet.

[rew]

Set syncache signature flag when received packet has a signature.

At this point in the code, a packet with an MD5 signature has
already been validated or dropped.

If we set the syncache signature flag based on whether the socket is
TCPMD5 enabled, then we will send signed segments to a peer wanting
to establish a connection that does not require TCPMD5.

[rew]

feedback from Pau Amma

clarify TCP_MD5SIG behavior when a non-signed segment is accepted