SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

On Sun, Oct 9, 2022 at 3:01 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Fri, Oct 7, 2022 at 5:55 PM Alexei Starovoitov
> <alexei.starovoitov at gmail.com> wrote:
> > 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.
>
> If we look at the very next sentence in my last reply you see that I
> acknowledge that there may be no callers that currently do that, but
> it seems like an easy mistake for someone to make.  I've seen kernel
> coding errors similar to this in the past, it seems like a reasonable
> thing to protect against, especially considering it is well outside of
> any performance critical path.
>
> > > 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.
>
> A couple of things come to mind ... First, the concern isn't the
> existing caller(s), as mentioned above, but future callers.  Second,
> while the kernel code review process is good, the number of serious
> kernel bugs that have passed uncaught through the code review process
> is staggering.
>
> > > 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.
>
> That's a pretty bold statement, good luck with that.
>
> > By that argument we should be checking all pointers for NULL
> > "because it's easy to do".
>
> That's not the argument being made here, but based on your previous
> statements of trusting code review to catch bugs and your opposition
> to defensive programming it seems pretty unlikely we're going to find
> common ground.
>
> I'll take care of this in the LSM tree.

Are you saying you'll add a patch to sk_getsockopt
in net/core/sock.c without going through net or bpf trees?
Paul, you're crossing the line.