diff mbox

IB/sa: Restrict SA Netlink to admin users

Message ID 1438895310-6087-1-git-send-email-ira.weiny@intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Ira Weiny Aug. 6, 2015, 9:08 p.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

The recently added SA Netlink service requires admin privileges to receive
kernel requests.  This is only partially sufficient to protect the kernel from
malicious users.  This patch fixes two issues.

	1) Path responses from user space could be spoofed if the sequence
	   number was properly guessed.
	2) The set timeout request message could be issued by any user.

Ignore these messages if not submitted by an admin user.

Fixes: 6619209af36c ("IB/sa: Route SA pathrecord query through netlink")
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---

Doug let me know if you would prefer that I get Kaike to squash this into the
original patch.

 drivers/infiniband/core/sa_query.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Haggai Eran Aug. 10, 2015, 6:11 a.m. UTC | #1 
On 07/08/2015 00:08, ira.weiny@intel.com wrote:
> @@ -754,6 +764,12 @@ static int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>  	int found = 0;
>  	int ret;
>  
> +	if (!ns_capable(sock_net(skb->sk)->user_ns, CAP_NET_ADMIN)) {
> +		pr_warn_ratelimited("SA netlink: invalid perm for response: `%s'.\n",
> +				    current->comm);
> +		return -EPERM;
> +	}
> +
>  	spin_lock_irqsave(&ib_nl_request_lock, flags);
>  	list_for_each_entry(query, &ib_nl_request_list, list) {
>  		/*

Maybe I'm missing something, but I thought you would want to check the
capability with init_user_ns as the target, since the SA queries will
affect all namespaces, not just the one that sent the response.

Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Aug. 10, 2015, 9:58 p.m. UTC | #2 
On Mon, Aug 10, 2015 at 09:11:08AM +0300, Haggai Eran wrote:
> On 07/08/2015 00:08, ira.weiny@intel.com wrote:
> > @@ -754,6 +764,12 @@ static int ib_nl_handle_resolve_resp(struct sk_buff *skb,
> >  	int found = 0;
> >  	int ret;
> >  
> > +	if (!ns_capable(sock_net(skb->sk)->user_ns, CAP_NET_ADMIN)) {
> > +		pr_warn_ratelimited("SA netlink: invalid perm for response: `%s'.\n",
> > +				    current->comm);
> > +		return -EPERM;
> > +	}
> > +
> >  	spin_lock_irqsave(&ib_nl_request_lock, flags);
> >  	list_for_each_entry(query, &ib_nl_request_list, list) {
> >  		/*
> 
> Maybe I'm missing something, but I thought you would want to check the
> capability with init_user_ns as the target, since the SA queries will
> affect all namespaces, not just the one that sent the response.

I'm not an expert in namespaces but these checks are only concerned with the
network namespace of the process which is acting as an SA proxy (via netlink).

AFAIK a user can't elevate a network namespace that the SA proxy is running in.
While you are correct that this data does affect all namespaces who are using a
particular port, this does not matter.  The SA data for that port is applicable
no matter which network namespace an application running on that port is in.

Furthermore, the check in netlink_bind also uses the socket namespace to
restrict the use of multicast.  This plus my checks should allow an admin to
place the SA proxy (ibacm in our test cases) in an alternate network namespace
if they so desire.  But this is independent to the namespace which may be used
for data applications.


All that said, I did find the netlink_net_capable helper call which I should
have used instead.  I'll work up a v2 today.

Thanks,
Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Aug. 11, 2015, 5:38 a.m. UTC | #3 
On Mon, Aug 10, 2015 at 05:58:30PM -0400, ira.weiny wrote:

> Furthermore, the check in netlink_bind also uses the socket namespace to
> restrict the use of multicast.  This plus my checks should allow an admin to
> place the SA proxy (ibacm in our test cases) in an alternate network namespace
> if they so desire.  But this is independent to the namespace which may be used
> for data applications.

I think Haggai is on to something, there is certainly a problem here,
that netlink_bind will let a namespace subscribe is a certainly a
problem for what Haggai is working on.

For now, I think, only root (or CAP_ whatever) in the init namespace
should have access to this feature. Not sure how to check that.

Even allowing a namespace to subscribe is problematic because it will
cause timeouts to hit.. Not sure what to do about that..

Also, why the incremental patch? The original isn't ready for mainline
without the message validation stuff..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wan, Kaike Aug. 11, 2015, 11:27 a.m. UTC | #4 
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Jason Gunthorpe
> Sent: Tuesday, August 11, 2015 1:38 AM
> To: Weiny, Ira
> Cc: Haggai Eran; dledford@redhat.com; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH] IB/sa: Restrict SA Netlink to admin users
> 
> On Mon, Aug 10, 2015 at 05:58:30PM -0400, ira.weiny wrote:
> 
> > Furthermore, the check in netlink_bind also uses the socket namespace
> > to restrict the use of multicast.  This plus my checks should allow an
> > admin to place the SA proxy (ibacm in our test cases) in an alternate
> > network namespace if they so desire.  But this is independent to the
> > namespace which may be used for data applications.
> 
> I think Haggai is on to something, there is certainly a problem here, that
> netlink_bind will let a namespace subscribe is a certainly a problem for what
> Haggai is working on.
> 
> For now, I think, only root (or CAP_ whatever) in the init namespace should
> have access to this feature. Not sure how to check that.

netlink_capable(skb, CAP_NET_ADMIN) will do the trick.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Aug. 11, 2015, 4:26 p.m. UTC | #5 
On Mon, Aug 10, 2015 at 11:38:10PM -0600, Jason Gunthorpe wrote:
> On Mon, Aug 10, 2015 at 05:58:30PM -0400, ira.weiny wrote:
> 
> > Furthermore, the check in netlink_bind also uses the socket namespace to
> > restrict the use of multicast.  This plus my checks should allow an admin to
> > place the SA proxy (ibacm in our test cases) in an alternate network namespace
> > if they so desire.  But this is independent to the namespace which may be used
> > for data applications.
> 
> I think Haggai is on to something, there is certainly a problem here,
> that netlink_bind will let a namespace subscribe is a certainly a
> problem for what Haggai is working on.

Ok, After thinking about this more I agree.  Haggai has a point about the arp
tables.  Like I said I'm not a namespace expert.

> 
> For now, I think, only root (or CAP_ whatever) in the init namespace
> should have access to this feature. Not sure how to check that.

For these 2 checks it is easy to change to netlink_capable instead of
netlink_net_capable.

> 
> Even allowing a namespace to subscribe is problematic because it will
> cause timeouts to hit.. Not sure what to do about that..


Ok, I look into how to deal with the netlink_bind.  I _think_ this may require
the RDMA netlink to provide a custom bind call.  :-(

> 
> Also, why the incremental patch? The original isn't ready for mainline
> without the message validation stuff..

Mainly because Kaike was on vacation and I was not sure what Doug would prefer.
Kaike and I have discussed a couple of changes he had queued up so we will need
a v9 so we will merge this into his next v9 submission.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Aug. 11, 2015, 4:27 p.m. UTC | #6 
On Tue, Aug 11, 2015 at 05:27:17AM -0600, Wan, Kaike wrote:
> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> > owner@vger.kernel.org] On Behalf Of Jason Gunthorpe
> > Sent: Tuesday, August 11, 2015 1:38 AM
> > To: Weiny, Ira
> > Cc: Haggai Eran; dledford@redhat.com; linux-rdma@vger.kernel.org
> > Subject: Re: [PATCH] IB/sa: Restrict SA Netlink to admin users
> > 
> > On Mon, Aug 10, 2015 at 05:58:30PM -0400, ira.weiny wrote:
> > 
> > > Furthermore, the check in netlink_bind also uses the socket namespace
> > > to restrict the use of multicast.  This plus my checks should allow an
> > > admin to place the SA proxy (ibacm in our test cases) in an alternate
> > > network namespace if they so desire.  But this is independent to the
> > > namespace which may be used for data applications.
> > 
> > I think Haggai is on to something, there is certainly a problem here, that
> > netlink_bind will let a namespace subscribe is a certainly a problem for what
> > Haggai is working on.
> > 
> > For now, I think, only root (or CAP_ whatever) in the init namespace should
> > have access to this feature. Not sure how to check that.
> 
> netlink_capable(skb, CAP_NET_ADMIN) will do the trick.
 
For these calls yes.  For the bind call I think we need to investigate more.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 70ceec4df02a..52be6d71b2f4 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -49,6 +49,7 @@ 
 #include <net/netlink.h>
 #include <uapi/rdma/ib_user_sa.h>
 #include <rdma/ib_marshall.h>
+#include <net/sock.h>
 #include "sa.h"
 
 MODULE_AUTHOR("Roland Dreier");
@@ -699,6 +700,12 @@  static int ib_nl_handle_set_timeout(struct sk_buff *skb,
 	struct nlattr *tb[LS_NLA_TYPE_MAX + 1];
 	int ret;
 
+	if (!ns_capable(sock_net(skb->sk)->user_ns, CAP_NET_ADMIN)) {
+		pr_warn_ratelimited("SA netlink: invalid perm for set timeout: `%s'.\n",
+				    current->comm);
+		return -EPERM;
+	}
+
 	ret = nla_parse(tb, LS_NLA_TYPE_MAX, nlmsg_data(nlh), nlmsg_len(nlh),
 			NULL);
 	attr = (const struct nlattr *)tb[LS_NLA_TYPE_TIMEOUT];
@@ -706,6 +713,9 @@  static int ib_nl_handle_set_timeout(struct sk_buff *skb,
 		goto settimeout_out;
 
 	timeout = *(int *) nla_data(attr);
+
+	pr_info("SA netlink: timeout: %d\n", timeout);
+
 	if (timeout < IB_SA_LOCAL_SVC_TIMEOUT_MIN)
 		timeout = IB_SA_LOCAL_SVC_TIMEOUT_MIN;
 	if (timeout > IB_SA_LOCAL_SVC_TIMEOUT_MAX)
@@ -754,6 +764,12 @@  static int ib_nl_handle_resolve_resp(struct sk_buff *skb,
 	int found = 0;
 	int ret;
 
+	if (!ns_capable(sock_net(skb->sk)->user_ns, CAP_NET_ADMIN)) {
+		pr_warn_ratelimited("SA netlink: invalid perm for response: `%s'.\n",
+				    current->comm);
+		return -EPERM;
+	}
+
 	spin_lock_irqsave(&ib_nl_request_lock, flags);
 	list_for_each_entry(query, &ib_nl_request_list, list) {
 		/*
@@ -770,6 +786,7 @@  static int ib_nl_handle_resolve_resp(struct sk_buff *skb,
 
 	if (!found) {
 		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+		pr_err_ratelimited("SA netlink: got unmatched response\n");
 		goto resp_out;
 	}