From patchwork Fri Nov 19 15:11:55 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Moore X-Patchwork-Id: 72258 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3A5F01007D2 for ; Sat, 20 Nov 2010 02:12:38 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754753Ab0KSPMF (ORCPT ); Fri, 19 Nov 2010 10:12:05 -0500 Received: from g1t0026.austin.hp.com ([15.216.28.33]:34756 "EHLO g1t0026.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753473Ab0KSPME (ORCPT ); Fri, 19 Nov 2010 10:12:04 -0500 Received: from g1t0038.austin.hp.com (g1t0038.austin.hp.com [16.236.32.44]) by g1t0026.austin.hp.com (Postfix) with ESMTP id F26B1C61D; Fri, 19 Nov 2010 15:12:00 +0000 (UTC) Received: from ldl (ldl.usa.hp.com [16.125.112.222]) by g1t0038.austin.hp.com (Postfix) with ESMTP id DC7C730370; Fri, 19 Nov 2010 15:11:58 +0000 (UTC) Received: from localhost (ldl.fc.hp.com [127.0.0.1]) by ldl (Postfix) with ESMTP id AE79FCF0020; Fri, 19 Nov 2010 08:11:58 -0700 (MST) Received: from ldl ([127.0.0.1]) by localhost (ldl.fc.hp.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uSRQz5HJr5Qh; Fri, 19 Nov 2010 08:11:58 -0700 (MST) Received: from [192.168.0.132] (squirrel.fc.hp.com [15.11.146.57]) (Authenticated sender: pmoore@fc.hp.com) by ldl (Postfix) with ESMTPSA id 8CCEFCF0008; Fri, 19 Nov 2010 08:11:56 -0700 (MST) Subject: Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error From: Paul Moore To: Eric Paris Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, selinux@tycho.nsa.gov, netfilter-devel@vger.kernel.org, equinox@diac24.net, eric.dumazet@gmail.com, davem@davemloft.net, hzhong@gmail.com, jmorris@namei.org, kaber@trash.net, kuznet@ms2.inr.ac.ru, pekkas@netcore.fi, sds@tycho.nsa.gov, yoshfuji@linux-ipv6.org In-Reply-To: <20101116215257.6727.12163.stgit@paris.rdu.redhat.com> References: <20101116215238.6727.39248.stgit@paris.rdu.redhat.com> <20101116215257.6727.12163.stgit@paris.rdu.redhat.com> Organization: Hewlett Packard Date: Fri, 19 Nov 2010 10:11:55 -0500 Message-ID: <1290179515.8839.41.camel@sifl> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, 2010-11-16 at 16:52 -0500, Eric Paris wrote: > The SELinux netfilter hooks just return NF_DROP if they drop a packet. We > want to signal that a drop in this hook is a permanant fatal error and is not > transient. If we do this the error will be passed back up the stack in some > places and applications will get a faster interaction that something went > wrong. Sorry for the delay, but I wasn't able to respond until today ... > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 8ba5001..b1104f9 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4594,11 +4594,11 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, > secmark_perm = PACKET__SEND; > break; > default: > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); > } > if (secmark_perm == PACKET__FORWARD_OUT) { > if (selinux_skb_peerlbl_sid(skb, family, &peer_sid)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); The error condition here isn't due to a policy decision, but some runtime error that happened when trying to determine the peer label of an individual packet. I think leaving this as just NF_DROP is the right thing to do as an error here can be temporary. > } else > peer_sid = SECINITSID_KERNEL; > } else { > @@ -4611,28 +4611,28 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex, > ad.u.net.netif = ifindex; > ad.u.net.family = family; > if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Same thing, this is a transient error and not due to a policy decision. We should keep this as NF_DROP. > if (secmark_active) > if (avc_has_perm(peer_sid, skb->secmark, > SECCLASS_PACKET, secmark_perm, &ad)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Yep, this is a good place as the error is the result of a policy decision, i.e. an avc_has_perm() call. > if (peerlbl_active) { > u32 if_sid; > u32 node_sid; > > if (sel_netif_sid(ifindex, &if_sid)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Another transient error case, should be NF_DROP. > if (avc_has_perm(peer_sid, if_sid, > SECCLASS_NETIF, NETIF__EGRESS, &ad)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Good. > if (sel_netnode_sid(addrp, family, &node_sid)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Bad. > if (avc_has_perm(peer_sid, node_sid, > SECCLASS_NODE, NODE__SENDTO, &ad)) > - return NF_DROP; > + return NF_DROP_ERR(-ECONNREFUSED); Good. I think you get the idea now. Also, while I think we can ignore the forwarding and output hooks, we do need to change the NF_DROPs in selinux_ip_postroute_compat() ... I'd suggest the following (the last change catches more than just policy decisions but considering the "compat" nature I think a little wiggle room is okay here): diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 65fa8bf..de1b79e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4520,11 +4520,11 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb, if (selinux_secmark_enabled()) if (avc_has_perm(sksec->sid, skb->secmark, SECCLASS_PACKET, PACKET__SEND, &ad)) - return NF_DROP; + return NF_DROP_ERR(-ECONNREFUSED); if (selinux_policycap_netpeer) if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto)) - return NF_DROP; + return NF_DROP_ERR(-ECONNREFUSED); return NF_ACCEPT; }