netdev - Re: SO_PEERSEC protections in sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQJ5VgTNiEhEhOtESRrK0q3-pUSbZfAWL=tXv-s2GXqq8Q@mail.gmail.com>
Date:   Fri, 7 Oct 2022 14:55:21 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Martin KaFai Lau <martin.lau@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        selinux@...r.kernel.org
Subject: Re: SO_PEERSEC protections in sk_getsockopt()?

On Fri, Oct 7, 2022 at 1:06 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> > On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul@...l-moore.com> wrote:
> > > On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@...l-moore.com> wrote:
> > > >
> > > > Hi Martin,
> > > >
> > > > In commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > > sockptr_t argument") I see you wrapped the getsockopt value/len
> > > > pointers with sockptr_t and in the SO_PEERSEC case you pass the
> > > > sockptr_t:user field to avoid having to update the LSM hook and
> > > > implementations.  I think that's fine, especially as you note that
> > > > eBPF does not support fetching the SO_PEERSEC information, but I think
> > > > it would be good to harden this case to prevent someone from calling
> > > > sk_getsockopt(SO_PEERSEC) with kernel pointers.  What do you think of
> > > > something like this?
> > > >
> > > >   static int sk_getsockopt(...)
> > > >   {
> > > >     /* ... */
> > > >     case SO_PEERSEC:
> > > >       if (optval.is_kernel || optlen.is_kernel)
> > > >         return -EINVAL;
> > > >       return security_socket_getpeersec_stream(...);
> > > >     /* ... */
> > > >   }
> > >
> > > Any thoughts on this Martin, Alexei?  It would be nice to see this
> > > fixed soon ...
> >
> > 'fixed' ?
> > I don't see any bug.
> > Maybe WARN_ON_ONCE can be added as a precaution, but also dubious value.
>
> Prior to the change it was impossible to call
> sock_getsockopt(SO_PEERSEC) with a kernel address space pointer, now
> with 4ff09db1b79b is it possible to call sk_getsockopt(SO_PEERSEC)
> with a kernel address space pointer and cause problems.

No. It's not possible. There is no path in the kernel that
can do that.

> Perhaps there
> are no callers in the kernel that do such a thing at the moment, but
> it seems like an easy mistake for someone to make, and the code to
> catch it is both trivial and out of any critical path.

Not easy at all.
There is only way place in the whole kernel that does:
                return sk_getsockopt(sk, SOL_SOCKET, optname,
                                     KERNEL_SOCKPTR(optval),
                                     KERNEL_SOCKPTR(optlen));

and there is an allowlist of optname-s right in front of it.
SO_PEERSEC is not there.
For security_socket_getpeersec_stream to be called with kernel
address the developer would need to add SO_PEERSEC to that allowlist.
Which will be trivially caught during the code review.

> This is one of those cases where preventing a future problem is easy,
> I think it would be foolish of us to ignore it.

Disagree. It's just a typical example of defensive programming
which I'm strongly against.
By that argument we should be checking all pointers for NULL
"because it's easy to do".