crypto: user - make NETLINK_CRYPTO work inside netns
diff mbox series

Message ID 20190709111124.31127-1-omosnace@redhat.com
State Accepted
Delegated to: Herbert Xu
Headers show
Series
  • crypto: user - make NETLINK_CRYPTO work inside netns
Related show

Commit Message

Ondrej Mosnacek July 9, 2019, 11:11 a.m. UTC
Currently, NETLINK_CRYPTO works only in the init network namespace. It
doesn't make much sense to cut it out of the other network namespaces,
so do the minor plumbing work necessary to make it work in any network
namespace. Code inspired by net/core/sock_diag.c.

Tested using kcapi-dgst from libkcapi [1]:
Before:
    # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
    libkcapi - Error: Netlink error: sendmsg failed
    libkcapi - Error: Netlink error: sendmsg failed
    libkcapi - Error: NETLINK_CRYPTO: cannot obtain cipher information for hmac(sha512) (is required crypto_user.c patch missing? see documentation)
    0

After:
    # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
    32

[1] https://github.com/smuellerDD/libkcapi

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 crypto/crypto_user_base.c            | 37 +++++++++++++++++++---------
 crypto/crypto_user_stat.c            |  4 ++-
 include/crypto/internal/cryptouser.h |  2 --
 include/net/net_namespace.h          |  3 +++
 4 files changed, 31 insertions(+), 15 deletions(-)

Comments

Herbert Xu July 9, 2019, 2:38 p.m. UTC | #1
On Tue, Jul 09, 2019 at 01:11:24PM +0200, Ondrej Mosnacek wrote:
> Currently, NETLINK_CRYPTO works only in the init network namespace. It
> doesn't make much sense to cut it out of the other network namespaces,
> so do the minor plumbing work necessary to make it work in any network
> namespace. Code inspired by net/core/sock_diag.c.
> 
> Tested using kcapi-dgst from libkcapi [1]:
> Before:
>     # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
>     libkcapi - Error: Netlink error: sendmsg failed
>     libkcapi - Error: Netlink error: sendmsg failed
>     libkcapi - Error: NETLINK_CRYPTO: cannot obtain cipher information for hmac(sha512) (is required crypto_user.c patch missing? see documentation)
>     0
> 
> After:
>     # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
>     32
> 
> [1] https://github.com/smuellerDD/libkcapi
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Should we really let root inside a namespace manipulate crypto
algorithms which are global?

I think we should only allow the query operations without deeper
surgery.

Cheers,
Ondrej Mosnacek July 9, 2019, 3:28 p.m. UTC | #2
On Tue, Jul 9, 2019 at 4:38 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jul 09, 2019 at 01:11:24PM +0200, Ondrej Mosnacek wrote:
> > Currently, NETLINK_CRYPTO works only in the init network namespace. It
> > doesn't make much sense to cut it out of the other network namespaces,
> > so do the minor plumbing work necessary to make it work in any network
> > namespace. Code inspired by net/core/sock_diag.c.
> >
> > Tested using kcapi-dgst from libkcapi [1]:
> > Before:
> >     # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
> >     libkcapi - Error: Netlink error: sendmsg failed
> >     libkcapi - Error: Netlink error: sendmsg failed
> >     libkcapi - Error: NETLINK_CRYPTO: cannot obtain cipher information for hmac(sha512) (is required crypto_user.c patch missing? see documentation)
> >     0
> >
> > After:
> >     # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
> >     32
> >
> > [1] https://github.com/smuellerDD/libkcapi
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Should we really let root inside a namespace manipulate crypto
> algorithms which are global?

I admit I'm not an expert on Linux namespaces, but aren't you
confusing network and user namespaces? Unless I'm mistaken, these
changes only affect _network_ namespaces (which only isolate the
network stuff itself) and the semantics of the netlink_capable(skb,
CAP_NET_ADMIN) calls remain unchanged - they check if the opener of
the socket has the CAP_NET_ADMIN capability within the global _user_
namespace.

