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
| ||
|
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".
Powered by blists - more mailing lists