lkml.org 
[lkml]   [2022]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v8 08/26] tcp: authopt: Disable via sysctl by default
    From
    On 9/7/22 02:11, Eric Dumazet wrote:
    > On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez <cdleonard@gmail.com> 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>
    >> ---
    >> Documentation/networking/ip-sysctl.rst | 6 ++++
    >> include/net/tcp_authopt.h | 1 +
    >> net/ipv4/sysctl_net_ipv4.c | 39 ++++++++++++++++++++++++++
    >> net/ipv4/tcp_authopt.c | 25 +++++++++++++++++
    >> 4 files changed, 71 insertions(+)
    >>
    >> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
    >> index a759872a2883..41be0e69d767 100644
    >> --- a/Documentation/networking/ip-sysctl.rst
    >> +++ b/Documentation/networking/ip-sysctl.rst
    >> @@ -1038,10 +1038,16 @@ tcp_challenge_ack_limit - INTEGER
    >> Note that this per netns rate limit can allow some side channel
    >> attacks and probably should not be enabled.
    >> TCP stack implements per TCP socket limits anyway.
    >> Default: INT_MAX (unlimited)
    >>
    >> +tcp_authopt - BOOLEAN
    >> + Enable the TCP Authentication Option (RFC5925), a replacement for TCP
    >> + MD5 Signatures (RFC2835).
    >> +
    >> + Default: 0
    >> +

    ...

    >> +#ifdef CONFIG_TCP_AUTHOPT
    >> +static int proc_tcp_authopt(struct ctl_table *ctl,
    >> + int write, void *buffer, size_t *lenp,
    >> + loff_t *ppos)
    >> +{
    >> + int val = sysctl_tcp_authopt;
    >
    > val = READ_ONCE(sysctl_tcp_authopt);
    >
    >> + struct ctl_table tmp = {
    >> + .data = &val,
    >> + .mode = ctl->mode,
    >> + .maxlen = sizeof(val),
    >> + .extra1 = SYSCTL_ZERO,
    >> + .extra2 = SYSCTL_ONE,
    >> + };
    >> + int err;
    >> +
    >> + err = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
    >> + if (err)
    >> + return err;
    >> + if (sysctl_tcp_authopt && !val) {
    >
    > READ_ONCE(sysctl_tcp_authopt)
    >
    > Note that this test would still be racy, because another cpu might
    > change sysctl_tcp_authopt right after the read.

    What meaningful races are possible here? This is a variable that changes
    from 0 to 1 at most once.

    In theory if two processes attempt to assign "non-zero" at the same time
    then one will "win" and the other will get an error but races between
    userspace writing different values are possible for any sysctl. The
    solution seems to be "write sysctls from a single place".

    All the checks are in sockopts - in theory if the sysctl is written on
    one CPU then a sockopt can still fail on another CPU until caches are
    flushed. Is this what you're worried about?

    In theory doing READ_ONCE might incur a slight penalty on sockopt but
    not noticeable.

    >
    >> + net_warn_ratelimited("Enabling TCP Authentication Option is permanent\n");
    >> + return -EINVAL;
    >> + }
    >> + sysctl_tcp_authopt = val;
    >
    > WRITE_ONCE(sysctl_tcp_authopt, val), or even better:
    >
    > if (val)
    > cmpxchg(&sysctl_tcp_authopt, 0, val);
    >
    >> + return 0;
    >> +}
    >> +#endif
    >> +

    This would be useful if we did any sort of initialization here but we
    don't. Crypto is initialized somewhere completely different.

    \
     
     \ /
      Last update: 2022-09-07 18:54    [W:4.039 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site