* Re: [PATCH v4 2/3] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Kees Cook @ 2019-04-11 17:29 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAG_fn=Vkod40bxxP8PU-+zXs2u4arqAB_bYxqei1bb_tgsg8GA@mail.gmail.com>
On Thu, Apr 11, 2019 at 1:39 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Wed, Apr 10, 2019 at 6:09 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Apr 10, 2019 at 6:18 AM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > This config option adds the possibility to initialize newly allocated
> > > pages and heap objects with a 0xAA pattern.
> > > There's already a number of places where allocations are initialized
> > > based on the presence of __GFP_ZERO flag. We just change this code so
> > > that under CONFIG_INIT_ALL_HEAP these allocations are always initialized
> > > with either 0x00 or 0xAA depending on the __GFP_ZERO.
> >
> > Why not just make __GFP_ZERO unconditional instead? This looks like
> > it'd be simpler and not need arch-specific implementation?
>
> Right, but it would mean we can only initialize with 0x00 pattern.
> I believe that for testing purposes a nonzero pattern is better,
Can it be implemented in a way that isn't arch-specific? I'd really
like to have a general solution that works immediately for all
architectures. (Can't everything just use a memset?)
> because it'll not only assure the execution is deterministic, but will
> also uncover logic bugs earlier (see the discussion at
> https://reviews.llvm.org/D54604?id=174471)
> For hardening purposes the pattern shouldn't matter much.
So, for hardening, it actually does matter but only in certain cases.
On 64-bit, a 0xAA... pointer will have the high bit set, so it'll land
in the non-canonical memory range, which is good. For 32-bit, 0xAA...
will be in userspace (TASK_SIZE is 0xC0000000). In the above URL I see
now that 32-bit pointer init gets 0x000000AA, which is good, but for
heap init, types aren't known. So perhaps use 0x000000AA for 32-bit
and 0xAA... for 64-bit heap init? (0xAA... has stronger properties
since there have been NULL page mapping bypass flaws in the (recent!)
past, so I could see keeping that for 64-bit instead of just using
0-init everywhere.)
> If you think arch-specific code is too much of a trouble, we could
> implement clear_page_pattern() using memset() on every architecture,
> but allow the user to choose between slow (0xAA) and production (0x00)
> modes.
How about 32-bit use 0x00, 64-bit use 0xAA (and provide per-arch
speed-ups with a generic "slow" version for all the other
architectures?)
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 3/3] kbuild: Implement Clang's stack initialization
From: Kees Cook @ 2019-04-11 17:07 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Alexander Popov, Michal Marek, Emese Revfy, James Morris,
Serge E. Hallyn, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAK7LNAQpm5Tz_Fo0rSq00emB7BLnfYB-+mwXd9GiMKAqKCfkrA@mail.gmail.com>
On Thu, Apr 11, 2019 at 1:06 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Thu, Apr 11, 2019 at 1:16 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > CONFIG_INIT_STACK_ALL turns on stack initialization based on
> > -ftrivial-auto-var-init in Clang builds and on
> > -fplugin-arg-structleak_plugin-byref-all in GCC builds.
>
> Is CONFIG_INIT_STACK_ALL wired up to GCC plugin in any way?
> I could not understand it from the code.
No, it's only available under Clang. Clang is all-or-nothing, and the
GCC plugin has a degrees up to "all passed by reference" which isn't
truly "all" (i.e. Clang will initialize variables that aren't passed
by reference and trigger a compiler warning about being
uninitialized.)
> > choice
> > prompt "Initialize kernel stack variables at function entry"
> > depends on CC_HAS_AUTO_VAR_INIT || GCC_PLUGINS
> > + default INIT_STACK_ALL if CC_HAS_AUTO_VAR_INIT
>
> Why should this be enabled by default?
> Ins't it a performance regression
> since it inserts instructions in function prologue?
There are very few users of Clang right now (mainly Android), so I
figured it'd be nice to start Clang builds from a "protected by
default" here, especially given Linus's thoughts on making this always
happen[1]. I don't want to do it for GCC yet, since that would likely
come as a huge surprise to everyone else. :) But I'm happy to change
this, of course.
-Kees
[1] https://lkml.kernel.org/r/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j942dA@mail.gmail.com
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 1/3] Kconfig: Create "kernel hardening" config area
From: Kees Cook @ 2019-04-11 16:59 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Alexander Popov, Michal Marek, Emese Revfy, James Morris,
Serge E. Hallyn, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAK7LNASLdZKNNM-Gft5bfVxKOG=4Ak-LBMnjb2E-QR4q_5GmEQ@mail.gmail.com>
On Thu, Apr 11, 2019 at 1:51 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Thu, Apr 11, 2019 at 1:16 AM Kees Cook <keescook@chromium.org> wrote:
> > diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
> > index 74271dba4f94..01874ef0f883 100644
> > --- a/scripts/gcc-plugins/Kconfig
> > +++ b/scripts/gcc-plugins/Kconfig
> > @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS
> > An arch should select this symbol if it supports building with
> > GCC plugins.
> >
> > -menuconfig GCC_PLUGINS
> > - bool "GCC plugins"
> > +config GCC_PLUGINS
> > + bool
>
>
> This will flatten the plugin config options.
>
> If you want to keep the current menu structure, you can do:
>
> menu "GCC plugins"
> ...
> endmenu
Ah, excellent point. I'll fix this.
> Another side-effect is Kbuild will descend into scripts/gcc-plugins/
> even when no plugin is selected.
> It is not a big build speed regression, though.
I suspect the plugins Kconfig may disppear eventually with the options
spread around other Kconfigs (since now the plugin capability is known
at config time).
> > diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> > new file mode 100644
> > index 000000000000..8223a8ab1a12
> > --- /dev/null
> > +++ b/security/Kconfig.hardening
> > @@ -0,0 +1,94 @@
> > +menu "Kernel hardening options"
> > +
> > +config GCC_PLUGIN_STRUCTLEAK
> > + bool
> > + depends on GCC_PLUGIN_STRUCTLEAK_USER || GCC_PLUGIN_STRUCTLEAK_BYREF || GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
>
>
> I think this 'depends on' is unnecessary.
Okay, I'll drop it.
>
>
> > +menu "Memory initialization"
> > +
> > +choice
> > + prompt "Initialize kernel stack variables at function entry"
> > + depends on CC_HAS_AUTO_VAR_INIT || GCC_PLUGINS
>
> Nit:
> CC_HAS_AUTO_VAR_INIT does not exist at this point.
> I will be added by 3/3.
Oops, yes, I split this chunk in the wrong place. I will fix it.
Thanks!
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().
From: Casey Schaufler @ 2019-04-11 16:45 UTC (permalink / raw)
To: Tetsuo Handa, linux-security-module
Cc: David Miller, netdev, syzbot+0049bebbf3042dbd2e8f,
syzbot+915c9f99f3dbc4bd6cd1, casey
In-Reply-To: <4d077355-03de-0b8a-b6fc-51757eafe7eb@i-love.sakura.ne.jp>
On 4/11/2019 4:31 AM, Tetsuo Handa wrote:
> On 2019/04/04 13:49, David Miller wrote:
>> From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
>> Date: Wed, 3 Apr 2019 06:07:40 +0900
>>
>>> On 2019/04/03 5:23, David Miller wrote:
>>>> Please fix RDS and other protocols to examine the length properly
>>>> instead.
>>> Do you prefer adding branches only for allow reading the family of socket address?
>> If the length is zero, there is no point in reading the family.
>>
> (Adding LSM people.)
>
> syzbot is reporting that RDS is not checking valid length of address given from userspace.
> It turned out that there are several users who access "struct sockaddr"->family without
> checking valid length (which will be reported by KMSAN).
>
> Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as a valid input,
> we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL at
> move_addr_to_kernel() which are called from bind()/connect() system calls. I proposed
> always setting "struct sockaddr"->family at move_addr_to_kernel() but David does not
> like such trick.
>
> Therefore, LSM modules which checks address and/or port have to check valid length
> before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 or UNIX.
>
> Below is all-in-one change (for x86_64 allmodconfig). Well, I've just realized that
> move_addr_to_kernel() is also called by sendmsg() system call and for now added changes
> for only security/ directory. Is this change appropriate for you? (I wish we could
> simplify this by automatically initializing "struct sockaddr_storage" with 0 at
> move_addr_to_kernel()...)
> ---
> drivers/isdn/mISDN/socket.c | 4 ++--
> net/bluetooth/sco.c | 4 ++--
> net/core/filter.c | 2 ++
> net/ipv6/udp.c | 2 ++
> net/llc/af_llc.c | 3 +--
> net/netlink/af_netlink.c | 3 ++-
> net/rds/af_rds.c | 3 +++
> net/rds/bind.c | 2 ++
> net/rxrpc/af_rxrpc.c | 3 ++-
> net/sctp/socket.c | 3 ++-
> security/apparmor/lsm.c | 6 ++++++
> security/selinux/hooks.c | 12 ++++++++++++
> security/smack/smack_lsm.c | 39 +++++++++++++++++++++++++++++++++++----
> security/tomoyo/network.c | 12 ++++++++++++
> 14 files changed, 85 insertions(+), 13 deletions(-)
Except as noted below this looks fine for Smack.
> ...
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c16135..7c19c04 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2805,13 +2805,21 @@ static int smack_socket_socketpair(struct socket *socka,
> *
> * Records the label bound to a port.
> *
> - * Returns 0
> + * Returns 0 on success, and error code otherwise
> */
> static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
> int addrlen)
> {
> - if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
> + if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (addr_len < SIN6_LEN_RFC2133 ||
+ if (addrlen < SIN6_LEN_RFC2133 ||
> + address->sa_family != AF_INET6)
> + return -EINVAL;
> smk_ipv6_port_label(sock, address);
> + }
> return 0;
> }
> #endif /* SMACK_IPV6_PORT_LABELING */
> @@ -2847,12 +2855,21 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>
> switch (sock->sk->sk_family) {
> case PF_INET:
> - if (addrlen < sizeof(struct sockaddr_in))
> + /*
> + * Reject if valid length is too short for IPv4 address or
> + * address family is not IPv4.
> + */
> + if (addrlen < sizeof(struct sockaddr_in) ||
> + sap->sa_family != AF_INET)
> return -EINVAL;
> rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
> break;
> case PF_INET6:
> - if (addrlen < sizeof(struct sockaddr_in6))
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
> return -EINVAL;
> #ifdef SMACK_IPV6_SECMARK_LABELING
> rsp = smack_ipv6host_label(sip);
> @@ -3682,9 +3699,23 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>
> switch (sock->sk->sk_family) {
> case AF_INET:
> + /*
> + * Reject if valid length is too short for IPv4 address or
> + * address family is not IPv4.
> + */
> + if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> + sip->sin_family != AF_INET)
> + return -EINVAL;
> rc = smack_netlabel_send(sock->sk, sip);
> break;
> case AF_INET6:
> + /*
> + * Reject if valid length is too short for IPv6 address or
> + * address family is not IPv6.
> + */
> + if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
> + sap->sin6_family != AF_INET6)
> + return -EINVAL;
> #ifdef SMACK_IPV6_SECMARK_LABELING
> rsp = smack_ipv6host_label(sap);
> if (rsp != NULL)
> diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
> index 9094f4b..3cbd6bd 100644
> --- a/security/tomoyo/network.c
> +++ b/security/tomoyo/network.c
> @@ -505,6 +505,12 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
> {
> struct tomoyo_inet_addr_info *i = &address->inet;
>
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addr_len < offsetofend(struct sockaddr, sa_family))
> + return 0;
> switch (addr->sa_family) {
> case AF_INET6:
> if (addr_len < SIN6_LEN_RFC2133)
> @@ -594,6 +600,12 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
> {
> struct tomoyo_unix_addr_info *u = &address->unix0;
>
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addr_len < offsetofend(struct sockaddr, sa_family))
> + return 0;
> if (addr->sa_family != AF_UNIX)
> return 0;
> u->addr = ((struct sockaddr_un *) addr)->sun_path;
^ permalink raw reply
* Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().
From: Tetsuo Handa @ 2019-04-11 11:31 UTC (permalink / raw)
To: linux-security-module
Cc: David Miller, netdev, syzbot+0049bebbf3042dbd2e8f,
syzbot+915c9f99f3dbc4bd6cd1
In-Reply-To: <20190403.214926.2003732231073495468.davem@davemloft.net>
On 2019/04/04 13:49, David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Date: Wed, 3 Apr 2019 06:07:40 +0900
>
>> On 2019/04/03 5:23, David Miller wrote:
>>> Please fix RDS and other protocols to examine the length properly
>>> instead.
>>
>> Do you prefer adding branches only for allow reading the family of socket address?
>
> If the length is zero, there is no point in reading the family.
>
(Adding LSM people.)
syzbot is reporting that RDS is not checking valid length of address given from userspace.
It turned out that there are several users who access "struct sockaddr"->family without
checking valid length (which will be reported by KMSAN).
Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as a valid input,
we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL at
move_addr_to_kernel() which are called from bind()/connect() system calls. I proposed
always setting "struct sockaddr"->family at move_addr_to_kernel() but David does not
like such trick.
Therefore, LSM modules which checks address and/or port have to check valid length
before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 or UNIX.
Below is all-in-one change (for x86_64 allmodconfig). Well, I've just realized that
move_addr_to_kernel() is also called by sendmsg() system call and for now added changes
for only security/ directory. Is this change appropriate for you? (I wish we could
simplify this by automatically initializing "struct sockaddr_storage" with 0 at
move_addr_to_kernel()...)
---
drivers/isdn/mISDN/socket.c | 4 ++--
net/bluetooth/sco.c | 4 ++--
net/core/filter.c | 2 ++
net/ipv6/udp.c | 2 ++
net/llc/af_llc.c | 3 +--
net/netlink/af_netlink.c | 3 ++-
net/rds/af_rds.c | 3 +++
net/rds/bind.c | 2 ++
net/rxrpc/af_rxrpc.c | 3 ++-
net/sctp/socket.c | 3 ++-
security/apparmor/lsm.c | 6 ++++++
security/selinux/hooks.c | 12 ++++++++++++
security/smack/smack_lsm.c | 39 +++++++++++++++++++++++++++++++++++----
security/tomoyo/network.c | 12 ++++++++++++
14 files changed, 85 insertions(+), 13 deletions(-)
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
index 4ab8b1b..a14e35d 100644
--- a/drivers/isdn/mISDN/socket.c
+++ b/drivers/isdn/mISDN/socket.c
@@ -710,10 +710,10 @@ static int data_sock_getsockopt(struct socket *sock, int level, int optname,
struct sock *sk = sock->sk;
int err = 0;
- if (!maddr || maddr->family != AF_ISDN)
+ if (addr_len < sizeof(struct sockaddr_mISDN))
return -EINVAL;
- if (addr_len < sizeof(struct sockaddr_mISDN))
+ if (!maddr || maddr->family != AF_ISDN)
return -EINVAL;
lock_sock(sk);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 9a58099..d892b7c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -523,12 +523,12 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr,
struct sock *sk = sock->sk;
int err = 0;
- BT_DBG("sk %p %pMR", sk, &sa->sco_bdaddr);
-
if (!addr || addr_len < sizeof(struct sockaddr_sco) ||
addr->sa_family != AF_BLUETOOTH)
return -EINVAL;
+ BT_DBG("sk %p %pMR", sk, &sa->sco_bdaddr);
+
lock_sock(sk);
if (sk->sk_state != BT_OPEN) {
diff --git a/net/core/filter.c b/net/core/filter.c
index 41f633c..b9089fd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4458,6 +4458,8 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
* Only binding to IP is supported.
*/
err = -EINVAL;
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return err;
if (addr->sa_family == AF_INET) {
if (addr_len < sizeof(struct sockaddr_in))
return err;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d538faf..2464fba 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1045,6 +1045,8 @@ static void udp_v6_flush_pending_frames(struct sock *sk)
static int udpv6_pre_connect(struct sock *sk, struct sockaddr *uaddr,
int addr_len)
{
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return -EINVAL;
/* The following checks are replicated from __ip6_datagram_connect()
* and intended to prevent BPF program called below from accessing
* bytes that are out of the bound specified by user in addr_len.
diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index b99e73a..2017b7d 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -320,14 +320,13 @@ static int llc_ui_bind(struct socket *sock, struct sockaddr *uaddr, int addrlen)
struct llc_sap *sap;
int rc = -EINVAL;
- dprintk("%s: binding %02X\n", __func__, addr->sllc_sap);
-
lock_sock(sk);
if (unlikely(!sock_flag(sk, SOCK_ZAPPED) || addrlen != sizeof(*addr)))
goto out;
rc = -EAFNOSUPPORT;
if (unlikely(addr->sllc_family != AF_LLC))
goto out;
+ dprintk("%s: binding %02X\n", __func__, addr->sllc_sap);
rc = -ENODEV;
rcu_read_lock();
if (sk->sk_bound_dev_if) {
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f28e937..216ab91 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -988,7 +988,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
struct netlink_sock *nlk = nlk_sk(sk);
struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
int err = 0;
- unsigned long groups = nladdr->nl_groups;
+ unsigned long groups;
bool bound;
if (addr_len < sizeof(struct sockaddr_nl))
@@ -996,6 +996,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
if (nladdr->nl_family != AF_NETLINK)
return -EINVAL;
+ groups = nladdr->nl_groups;
/* Only superuser is allowed to listen multicasts */
if (groups) {
diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d6cc97f..2b969f9 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -543,6 +543,9 @@ static int rds_connect(struct socket *sock, struct sockaddr *uaddr,
struct rds_sock *rs = rds_sk_to_rs(sk);
int ret = 0;
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return -EINVAL;
+
lock_sock(sk);
switch (uaddr->sa_family) {
diff --git a/net/rds/bind.c b/net/rds/bind.c
index 17c9d9f..0f4398e 100644
--- a/net/rds/bind.c
+++ b/net/rds/bind.c
@@ -173,6 +173,8 @@ int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
/* We allow an RDS socket to be bound to either IPv4 or IPv6
* address.
*/
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return -EINVAL;
if (uaddr->sa_family == AF_INET) {
struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 96f2952..c54dce3 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -135,7 +135,7 @@ static int rxrpc_bind(struct socket *sock, struct sockaddr *saddr, int len)
struct sockaddr_rxrpc *srx = (struct sockaddr_rxrpc *)saddr;
struct rxrpc_local *local;
struct rxrpc_sock *rx = rxrpc_sk(sock->sk);
- u16 service_id = srx->srx_service;
+ u16 service_id;
int ret;
_enter("%p,%p,%d", rx, saddr, len);
@@ -143,6 +143,7 @@ static int rxrpc_bind(struct socket *sock, struct sockaddr *saddr, int len)
ret = rxrpc_validate_address(rx, srx, len);
if (ret < 0)
goto error;
+ service_id = srx->srx_service;
lock_sock(&rx->sk);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9874e60..4583fa9 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4847,7 +4847,8 @@ static int sctp_connect(struct sock *sk, struct sockaddr *addr,
}
/* Validate addr_len before calling common connect/connectx routine. */
- af = sctp_get_af_specific(addr->sa_family);
+ af = addr_len < offsetofend(struct sockaddr, sa_family) ? NULL :
+ sctp_get_af_specific(addr->sa_family);
if (!af || addr_len < af->sockaddr_len) {
err = -EINVAL;
} else {
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index e338359..e8b2163 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -866,6 +866,7 @@ static int apparmor_socket_bind(struct socket *sock,
AA_BUG(!address);
AA_BUG(in_interrupt());
+ /* No need to check addrlen because bind_perm() is not evaluated. */
return af_select(sock->sk->sk_family,
bind_perm(sock, address, addrlen),
aa_sk_perm(OP_BIND, AA_MAY_BIND, sock->sk));
@@ -882,6 +883,7 @@ static int apparmor_socket_connect(struct socket *sock,
AA_BUG(!address);
AA_BUG(in_interrupt());
+ /* No need to check addrlen because connect_perm() is not evaluated. */
return af_select(sock->sk->sk_family,
connect_perm(sock, address, addrlen),
aa_sk_perm(OP_CONNECT, AA_MAY_CONNECT, sock->sk));
@@ -927,6 +929,10 @@ static int aa_sock_msg_perm(const char *op, u32 request, struct socket *sock,
AA_BUG(!msg);
AA_BUG(in_interrupt());
+ /*
+ * No need to check msg->msg_namelen because msg_perm() is not
+ * evaluated.
+ */
return af_select(sock->sk->sk_family,
msg_perm(op, request, sock, msg, size),
aa_sk_perm(op, request, sock->sk));
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d5fdcb0..710d386 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4503,6 +4503,12 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
err = sock_has_perm(sk, SOCKET__BIND);
if (err)
goto out;
+ /*
+ * Nothing more to do if valid length is too short to check
+ * address->sa_family.
+ */
+ if (addrlen < offsetofend(struct sockaddr, sa_family))
+ goto out;
/* If PF_INET or PF_INET6, check name_bind permission for the port. */
family = sk->sk_family;
@@ -4634,6 +4640,12 @@ static int selinux_socket_connect_helper(struct socket *sock,
err = sock_has_perm(sk, SOCKET__CONNECT);
if (err)
return err;
+ /*
+ * Nothing more to do if valid length is too short to check
+ * address->sa_family.
+ */
+ if (addrlen < offsetofend(struct sockaddr, sa_family))
+ return 0;
/*
* If a TCP, DCCP or SCTP socket, check name_connect permission
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 5c16135..7c19c04 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2805,13 +2805,21 @@ static int smack_socket_socketpair(struct socket *socka,
*
* Records the label bound to a port.
*
- * Returns 0
+ * Returns 0 on success, and error code otherwise
*/
static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
int addrlen)
{
- if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
+ if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
+ /*
+ * Reject if valid length is too short for IPv6 address or
+ * address family is not IPv6.
+ */
+ if (addr_len < SIN6_LEN_RFC2133 ||
+ address->sa_family != AF_INET6)
+ return -EINVAL;
smk_ipv6_port_label(sock, address);
+ }
return 0;
}
#endif /* SMACK_IPV6_PORT_LABELING */
@@ -2847,12 +2855,21 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
switch (sock->sk->sk_family) {
case PF_INET:
- if (addrlen < sizeof(struct sockaddr_in))
+ /*
+ * Reject if valid length is too short for IPv4 address or
+ * address family is not IPv4.
+ */
+ if (addrlen < sizeof(struct sockaddr_in) ||
+ sap->sa_family != AF_INET)
return -EINVAL;
rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
break;
case PF_INET6:
- if (addrlen < sizeof(struct sockaddr_in6))
+ /*
+ * Reject if valid length is too short for IPv6 address or
+ * address family is not IPv6.
+ */
+ if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sip);
@@ -3682,9 +3699,23 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
switch (sock->sk->sk_family) {
case AF_INET:
+ /*
+ * Reject if valid length is too short for IPv4 address or
+ * address family is not IPv4.
+ */
+ if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
+ sip->sin_family != AF_INET)
+ return -EINVAL;
rc = smack_netlabel_send(sock->sk, sip);
break;
case AF_INET6:
+ /*
+ * Reject if valid length is too short for IPv6 address or
+ * address family is not IPv6.
+ */
+ if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
+ sap->sin6_family != AF_INET6)
+ return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sap);
if (rsp != NULL)
diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
index 9094f4b..3cbd6bd 100644
--- a/security/tomoyo/network.c
+++ b/security/tomoyo/network.c
@@ -505,6 +505,12 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
{
struct tomoyo_inet_addr_info *i = &address->inet;
+ /*
+ * Nothing more to do if valid length is too short to check
+ * address->sa_family.
+ */
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return 0;
switch (addr->sa_family) {
case AF_INET6:
if (addr_len < SIN6_LEN_RFC2133)
@@ -594,6 +600,12 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
{
struct tomoyo_unix_addr_info *u = &address->unix0;
+ /*
+ * Nothing more to do if valid length is too short to check
+ * address->sa_family.
+ */
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return 0;
if (addr->sa_family != AF_UNIX)
return 0;
u->addr = ((struct sockaddr_un *) addr)->sun_path;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 1/3] Kconfig: Create "kernel hardening" config area
From: Masahiro Yamada @ 2019-04-11 8:50 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Alexander Popov, Michal Marek, Emese Revfy, James Morris,
Serge E. Hallyn, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190410161612.18545-2-keescook@chromium.org>
On Thu, Apr 11, 2019 at 1:16 AM Kees Cook <keescook@chromium.org> wrote:
> diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
> index 74271dba4f94..01874ef0f883 100644
> --- a/scripts/gcc-plugins/Kconfig
> +++ b/scripts/gcc-plugins/Kconfig
> @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS
> An arch should select this symbol if it supports building with
> GCC plugins.
>
> -menuconfig GCC_PLUGINS
> - bool "GCC plugins"
> +config GCC_PLUGINS
> + bool
This will flatten the plugin config options.
If you want to keep the current menu structure, you can do:
menu "GCC plugins"
...
endmenu
Another side-effect is Kbuild will descend into scripts/gcc-plugins/
even when no plugin is selected.
It is not a big build speed regression, though.
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> new file mode 100644
> index 000000000000..8223a8ab1a12
> --- /dev/null
> +++ b/security/Kconfig.hardening
> @@ -0,0 +1,94 @@
> +menu "Kernel hardening options"
> +
> +config GCC_PLUGIN_STRUCTLEAK
> + bool
> + depends on GCC_PLUGIN_STRUCTLEAK_USER || GCC_PLUGIN_STRUCTLEAK_BYREF || GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
I think this 'depends on' is unnecessary.
> +menu "Memory initialization"
> +
> +choice
> + prompt "Initialize kernel stack variables at function entry"
> + depends on CC_HAS_AUTO_VAR_INIT || GCC_PLUGINS
Nit:
CC_HAS_AUTO_VAR_INIT does not exist at this point.
I will be added by 3/3.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v4 2/3] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-04-11 8:39 UTC (permalink / raw)
To: Kees Cook
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAGXu5jJt3uNWVXDD=GR6oVwn=g7O=Jcz9sxai5q10X9yyR56XA@mail.gmail.com>
On Wed, Apr 10, 2019 at 6:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 6:18 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > This config option adds the possibility to initialize newly allocated
> > pages and heap objects with a 0xAA pattern.
> > There's already a number of places where allocations are initialized
> > based on the presence of __GFP_ZERO flag. We just change this code so
> > that under CONFIG_INIT_ALL_HEAP these allocations are always initialized
> > with either 0x00 or 0xAA depending on the __GFP_ZERO.
>
> Why not just make __GFP_ZERO unconditional instead? This looks like
> it'd be simpler and not need arch-specific implementation?
Right, but it would mean we can only initialize with 0x00 pattern.
I believe that for testing purposes a nonzero pattern is better,
because it'll not only assure the execution is deterministic, but will
also uncover logic bugs earlier (see the discussion at
https://reviews.llvm.org/D54604?id=174471)
For hardening purposes the pattern shouldn't matter much.
If you think arch-specific code is too much of a trouble, we could
implement clear_page_pattern() using memset() on every architecture,
but allow the user to choose between slow (0xAA) and production (0x00)
modes.
> --
> Kees Cook
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 3/3] kbuild: Implement Clang's stack initialization
From: Masahiro Yamada @ 2019-04-11 8:05 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Alexander Popov, Michal Marek, Emese Revfy, James Morris,
Serge E. Hallyn, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190410161612.18545-4-keescook@chromium.org>
On Thu, Apr 11, 2019 at 1:16 AM Kees Cook <keescook@chromium.org> wrote:
>
> CONFIG_INIT_STACK_ALL turns on stack initialization based on
> -ftrivial-auto-var-init in Clang builds and on
> -fplugin-arg-structleak_plugin-byref-all in GCC builds.
Is CONFIG_INIT_STACK_ALL wired up to GCC plugin in any way?
I could not understand it from the code.
>
> -ftrivial-auto-var-init is a Clang flag that provides trivial
> initializers for uninitialized local variables, variable fields and
> padding.
>
> It has three possible values:
> pattern - uninitialized locals are filled with a fixed pattern
> (mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
> for more details) likely to cause crashes when uninitialized value is
> used;
> zero (it's still debated whether this flag makes it to the official
> Clang release) - uninitialized locals are filled with zeroes;
> uninitialized (default) - uninitialized locals are left intact.
>
> The proposed config builds the kernel with
> -ftrivial-auto-var-init=pattern when selected.
>
> Developers have the possibility to opt-out of this feature on a
> per-variable basis by using __attribute__((uninitialized)).
>
> Co-developed-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Makefile | 5 +++++
> security/Kconfig.hardening | 14 ++++++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index c0a34064c574..a7d9c6cd0267 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -745,6 +745,11 @@ KBUILD_CFLAGS += -fomit-frame-pointer
> endif
> endif
>
> +# Initialize all stack variables with a pattern, if desired.
> +ifdef CONFIG_INIT_STACK_ALL
> +KBUILD_CFLAGS += -ftrivial-auto-var-init=pattern
> +endif
> +
> DEBUG_CFLAGS := $(call cc-option, -fno-var-tracking-assignments)
>
> ifdef CONFIG_DEBUG_INFO
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index 9942d9869864..d744e20140b4 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -19,9 +19,13 @@ config GCC_PLUGIN_STRUCTLEAK
>
> menu "Memory initialization"
>
> +config CC_HAS_AUTO_VAR_INIT
> + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> +
> choice
> prompt "Initialize kernel stack variables at function entry"
> depends on CC_HAS_AUTO_VAR_INIT || GCC_PLUGINS
> + default INIT_STACK_ALL if CC_HAS_AUTO_VAR_INIT
Why should this be enabled by default?
Ins't it a performance regression
since it inserts instructions in function prologue?
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 0/3] Kconfig: Refactor memory initialization hardening
From: Masahiro Yamada @ 2019-04-11 7:59 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Alexander Popov, Michal Marek, Emese Revfy, James Morris,
Serge E. Hallyn, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190410161612.18545-1-keescook@chromium.org>
On Thu, Apr 11, 2019 at 1:16 AM Kees Cook <keescook@chromium.org> wrote:
>
> This is a proposed alternative for the memory initialization series,
> which refactoring the existing gcc plugins into a separate Kconfig
> file and collects all the related options together with some more
> language to describe their differences. The last patch adds the
> Clang auto init option, as done by Alexander Potapenko.
>
> Since there isn't really a good way to "select" with dependencies,
> I've left out CONFIG_INIT_ALL_MEMORY for the moment...
>
> -Kees
>
> Kees Cook (3):
> Kconfig: Create "kernel hardening" config area
I want to see "kconfig:" prefix in the subject line
only for changed in scripts/kconfig/.
> kbuild: Move stackleak config to Kconfig.hardening
This is not a change in the build system.
> kbuild: Implement Clang's stack initialization
I think "gcc-plugin:", "security:' or something is better for the
patch subjects, and this patch series is out of
my maintenance area.
> Makefile | 5 ++
> scripts/gcc-plugins/Kconfig | 121 +-------------------------
> security/Kconfig | 2 +
> security/Kconfig.hardening | 165 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 175 insertions(+), 118 deletions(-)
> create mode 100644 security/Kconfig.hardening
>
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Rik van Riel @ 2019-04-11 1:37 UTC (permalink / raw)
To: Eric Biggers, Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott
In-Reply-To: <20190410231156.GB120258@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 563 bytes --]
On Wed, 2019-04-10 at 16:11 -0700, Eric Biggers wrote:
> You've explained *what* it does again, but not *why*. *Why* do you
> want
> hardened usercopy to detect copies across page boundaries, when there
> is no
> actual buffer overflow?
When some subsystem in the kernel allocates multiple
pages without _GFP_COMP, there is no way afterwards
to detect exactly how many pages it allocated.
In other words, there is no way to see how large the
buffer is, nor whether the copy operation in question
would overflow it.
--
All Rights Reversed.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-04-10 23:27 UTC (permalink / raw)
To: Eric Biggers
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <20190410231156.GB120258@gmail.com>
On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
> You've explained *what* it does again, but not *why*. *Why* do you want
> hardened usercopy to detect copies across page boundaries, when there is no
> actual buffer overflow?
But that *is* how it determines it was a buffer overflow: "if you
cross page boundaries (of a non-compound allocation), it *is* a buffer
overflow". This assertion, however, is flawed because many contiguous
allocations are not marked as being grouped together when it reality
they were. It was an attempt to get allocation size information out of
the page allocator, similar to how slab can be queries about
allocation size. I'm open to improvements here, since it's obviously
broken in its current state. :)
--
Kees Cook
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-04-10 23:11 UTC (permalink / raw)
To: Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <CAGXu5j+Gfvj9G6s_UQTw6hE11gJb1edt3BXzpbDQdc_dpcHgag@mail.gmail.com>
On Wed, Apr 10, 2019 at 02:57:46PM -0700, Kees Cook wrote:
> On Wed, Apr 10, 2019 at 12:07 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > That didn't answer my question. My question is what is the purpose of this? If
> > there was actual buffer overflow when __GFP_COMP isn't specified that would make
> > perfect sense, but AFAICS there isn't. So why does hardened usercopy consider
> > it broken when __GFP_COMP isn't specified?
>
> The goal of CONFIG_HARDENED_USERCOPY_PAGESPAN was to detect copies
> across page boundaries in memory allocated by the page allocator.
> There appear to be enough cases of allocations that span pages but do
> not mark them with __GFP_COMP, so this logic hasn't proven useful in
> the real world (which is why no one should use the ..._PAGESPAN config
> in production). I'd like to get the kernel to the point where hardened
> usercopy can correctly do these checks (right now it's mainly only
> useful at checking for overflows in slub and slab), but it'll take
> time/focus for a while. No one has had time yet to track all of these
> down and fix them. (I defer to Laura and Rik on the design of the
> pagespan checks; they did the bulk of the work there.)
>
> Does that help explain it, or am I still missing your question?
>
> --
> Kees Cook
You've explained *what* it does again, but not *why*. *Why* do you want
hardened usercopy to detect copies across page boundaries, when there is no
actual buffer overflow?
- Eric
^ permalink raw reply
* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
From: James Morris @ 2019-04-10 22:00 UTC (permalink / raw)
To: Al Viro
Cc: Kangjie Lu, pakki001, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20190410180156.GZ2217@ZenIV.linux.org.uk>
On Wed, 10 Apr 2019, Al Viro wrote:
> Rather bad way to do it - generally, register_filesystem() should be
> the last thing done by initialization. Any modular code that
> does unregister_filesystem() on failure exit is flat-out broken;
> here it's not instantly FUBAR, but it's a bloody bad example.
>
> What's more, why not let simple_fill_super() do it? Just
> static int fill_super(struct super_block *sb, void *data, int silent)
> {
> static const struct tree_descr files[] = {
> #ifdef CONFIG_SECURITY
> {"lsm", &lsm_ops, 0444},
> #endif
> {""}
> };
>
> and to hell with that call of securityfs_create_file() and all its
> failure handling...
Thanks for the review. Reverted.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-04-10 21:57 UTC (permalink / raw)
To: Eric Biggers
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <20190410190729.GA120258@gmail.com>
On Wed, Apr 10, 2019 at 12:07 PM Eric Biggers <ebiggers@kernel.org> wrote:
> That didn't answer my question. My question is what is the purpose of this? If
> there was actual buffer overflow when __GFP_COMP isn't specified that would make
> perfect sense, but AFAICS there isn't. So why does hardened usercopy consider
> it broken when __GFP_COMP isn't specified?
The goal of CONFIG_HARDENED_USERCOPY_PAGESPAN was to detect copies
across page boundaries in memory allocated by the page allocator.
There appear to be enough cases of allocations that span pages but do
not mark them with __GFP_COMP, so this logic hasn't proven useful in
the real world (which is why no one should use the ..._PAGESPAN config
in production). I'd like to get the kernel to the point where hardened
usercopy can correctly do these checks (right now it's mainly only
useful at checking for overflows in slub and slab), but it'll take
time/focus for a while. No one has had time yet to track all of these
down and fix them. (I defer to Laura and Rik on the design of the
pagespan checks; they did the bulk of the work there.)
Does that help explain it, or am I still missing your question?
--
Kees Cook
^ permalink raw reply
* Re: KASAN: use-after-free Read in path_lookupat
From: Linus Torvalds @ 2019-04-10 19:44 UTC (permalink / raw)
To: Al Viro
Cc: syzbot, Alexei Starovoitov, Daniel Borkmann, linux-fsdevel,
Linux List Kernel Mailing, syzkaller-bugs, James Morris,
linux-security-module
In-Reply-To: <20190410181135.GA32071@ZenIV.linux.org.uk>
On Wed, Apr 10, 2019 at 8:11 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Both are in vfs.git#fixes. Which way should that go - directly or
> via linux-security.git?
Just do it directly. I doubt you can trigger them for securityfs and
apparmourfs, since normal users have no way to remove any files from
them, so the race with final unlink sounds fairly irrelevant in
practice, no?
Linus
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-04-10 19:07 UTC (permalink / raw)
To: Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List
In-Reply-To: <CAGXu5j+SZ2+Zkc=Vp5CXQUHhiR9f_OOnca684JTRW3T0yXdaNQ@mail.gmail.com>
On Wed, Apr 10, 2019 at 11:30:57AM -0700, Kees Cook wrote:
> On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote:
> > > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote:
> > > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> > > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > > > > > > When running the sha1-asm crypto selftest on arm with
> > > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> > > > > > >
> > > > > > > usercopy: Kernel memory overwrite attempt detected to spans
> > > > > > > multiple pages (offset 0, size 42)!
> > > > > >
> > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation
> > > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't
> > > > > > understand why hardened usercopy considers it a bug though, as there's no buffer
> > > > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page
> > > > > > buffer that was allocated with __get_free_pages():
> > > > > >
> > > > > > __get_free_pages(GFP_KERNEL, 1)
> > > > > >
> > > > > > ... where 1 means an order-1 allocation.
> > > > > >
> > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy
> > > > > > considers it a bug even though the buffer is 8192 bytes long. Why?
> > > > > >
> > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> > > > > > with ITER_KVEC.
> > > > > >
> > > > > > - Eric
> > > > >
> > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page
> > > > > boundary when they seem to be fine?
> > > >
> > > > This is due to missing the compound page marking, if I remember
> > > > correctly. However, I tend to leave the pagespan test disabled: it
> > > > really isn't ready for production use -- there are a lot of missing
> > > > annotations still.
> > > >
> > >
> > > So do I need to add __GFP_COMP? Is there any actual reason to do so?
> > > Why does hardened usercopy check for it?
> > >
> > > - Eric
> >
> > Hi Kees, any answer to this question?
>
> Hi! Sorry, this got lost in my inbox. Yes, if you can add __GFP_COMP,
> that would fix this case. No one has had time lately to track down all
> these cases, but avoiding adding new ones would be wonderful. :)
>
> It's in there because it's a state I'd like to get to in the kernel,
> but it'll require a lot more work to get there.
>
That didn't answer my question. My question is what is the purpose of this? If
there was actual buffer overflow when __GFP_COMP isn't specified that would make
perfect sense, but AFAICS there isn't. So why does hardened usercopy consider
it broken when __GFP_COMP isn't specified?
- Eric
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-04-10 18:30 UTC (permalink / raw)
To: Eric Biggers
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List
In-Reply-To: <20190410031734.GB7140@sol.localdomain>
On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote:
> > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote:
> > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > > > > > When running the sha1-asm crypto selftest on arm with
> > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> > > > > >
> > > > > > usercopy: Kernel memory overwrite attempt detected to spans
> > > > > > multiple pages (offset 0, size 42)!
> > > > >
> > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation
> > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't
> > > > > understand why hardened usercopy considers it a bug though, as there's no buffer
> > > > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page
> > > > > buffer that was allocated with __get_free_pages():
> > > > >
> > > > > __get_free_pages(GFP_KERNEL, 1)
> > > > >
> > > > > ... where 1 means an order-1 allocation.
> > > > >
> > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy
> > > > > considers it a bug even though the buffer is 8192 bytes long. Why?
> > > > >
> > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> > > > > with ITER_KVEC.
> > > > >
> > > > > - Eric
> > > >
> > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page
> > > > boundary when they seem to be fine?
> > >
> > > This is due to missing the compound page marking, if I remember
> > > correctly. However, I tend to leave the pagespan test disabled: it
> > > really isn't ready for production use -- there are a lot of missing
> > > annotations still.
> > >
> >
> > So do I need to add __GFP_COMP? Is there any actual reason to do so?
> > Why does hardened usercopy check for it?
> >
> > - Eric
>
> Hi Kees, any answer to this question?
Hi! Sorry, this got lost in my inbox. Yes, if you can add __GFP_COMP,
that would fix this case. No one has had time lately to track down all
these cases, but avoiding adding new ones would be wonderful. :)
It's in there because it's a state I'd like to get to in the kernel,
but it'll require a lot more work to get there.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
From: Kees Cook @ 2019-04-10 18:20 UTC (permalink / raw)
To: Jann Horn
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAG48ez0a6iyw_TCF2aY4ADbiveaAy+tZeiKjT0j7+36qC6W_og@mail.gmail.com>
On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Apr 10, 2019 at 7:24 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > > From: Jann Horn <jannh@google.com>
> > >
> > > The current API of the SafeSetID LSM uses one write() per rule, and applies
> > > each written rule instantly. This has several downsides:
> > >
> > > - While a policy is being loaded, once a single parent-child pair has been
> > > loaded, the parent is restricted to that specific child, even if
> > > subsequent rules would allow transitions to other child UIDs. This means
> > > that during policy loading, set*uid() can randomly fail.
> > > - To replace the policy without rebooting, it is necessary to first flush
> > > all old rules. This creates a time window in which no constraints are
> > > placed on the use of CAP_SETUID.
> > > - If we want to perform sanity checks on the final policy, this requires
> > > that the policy isn't constructed in a piecemeal fashion without telling
> > > the kernel when it's done.
> > >
> > > Other kernel APIs - including things like the userns code and netfilter -
> > > avoid this problem by performing updates atomically. Luckily, SafeSetID
> > > hasn't landed in a stable (upstream) release yet, so maybe it's not too
> > > late to completely change the API.
> > >
> > > The new API for SafeSetID is: If you want to change the policy, open
> > > "safesetid/whitelist_policy" and write the entire policy,
> > > newline-delimited, in there.
> >
> > So the entire policy is expected to be sent in a single write() call?
> >
> > open()
> > write(policy1)
> > write(policy2)
> > close()
> >
> > means only policy2 is active?
>
> No; if you do that, the first write() sets policy1, and the second
> write() fails with -EINVAL because of the "if (*ppos != 0) return
> -EINVAL;" in safesetid_file_write() (which already exists in the
> current version of the LSM).
Ah yes, thanks! I missed that check. Good!
>
> > I thought policy was meant to be built
> > over time? i.e. new policy could get appended to existing?
>
> That's what the current API does; as I've explained in the commit
> message, I think that that's a bad idea.
Okay, sounds fine. It wasn't clear to me from the commit message if
you meant "write the whole policy during a single open/close" or
"write whole policy with a single initial write".
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Kees Cook @ 2019-04-10 18:17 UTC (permalink / raw)
To: Jann Horn
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAG48ez2gub+rSD3H+XZkd3Wp8Sh6KTzcEStZaQ=g1sG0=Tid+A@mail.gmail.com>
On Wed, Apr 10, 2019 at 10:37 AM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > > From: Jann Horn <jannh@google.com>
> > >
> > > Someone might write a ruleset like the following, expecting that it
> > > securely constrains UID 1 to UIDs 1, 2 and 3:
> > >
> > > 1:2
> > > 1:3
> > >
> > > However, because no constraints are applied to UIDs 2 and 3, an attacker
> > > with UID 1 can simply first switch to UID 2, then switch to any UID from
> > > there. The secure way to write this ruleset would be:
> > >
> > > 1:2
> > > 1:3
> > > 2:2
> > > 3:3
> > >
> > > , which uses "transition to self" as a way to inhibit the default-allow
> > > policy without allowing anything specific.
> > >
> > > This is somewhat unintuitive. To make sure that policy authors don't
> > > accidentally write insecure policies because of this, let the kernel verify
> > > that a new ruleset does not contain any entries that are constrained, but
> > > transitively unconstrained.
> > >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > > ---
> > > security/safesetid/securityfs.c | 21 +++++++++++++++++++
> > > .../selftests/safesetid/safesetid-test.c | 4 +++-
> > > 2 files changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > > index 7a08fff2bc14..3ec64487f0e9 100644
> > > --- a/security/safesetid/securityfs.c
> > > +++ b/security/safesetid/securityfs.c
> > > @@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
> > > call_rcu(&pol->rcu, __release_ruleset);
> > > }
> > >
> > > +static int verify_ruleset(struct setuid_ruleset *pol)
> > > +{
> > > + int bucket;
> > > + struct setuid_rule *rule;
> > > +
> > > + hash_for_each(pol->rules, bucket, rule, next) {
> > > + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> > > + SIDPOL_DEFAULT) {
> > > + pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
> > > + __kuid_val(rule->src_uid),
> > > + __kuid_val(rule->dst_uid));
> > > + return -EINVAL;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> >
> > Why fail open? How about having the policy automatically add the
> > constraints (since the entire policy is known at verification time)?
>
> Are you suggesting to print a warning, automatically add the
> constraint, apply the policy, and still return -EINVAL? I guess that
> would work.
I meant not return EINVAL in this case but just fix it.
> I think it is a good idea to require userspace to explicitly write
> these rules, since implicitly-added rules might make the rules harder
> to understand for someone reading a policy file.
Mostly it's a question for the userspace design. If EINVAL is checked,
then I think it's probably fine, but will that error propagate to
whatever will eventually launch the processes that are supposed to be
confined? Since it's a detectable case, why not just fix it (with a
warning) and run safely?
--
Kees Cook
^ permalink raw reply
* Re: KASAN: use-after-free Read in path_lookupat
From: Al Viro @ 2019-04-10 18:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: syzbot, Alexei Starovoitov, Daniel Borkmann, linux-fsdevel,
Linux List Kernel Mailing, syzkaller-bugs, James Morris,
linux-security-module
In-Reply-To: <20190326014552.GY2217@ZenIV.linux.org.uk>
On Tue, Mar 26, 2019 at 01:45:52AM +0000, Al Viro wrote:
> On Mon, Mar 25, 2019 at 11:37:32PM +0000, Al Viro wrote:
>
> > For debugfs it's clearly "use default ->evict_inode(), have explicit
> > ->destroy_inode() using free_inode_nonrcu()" - there we have nothing
> > else done in ->evict_inode() and kfree is obviously safe in softirq.
> > I'll post that (or push to vfs.git#fixes), along with minimal fixes
> > for other 3. If bpf_any_put() is softirq-safe, we'll have the full
> > set for -stable and the rest could be done on top of that.
> >
> > Won't solve the documetation problem, unfortunately ;-/
>
> Posted; all of those (as well as Daniel's bpf patch) are Cc:stable
> fodder. Documentation is still, er, deficient...
... and unfortunately there are two more, exactly like debugfs -
securityfs and apparmorfs, found while sorting out the series
for separate rcu-delayed counterpart of ->destroy_inode().
Both are in vfs.git#fixes. Which way should that go - directly or
via linux-security.git? Both are stable fodder, in theory, but
much harder to hit than their ubifs/debugfs/bpf counterparts...
^ permalink raw reply
* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
From: Al Viro @ 2019-04-10 18:01 UTC (permalink / raw)
To: James Morris
Cc: Kangjie Lu, pakki001, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <alpine.LRH.2.21.1904110334380.25754@namei.org>
On Thu, Apr 11, 2019 at 03:34:43AM +1000, James Morris wrote:
> On Fri, 15 Mar 2019, Kangjie Lu wrote:
>
> > securityfs_create_file may fail. The fix checks its status and
> > returns the error code upstream if it fails.
> >
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> >
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
>
> > ---
> > Return the exact error code upstream.
> > ---
> > security/inode.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/security/inode.c b/security/inode.c
> > index b7772a9b315e..667f8b15027d 100644
> > --- a/security/inode.c
> > +++ b/security/inode.c
> > @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
> > #ifdef CONFIG_SECURITY
> > lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
> > &lsm_ops);
> > + if (IS_ERR(lsm_dentry)) {
> > + unregister_filesystem(&fs_type);
> > + sysfs_remove_mount_point(kernel_kobj, "security");
> > + return PTR_ERR(lsm_dentry);
> > + }
Rather bad way to do it - generally, register_filesystem() should be
the last thing done by initialization. Any modular code that
does unregister_filesystem() on failure exit is flat-out broken;
here it's not instantly FUBAR, but it's a bloody bad example.
What's more, why not let simple_fill_super() do it? Just
static int fill_super(struct super_block *sb, void *data, int silent)
{
static const struct tree_descr files[] = {
#ifdef CONFIG_SECURITY
{"lsm", &lsm_ops, 0444},
#endif
{""}
};
and to hell with that call of securityfs_create_file() and all its
failure handling...
^ permalink raw reply
* Re: [PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
From: Jann Horn @ 2019-04-10 17:47 UTC (permalink / raw)
To: Kees Cook
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAGXu5jK9tc28tsxEKOQ=R9B2AUOJ5Rt3j=VNqr1PQcAyAeNpgQ@mail.gmail.com>
On Wed, Apr 10, 2019 at 7:24 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > From: Jann Horn <jannh@google.com>
> >
> > The current API of the SafeSetID LSM uses one write() per rule, and applies
> > each written rule instantly. This has several downsides:
> >
> > - While a policy is being loaded, once a single parent-child pair has been
> > loaded, the parent is restricted to that specific child, even if
> > subsequent rules would allow transitions to other child UIDs. This means
> > that during policy loading, set*uid() can randomly fail.
> > - To replace the policy without rebooting, it is necessary to first flush
> > all old rules. This creates a time window in which no constraints are
> > placed on the use of CAP_SETUID.
> > - If we want to perform sanity checks on the final policy, this requires
> > that the policy isn't constructed in a piecemeal fashion without telling
> > the kernel when it's done.
> >
> > Other kernel APIs - including things like the userns code and netfilter -
> > avoid this problem by performing updates atomically. Luckily, SafeSetID
> > hasn't landed in a stable (upstream) release yet, so maybe it's not too
> > late to completely change the API.
> >
> > The new API for SafeSetID is: If you want to change the policy, open
> > "safesetid/whitelist_policy" and write the entire policy,
> > newline-delimited, in there.
>
> So the entire policy is expected to be sent in a single write() call?
>
> open()
> write(policy1)
> write(policy2)
> close()
>
> means only policy2 is active?
No; if you do that, the first write() sets policy1, and the second
write() fails with -EINVAL because of the "if (*ppos != 0) return
-EINVAL;" in safesetid_file_write() (which already exists in the
current version of the LSM).
> I thought policy was meant to be built
> over time? i.e. new policy could get appended to existing?
That's what the current API does; as I've explained in the commit
message, I think that that's a bad idea.
Are you asking because you have a usecase where you actually want to
"append" rules after loading an initial policy?
If so, I think that the simplest way to do that would be to have
userspace concatenate the policies and then shove the result of that
into the kernel. Otherwise:
- you'd need a way to distinguish between policy replacement and
appending to policy
- to securely replace an existing policy, userspace would always have
to concatenate all the new policy fragments anyway
^ permalink raw reply
* Re: [PATCH 08/10] LSM: SafeSetID: add read handler
From: Jann Horn @ 2019-04-10 17:42 UTC (permalink / raw)
To: Kees Cook
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAGXu5jLUFURN7-bQqwdzFLXF5XH1qAS+rb1pCGFZqrqZqMP6AQ@mail.gmail.com>
On Wed, Apr 10, 2019 at 7:27 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > For debugging a running system, it is very helpful to be able to see what
> > policy the system is using. Add a read handler that can dump out a copy of
> > the loaded policy.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> > security/safesetid/lsm.h | 3 +++
> > security/safesetid/securityfs.c | 38 +++++++++++++++++++++++++++++++--
> > 2 files changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index 4a34f558d964..9380329fe30a 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -17,6 +17,7 @@
> > #include <linux/types.h>
> > #include <linux/uidgid.h>
> > #include <linux/hashtable.h>
> > +#include <linux/refcount.h>
> >
> > /* Flag indicating whether initialization completed */
> > extern int safesetid_initialized;
> > @@ -41,7 +42,9 @@ struct setuid_rule {
> >
> > struct setuid_ruleset {
> > DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> > + char *policy_str;
> > struct rcu_head rcu;
> > + refcount_t refcount;
> > };
>
> refcount seems like overkill? Why not just use the spinlock? Neither
> read nor write are "fast path".
You can't copy to userspace under a spinlock or under RCU. But we
could change policy_update_lock to a mutex and hold that across the
policy read.
^ permalink raw reply
* Re: [PATCH] Yama: mark function as static
From: James Morris @ 2019-04-10 17:37 UTC (permalink / raw)
To: Mukesh Ojha; +Cc: keescook, jannh, linux-kernel, serge, linux-security-module
In-Reply-To: <1553673018-19234-1-git-send-email-mojha@codeaurora.org>
On Wed, 27 Mar 2019, Mukesh Ojha wrote:
> Sparse complains yama_task_prctl can be static. Fix it by making
> it static.
>
> Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Jann Horn @ 2019-04-10 17:36 UTC (permalink / raw)
To: Kees Cook
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAGXu5j+HTSPDR0Sp17=Jr3gRUSOX39dohy0bgxwyL+Wbh9swkQ@mail.gmail.com>
On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > From: Jann Horn <jannh@google.com>
> >
> > Someone might write a ruleset like the following, expecting that it
> > securely constrains UID 1 to UIDs 1, 2 and 3:
> >
> > 1:2
> > 1:3
> >
> > However, because no constraints are applied to UIDs 2 and 3, an attacker
> > with UID 1 can simply first switch to UID 2, then switch to any UID from
> > there. The secure way to write this ruleset would be:
> >
> > 1:2
> > 1:3
> > 2:2
> > 3:3
> >
> > , which uses "transition to self" as a way to inhibit the default-allow
> > policy without allowing anything specific.
> >
> > This is somewhat unintuitive. To make sure that policy authors don't
> > accidentally write insecure policies because of this, let the kernel verify
> > that a new ruleset does not contain any entries that are constrained, but
> > transitively unconstrained.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> > security/safesetid/securityfs.c | 21 +++++++++++++++++++
> > .../selftests/safesetid/safesetid-test.c | 4 +++-
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 7a08fff2bc14..3ec64487f0e9 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
> > call_rcu(&pol->rcu, __release_ruleset);
> > }
> >
> > +static int verify_ruleset(struct setuid_ruleset *pol)
> > +{
> > + int bucket;
> > + struct setuid_rule *rule;
> > +
> > + hash_for_each(pol->rules, bucket, rule, next) {
> > + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> > + SIDPOL_DEFAULT) {
> > + pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
> > + __kuid_val(rule->src_uid),
> > + __kuid_val(rule->dst_uid));
> > + return -EINVAL;
> > + }
> > + }
> > + return 0;
> > +}
>
> Why fail open? How about having the policy automatically add the
> constraints (since the entire policy is known at verification time)?
Are you suggesting to print a warning, automatically add the
constraint, apply the policy, and still return -EINVAL? I guess that
would work.
I think it is a good idea to require userspace to explicitly write
these rules, since implicitly-added rules might make the rules harder
to understand for someone reading a policy file.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox