SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

On Mon, Oct 10, 2022 at 10:10 AM Alexei Starovoitov
<alexei.starovoitov at gmail.com> wrote:
> On Mon, Oct 10, 2022 at 6:29 AM Paul Moore <paul at paul-moore.com> wrote:
> > On Mon, Oct 10, 2022 at 2:19 AM Alexei Starovoitov
> > <alexei.starovoitov at gmail.com> wrote:
> > > 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.
> >
> > I believe my exact comment was "I'll take care of this in the LSM
> > tree."  I haven't thought tpo hard about the details yet, but thinking
> > quickly I can imagine several different approaches with varying levels
> > of change required in sk_getsockopt(); it would be premature to
> > comment much beyond that.  It also looks like David Laight has similar
> > concerns, so it's possible he might work on resolving this too,
> > discussions are (obviously) ongoing.
> >
> > As far as crossing a line is concerned, I suggest you first look in
> > the mirror with respect to changes in the security/ subdir that did
> > not go through one of the LSM trees.  There are quite a few patches
> > from netdev/bpf that have touched security/ without going through a
> > LSM tree or getting a Reviewed-by/Acked-by/etc. from a LSM developer.
> > In fact I don't even have to go back a year and I see at least one
> > patch that touches code under security/ that was committed by you
> > without any LSM developer reviews/acks/etc.
>
> Since you're going to take patches to sock.c despite objections
> please make sure to add my Nack and see you at the next merge window.

I never committed to that, did I?

> Enough of this useless thread that destroyed trust and respect
> over complete non-issue.

I suggest re-reading this thread from the start in a few days, time
might soften some of your reactions and thoughts on this.

-- 
paul-moore.com