SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

On Fri, Oct 7, 2022 at 1:06 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
> <alexei.starovoitov at gmail.com> wrote:
> > On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul at paul-moore.com> wrote:
> > > On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul at paul-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".