>
> I think we should only allow the query operations without deeper
> surgery.
>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu July 9, 2019, 4:14 p.m. UTC | #3
On Tue, Jul 09, 2019 at 05:28:35PM +0200, Ondrej Mosnacek wrote:
>
> I admit I'm not an expert on Linux namespaces, but aren't you
> confusing network and user namespaces? Unless I'm mistaken, these
> changes only affect _network_ namespaces (which only isolate the
> network stuff itself) and the semantics of the netlink_capable(skb,
> CAP_NET_ADMIN) calls remain unchanged - they check if the opener of
> the socket has the CAP_NET_ADMIN capability within the global _user_
> namespace.

Good point.  I think your patch should be OK then.

Thanks,
Herbert Xu July 26, 2019, 12:32 p.m. UTC | #4
Ondrej Mosnacek <omosnace@redhat.com> wrote:
> Currently, NETLINK_CRYPTO works only in the init network namespace. It
> doesn't make much sense to cut it out of the other network namespaces,
> so do the minor plumbing work necessary to make it work in any network
> namespace. Code inspired by net/core/sock_diag.c.
> 
> Tested using kcapi-dgst from libkcapi [1]:
> Before:
>    # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
>    libkcapi - Error: Netlink error: sendmsg failed
>    libkcapi - Error: Netlink error: sendmsg failed
>    libkcapi - Error: NETLINK_CRYPTO: cannot obtain cipher information for hmac(sha512) (is required crypto_user.c patch missing? see documentation)
>    0
> 
> After:
>    # unshare -n kcapi-dgst -c sha256 </dev/null | wc -c
>    32
> 
> [1] https://github.com/smuellerDD/libkcapi
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> crypto/crypto_user_base.c            | 37 +++++++++++++++++++---------
> crypto/crypto_user_stat.c            |  4 ++-
> include/crypto/internal/cryptouser.h |  2 --
> include/net/net_namespace.h          |  3 +++
> 4 files changed, 31 insertions(+), 15 deletions(-)

Patch applied.  Thanks.

Patch
diff mbox series

diff --git a/crypto/crypto_user_base.c b/crypto/crypto_user_base.c
index e48da3b75c71..c92d415eaf82 100644
--- a/crypto/crypto_user_base.c
+++ b/crypto/crypto_user_base.c
@@ -22,9 +22,10 @@ 
 #include <linux/crypto.h>
 #include <linux/cryptouser.h>
 #include <linux/sched.h>
-#include <net/netlink.h>
 #include <linux/security.h>
+#include <net/netlink.h>
 #include <net/net_namespace.h>
+#include <net/sock.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/internal/rng.h>
 #include <crypto/akcipher.h>
@@ -37,9 +38,6 @@ 
 
 static DEFINE_MUTEX(crypto_cfg_mutex);
 
-/* The crypto netlink socket */
-struct sock *crypto_nlsk;
-
 struct crypto_dump_info {
 	struct sk_buff *in_skb;
 	struct sk_buff *out_skb;
@@ -195,6 +193,7 @@  out:
 static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
 			 struct nlattr **attrs)
 {
+	struct net *net = sock_net(in_skb->sk);
 	struct crypto_user_alg *p = nlmsg_data(in_nlh);
 	struct crypto_alg *alg;
 	struct sk_buff *skb;
@@ -226,7 +225,7 @@  drop_alg:
 	if (err)
 		return err;
 
-	return nlmsg_unicast(crypto_nlsk, skb, NETLINK_CB(in_skb).portid);
+	return nlmsg_unicast(net->crypto_nlsk, skb, NETLINK_CB(in_skb).portid);
 }
 
 static int crypto_dump_report(struct sk_buff *skb, struct netlink_callback *cb)
