SO_PEERSEC protections in sk_getsockopt()?

SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

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(...);
    /* ... */
  }

-- 
paul-moore.com

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@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 ...

-- 
paul-moore.com

Re: SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@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.

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

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

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

--
paul-moore.com

Re: SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

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

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

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

-- 
paul-moore.com

Re: SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

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

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

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

-- 
paul-moore.com

Re: SO_PEERSEC protections in sk_getsockopt()?

[Alexei Starovoitov]

On Mon, Oct 10, 2022 at 6:29 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Oct 10, 2022 at 2:19 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Sun, Oct 9, 2022 at 3:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Oct 7, 2022 at 5:55 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > On Fri, Oct 7, 2022 at 1:06 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > > On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@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.
Enough of this useless thread that destroyed trust and respect
over complete non-issue.

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

On Mon, Oct 10, 2022 at 10:10 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Mon, Oct 10, 2022 at 6:29 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Oct 10, 2022 at 2:19 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > On Sun, Oct 9, 2022 at 3:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Oct 7, 2022 at 5:55 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > On Fri, Oct 7, 2022 at 1:06 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
> > > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > > > On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > > On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@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

RE: SO_PEERSEC protections in sk_getsockopt()?

[David Laight]

From: Alexei Starovoitov
> Sent: 07 October 2022 22:55
....
> 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));

Until I add change my out of tree driver to work with
the new code.
(Although it actually needs to do a getsockopt into SCTP.)

I didn't spot the change to sk_getsockopt() going though.
But KERNEL_SOCKPTR() is really the wrong function/type
for the length.
It would be much safer to have a struct with two members,
one an __user pointer and one a kernel pointer both to
socklen_t.

It isn't really ideal for the buffer pointer either.
That started as a single field (assuming the caller
has verified the user/kernel status), then the is_kernel
field was added for architectures where user/kernel
addresses use the same values.
Then a horrid bug (forgotten where) forced the is_kernel
field be used everywhere.
Again a structure with two pointers would be much safer.

Indeed the length could likely be included as well.
That would even give scope for a short user buffer being
copied into kernel memory while letting code that needs
a long buffer (or ignores the length) still directly
access userspace.

I can't remember, but something makes me think that a lot
of the 'not checking the length' setsockopt calls were in
decnet - which has now been deleted.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

On Mon, Oct 10, 2022 at 8:54 AM David Laight <David.Laight@aculab.com> wrote:
> From: Alexei Starovoitov
> > Sent: 07 October 2022 22:55
> ....
> > 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));
>
> Until I add change my out of tree driver to work with
> the new code.
> (Although it actually needs to do a getsockopt into SCTP.)
>
> I didn't spot the change to sk_getsockopt() going though.
> But KERNEL_SOCKPTR() is really the wrong function/type
> for the length.
> It would be much safer to have a struct with two members,
> one an __user pointer and one a kernel pointer both to
> socklen_t.

Yes, agreed.

> It isn't really ideal for the buffer pointer either.
> That started as a single field (assuming the caller
> has verified the user/kernel status), then the is_kernel
> field was added for architectures where user/kernel
> addresses use the same values.
> Then a horrid bug (forgotten where) forced the is_kernel
> field be used everywhere.
> Again a structure with two pointers would be much safer.

Any chance you have plans to work on this David?

-- 
paul-moore.com

RE: SO_PEERSEC protections in sk_getsockopt()?

[David Laight]

From: Paul Moore
> Sent: 10 October 2022 14:19
....
> > It isn't really ideal for the buffer pointer either.
> > That started as a single field (assuming the caller
> > has verified the user/kernel status), then the is_kernel
> > field was added for architectures where user/kernel
> > addresses use the same values.
> > Then a horrid bug (forgotten where) forced the is_kernel
> > field be used everywhere.
> > Again a structure with two pointers would be much safer.
> 
> Any chance you have plans to work on this David?

I'd only spend any significant time on it if there
is a reasonable chance of the patches being accepted.

My use would be an out-of-tree non-GPL module calling
kernel_getsockopt().
The main in-tree user is bpf - which seems to need an
ever-increasing number of socket options, but support has
been added one by one.

While most getsockopt() calls just return set values, SCTP
uses some to retrieve the result of values negotiated with
the peer. The number of valid data streams is needed for
even trivial SCTP applications.
However I've a workaround for a bug in 5.1 to 5.8 that
returned the wrong values (my tests didn't check negotiation)
that also obtains the values on later kernels.
So I'm not (yet) in a hurry!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: SO_PEERSEC protections in sk_getsockopt()?

[Paul Moore]

On Mon, Oct 10, 2022 at 11:34 AM David Laight <David.Laight@aculab.com> wrote:
> From: Paul Moore
> > Sent: 10 October 2022 14:19
> ....
> > > It isn't really ideal for the buffer pointer either.
> > > That started as a single field (assuming the caller
> > > has verified the user/kernel status), then the is_kernel
> > > field was added for architectures where user/kernel
> > > addresses use the same values.
> > > Then a horrid bug (forgotten where) forced the is_kernel
> > > field be used everywhere.
> > > Again a structure with two pointers would be much safer.
> >
> > Any chance you have plans to work on this David?
>
> I'd only spend any significant time on it if there
> is a reasonable chance of the patches being accepted.
>
> My use would be an out-of-tree non-GPL module calling
> kernel_getsockopt().
> The main in-tree user is bpf - which seems to need an
> ever-increasing number of socket options, but support has
> been added one by one.
>
> While most getsockopt() calls just return set values, SCTP
> uses some to retrieve the result of values negotiated with
> the peer. The number of valid data streams is needed for
> even trivial SCTP applications.
> However I've a workaround for a bug in 5.1 to 5.8 that
> returned the wrong values (my tests didn't check negotiation)
> that also obtains the values on later kernels.
> So I'm not (yet) in a hurry!

It looks like it might still be a good idea to add hardening/support
for the LSM hook as your needs still seem a bit far off, but I
appreciate the background - thanks!

-- 
paul-moore.com