Page MenuHomeFreeBSD

syncache: accept packets with no SA when TCP_MD5SIG is set
ClosedPublic

Authored by rew on Dec 1 2021, 11:09 PM.

Details

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

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rew requested review of this revision.Dec 1 2021, 11:09 PM
bz requested changes to this revision.Dec 8 2021, 10:53 PM
bz added inline comments.
sys/netinet/tcp_syncache.c
1533

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;
1534

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

1540

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
274

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

This revision now requires changes to proceed.Dec 8 2021, 10:53 PM
sys/netinet/tcp_syncache.c
1533

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?

1540

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..

sys/netinet/tcp_syncache.c
1533

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 marked 2 inline comments as not done.

address feedback from bz

rew marked 4 inline comments as done.Dec 13 2021, 7:07 AM
rew added inline comments.
sys/netinet/tcp_syncache.c
1533

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
274

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 requested changes to this revision.Dec 13 2021, 7:19 PM

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.

This revision now requires changes to proceed.Dec 13 2021, 7:19 PM
rew marked an inline comment as done.Dec 13 2021, 8:03 PM
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.

  • set SCF_SIGNATURE when socket is enabled with TCP-MD5
  • update tcp(4) to reflect new behavior

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

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

share/man/man4/tcp.4
342–345

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.

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.

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.

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 :)

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.

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.

feedback from Pau Amma

clarify TCP_MD5SIG behavior when a non-signed segment is accepted

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Jan 9, 1:45 AM
This revision was automatically updated to reflect the committed changes.