@@ -429,6 +428,7 @@  static const struct crypto_link {
 static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			       struct netlink_ext_ack *extack)
 {
+	struct net *net = sock_net(skb->sk);
 	struct nlattr *attrs[CRYPTOCFGA_MAX+1];
 	const struct crypto_link *link;
 	int type, err;
@@ -459,7 +459,7 @@  static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 				.done = link->done,
 				.min_dump_alloc = min(dump_alloc, 65535UL),
 			};
-			err = netlink_dump_start(crypto_nlsk, skb, nlh, &c);
+			err = netlink_dump_start(net->crypto_nlsk, skb, nlh, &c);
 		}
 
 		return err;
@@ -483,22 +483,35 @@  static void crypto_netlink_rcv(struct sk_buff *skb)
 	mutex_unlock(&crypto_cfg_mutex);
 }
 
-static int __init crypto_user_init(void)
+static int __net_init crypto_netlink_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= crypto_netlink_rcv,
 	};
 
-	crypto_nlsk = netlink_kernel_create(&init_net, NETLINK_CRYPTO, &cfg);
-	if (!crypto_nlsk)
-		return -ENOMEM;
+	net->crypto_nlsk = netlink_kernel_create(net, NETLINK_CRYPTO, &cfg);
+	return net->crypto_nlsk == NULL ? -ENOMEM : 0;
+}
 
-	return 0;
+static void __net_exit crypto_netlink_exit(struct net *net)
+{
+	netlink_kernel_release(net->crypto_nlsk);
+	net->crypto_nlsk = NULL;
+}
+
+static struct pernet_operations crypto_netlink_net_ops = {
+	.init = crypto_netlink_init,
+	.exit = crypto_netlink_exit,
+};
+
+static int __init crypto_user_init(void)
+{
+	return register_pernet_subsys(&crypto_netlink_net_ops);
 }
 
 static void __exit crypto_user_exit(void)
 {
-	netlink_kernel_release(crypto_nlsk);
+	unregister_pernet_subsys(&crypto_netlink_net_ops);
 }
 
 module_init(crypto_user_init);
diff --git a/crypto/crypto_user_stat.c b/crypto/crypto_user_stat.c
index a03f326a63d3..8bad88413de1 100644
--- a/crypto/crypto_user_stat.c
+++ b/crypto/crypto_user_stat.c
@@ -10,6 +10,7 @@ 
 #include <linux/cryptouser.h>
 #include <linux/sched.h>
 #include <net/netlink.h>
+#include <net/sock.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/internal/rng.h>
 #include <crypto/akcipher.h>
@@ -298,6 +299,7 @@  out:
 int crypto_reportstat(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
 		      struct nlattr **attrs)
 {
+	struct net *net = sock_net(in_skb->sk);
 	struct crypto_user_alg *p = nlmsg_data(in_nlh);
 	struct crypto_alg *alg;
 	struct sk_buff *skb;
@@ -329,7 +331,7 @@  drop_alg:
 	if (err)
 		return err;
 
-	return nlmsg_unicast(crypto_nlsk, skb, NETLINK_CB(in_skb).portid);
+	return nlmsg_unicast(net->crypto_nlsk, skb, NETLINK_CB(in_skb).portid);
 }
 
 MODULE_LICENSE("GPL");
diff --git a/include/crypto/internal/cryptouser.h b/include/crypto/internal/cryptouser.h
index 8c602b187c58..40623f4457df 100644
--- a/include/crypto/internal/cryptouser.h
+++ b/include/crypto/internal/cryptouser.h
@@ -1,8 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <net/netlink.h>
 
-extern struct sock *crypto_nlsk;
-
 struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact);
 
 #ifdef CONFIG_CRYPTO_STATS
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 12689ddfc24c..610e40eaea52 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -165,6 +165,9 @@  struct net {
 #endif
 #ifdef CONFIG_XDP_SOCKETS
 	struct netns_xdp	xdp;
+#endif
+#if IS_ENABLED(CONFIG_CRYPTO_USER)
+	struct sock		*crypto_nlsk;
 #endif
 	struct sock		*diag_nlsk;
 	atomic_t		fnhe_genid;