Details

Reviewers

bz
ae
melifaro

Group Reviewers

transport
manpages

Commits

rG7205809809e7: syncache: accept packet with no SA when TCP_MD5SIG is set
rGeb18708ec8c7: syncache: accept packet with no SA when TCP_MD5SIG is set

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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 43325
Build 40213: arc lint + arc unit

Event Timeline

rew created this revision.Dec 1 2021, 11:09 PM

Herald added a reviewer: transport. · View Herald TranscriptDec 1 2021, 11:09 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald Transcript

rew requested review of this revision.Dec 1 2021, 11:09 PM

Harbormaster completed remote builds in B43101: Diff 99336.Dec 1 2021, 11:09 PM

rew added a parent revision: D33226: tcpmd5: return ENOENT when security association not found.Dec 1 2021, 11:09 PM

bz added a reviewer: bz.Dec 2 2021, 9:20 PM

allanjude added reviewers: ae, melifaro.Dec 8 2021, 8:33 PM

bz requested changes to this revision.Dec 8 2021, 10:53 PM

bz added inline comments.

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?

This revision now requires changes to proceed.Dec 8 2021, 10:53 PM

rew added inline comments.Dec 9 2021, 2:24 AM

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 added inline comments.Dec 9 2021, 2:14 PM

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.

address feedback from bz

Harbormaster completed remote builds in B43306: Diff 99909.Dec 13 2021, 6:07 AM

rew marked 4 inline comments as done.Dec 13 2021, 7:07 AM

rew added inline comments.

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

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

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

Herald added a reviewer: manpages. · View Herald TranscriptDec 13 2021, 8:51 PM

Harbormaster completed remote builds in B43325: Diff 99958.Dec 13 2021, 8:51 PM

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

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

rew added inline comments.Dec 15 2021, 10:35 PM

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.

Harbormaster completed remote builds in B43513: Diff 100373.Dec 20 2021, 8:12 PM

feedback from Pau Amma

clarify TCP_MD5SIG behavior when a non-signed segment is accepted

Harbormaster completed remote builds in B43688: Diff 100898.Jan 3 2022, 9:30 PM

This revision was not accepted when it landed; it landed in state Needs Review.Jan 9 2022, 1:45 AM

Closed by commit rGeb18708ec8c7: syncache: accept packet with no SA when TCP_MD5SIG is set (authored by rew). · Explain Why

This revision was automatically updated to reflect the committed changes.

rew added a commit: rGeb18708ec8c7: syncache: accept packet with no SA when TCP_MD5SIG is set.

rew added a commit: rG7205809809e7: syncache: accept packet with no SA when TCP_MD5SIG is set.Feb 10 2022, 7:33 PM