| Message ID | 137399b962131c278acbfa5446a3b6d59aa0547b.1635784253.git.cdleonard@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,01/25] tcp: authopt: Initial support and key management | expand |
On 11/1/21 10:34 AM, Leonard Crestez wrote: > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c > index 97eb54774924..cc34de6e4817 100644 > --- a/net/ipv4/sysctl_net_ipv4.c > +++ b/net/ipv4/sysctl_net_ipv4.c > @@ -17,10 +17,11 @@ > #include <net/udp.h> > #include <net/cipso_ipv4.h> > #include <net/ping.h> > #include <net/protocol.h> > #include <net/netevent.h> > +#include <net/tcp_authopt.h> > > static int two = 2; > static int three __maybe_unused = 3; > static int four = 4; > static int thousand = 1000; > @@ -583,10 +584,19 @@ static struct ctl_table ipv4_table[] = { > .mode = 0644, > .proc_handler = proc_douintvec_minmax, > .extra1 = &sysctl_fib_sync_mem_min, > .extra2 = &sysctl_fib_sync_mem_max, > }, > +#ifdef CONFIG_TCP_AUTHOPT > + { > + .procname = "tcp_authopt", > + .data = &sysctl_tcp_authopt, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = proc_dointvec, Just add it to the namespace set, and this could be a u8 (try to plug a hole if possible) with min/max specified: .maxlen = sizeof(u8), .mode = 0644, .extra1 = SYSCTL_ZERO, .extra2 = SYSCTL_ONE see icmp_echo_enable_probe as an example. And if you are not going to clean up when toggled off, you need a handler that tells the user it can not be disabled by erroring out on attempts to disable it.
On 11/1/21 16:34, Leonard Crestez wrote: > This is mainly intended to protect against local privilege escalations > through a rarely used feature so it is deliberately not namespaced. > > Enforcement is only at the setsockopt level, this should be enough to > ensure that the tcp_authopt_needed static key never turns on. > > No effort is made to handle disabling when the feature is already in > use. > > Signed-off-by: Leonard Crestez <cdleonard@gmail.com> > --- [..] > diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c > index 5e80e5e5e36e..7c49dcce7d24 100644 > --- a/net/ipv4/tcp_authopt.c > +++ b/net/ipv4/tcp_authopt.c > @@ -3,10 +3,15 @@ > #include <linux/kernel.h> > #include <net/tcp.h> > #include <net/tcp_authopt.h> > #include <crypto/hash.h> > > +/* This is mainly intended to protect against local privilege escalations through > + * a rarely used feature so it is deliberately not namespaced. > + */ > +int sysctl_tcp_authopt; Could you add pr_warn_once() for setsockopt() without this set, so that it's visible in dmesg for a user that gets -EPERM. Thanks, Dmitry
On 11/3/21 4:39 AM, David Ahern wrote: > On 11/1/21 10:34 AM, Leonard Crestez wrote: >> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c >> index 97eb54774924..cc34de6e4817 100644 >> --- a/net/ipv4/sysctl_net_ipv4.c >> +++ b/net/ipv4/sysctl_net_ipv4.c >> @@ -17,10 +17,11 @@ >> #include <net/udp.h> >> #include <net/cipso_ipv4.h> >> #include <net/ping.h> >> #include <net/protocol.h> >> #include <net/netevent.h> >> +#include <net/tcp_authopt.h> >> >> static int two = 2; >> static int three __maybe_unused = 3; >> static int four = 4; >> static int thousand = 1000; >> @@ -583,10 +584,19 @@ static struct ctl_table ipv4_table[] = { >> .mode = 0644, >> .proc_handler = proc_douintvec_minmax, >> .extra1 = &sysctl_fib_sync_mem_min, >> .extra2 = &sysctl_fib_sync_mem_max, >> }, >> +#ifdef CONFIG_TCP_AUTHOPT >> + { >> + .procname = "tcp_authopt", >> + .data = &sysctl_tcp_authopt, >> + .maxlen = sizeof(int), >> + .mode = 0644, >> + .proc_handler = proc_dointvec, > > Just add it to the namespace set, and this could be a u8 (try to plug a > hole if possible) with min/max specified: > > .maxlen = sizeof(u8), > .mode = 0644, > .extra1 = SYSCTL_ZERO, > .extra2 = SYSCTL_ONE > > > see icmp_echo_enable_probe as an example. And if you are not going to > clean up when toggled off, you need a handler that tells the user it can > not be disabled by erroring out on attempts to disable it. This is deliberately per-system because the goal is to avoid possible local privilege escalations by reducing the attack surface. Even the smallest flaw could be exploited by a malicious application establishing an authenticated connection on loopback. Applications running in containers frequently have full access to sysctls so making this per-namespace would defeat the original purpose. I can't think of any reason to prevent using this feature at the namespace level, it has no interesting effects outside TCP connections for which it is enabled. I also believe that as similar sysctl would be useful for TCP-MD5. You're right about adding additional prints. -- Regards, Leonard
diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst index 16b8bf72feaf..3f00681f73d7 100644 --- a/Documentation/networking/ip-sysctl.rst +++ b/Documentation/networking/ip-sysctl.rst @@ -987,10 +987,16 @@ tcp_limit_output_bytes - INTEGER tcp_challenge_ack_limit - INTEGER Limits number of Challenge ACK sent per second, as recommended in RFC 5961 (Improving TCP's Robustness to Blind In-Window Attacks) Default: 1000 +tcp_authopt - BOOLEAN + Enable the TCP Authentication Option (RFC5925), a replacement for TCP + MD5 Signatures (RFC2835). + + Default: 0 + UDP variables ============= udp_l3mdev_accept - BOOLEAN Enabling this option allows a "global" bound socket to work diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h index 8bb76128ed11..a505db1dd67b 100644 --- a/include/net/tcp_authopt.h +++ b/include/net/tcp_authopt.h @@ -65,10 +65,11 @@ struct tcp_authopt_info { /** @dst_isn: Remote Initial Sequence Number */ u32 dst_isn; }; #ifdef CONFIG_TCP_AUTHOPT +extern int sysctl_tcp_authopt; DECLARE_STATIC_KEY_FALSE(tcp_authopt_needed); void tcp_authopt_free(struct sock *sk, struct tcp_authopt_info *info); void tcp_authopt_clear(struct sock *sk); int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen); diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c index 97eb54774924..cc34de6e4817 100644 --- a/net/ipv4/sysctl_net_ipv4.c +++ b/net/ipv4/sysctl_net_ipv4.c @@ -17,10 +17,11 @@ #include <net/udp.h> #include <net/cipso_ipv4.h> #include <net/ping.h> #include <net/protocol.h> #include <net/netevent.h> +#include <net/tcp_authopt.h> static int two = 2; static int three __maybe_unused = 3; static int four = 4; static int thousand = 1000; @@ -583,10 +584,19 @@ static struct ctl_table ipv4_table[] = { .mode = 0644, .proc_handler = proc_douintvec_minmax, .extra1 = &sysctl_fib_sync_mem_min, .extra2 = &sysctl_fib_sync_mem_max, }, +#ifdef CONFIG_TCP_AUTHOPT + { + .procname = "tcp_authopt", + .data = &sysctl_tcp_authopt, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec, + }, +#endif { } }; static struct ctl_table ipv4_net_table[] = { { diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c index 5e80e5e5e36e..7c49dcce7d24 100644 --- a/net/ipv4/tcp_authopt.c +++ b/net/ipv4/tcp_authopt.c @@ -3,10 +3,15 @@ #include <linux/kernel.h> #include <net/tcp.h> #include <net/tcp_authopt.h> #include <crypto/hash.h> +/* This is mainly intended to protect against local privilege escalations through + * a rarely used feature so it is deliberately not namespaced. + */ +int sysctl_tcp_authopt; + /* This is enabled when first struct tcp_authopt_info is allocated and never released */ DEFINE_STATIC_KEY_FALSE(tcp_authopt_needed); EXPORT_SYMBOL(tcp_authopt_needed); /* All current algorithms have a mac length of 12 but crypto API digestsize can be larger */ @@ -360,10 +365,12 @@ int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen) struct tcp_authopt opt; struct tcp_authopt_info *info; int err; sock_owned_by_me(sk); + if (!sysctl_tcp_authopt) + return -EPERM; err = _copy_from_sockptr_tolerant((u8 *)&opt, sizeof(opt), optval, optlen); if (err) return err; @@ -382,13 +389,15 @@ int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen) int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt) { struct tcp_sock *tp = tcp_sk(sk); struct tcp_authopt_info *info; + memset(opt, 0, sizeof(*opt)); sock_owned_by_me(sk); + if (!sysctl_tcp_authopt) + return -EPERM; - memset(opt, 0, sizeof(*opt)); info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk)); if (!info) return -ENOENT; opt->flags = info->flags & TCP_AUTHOPT_KNOWN_FLAGS; @@ -451,10 +460,12 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen) struct tcp_authopt_key_info *key_info, *old_key_info; struct tcp_authopt_alg_imp *alg; int err; sock_owned_by_me(sk); + if (!sysctl_tcp_authopt) + return -EPERM; err = _copy_from_sockptr_tolerant((u8 *)&opt, sizeof(opt), optval, optlen); if (err) return err;
This is mainly intended to protect against local privilege escalations through a rarely used feature so it is deliberately not namespaced. Enforcement is only at the setsockopt level, this should be enough to ensure that the tcp_authopt_needed static key never turns on. No effort is made to handle disabling when the feature is already in use. Signed-off-by: Leonard Crestez <cdleonard@gmail.com> --- Documentation/networking/ip-sysctl.rst | 6 ++++++ include/net/tcp_authopt.h | 1 + net/ipv4/sysctl_net_ipv4.c | 10 ++++++++++ net/ipv4/tcp_authopt.c | 13 ++++++++++++- 4 files changed, 29 insertions(+), 1 deletion(-)