Stephen Smalley wrote:
On Wed, 2006-05-03 at 15:53 -0400, Paul Moore wrote:
diff -purN kernel-2.6.16/security/selinux/hooks.c
kernel-2.6.16-cipso_05032006/security/selinux/hooks.c
--- kernel-2.6.16/security/selinux/hooks.c 2006-05-02 10:41:02.000000000
-0400
+++ kernel-2.6.16-cipso_05032006/security/selinux/hooks.c 2006-05-02
14:19:13.000000000 -0400
@@ -3304,10 +3317,26 @@ static int selinux_socket_sock_rcv_skb(s
err = avc_has_perm(sock_sid, port_sid,
sock_class, recv_perm, &ad);
+ if (err)
+ goto out;
}
- if (!err)
- err = selinux_xfrm_sock_rcv_skb(sock_sid, skb);
+ /* PM - discuss these changes on the list */
+ err = selinux_xfrm_sock_rcv_skb(sock_sid, skb);
+ if (err == 0)
+ goto out;
+
+ err = security_netlbl_sid(skb, &netlbl_sid);
+ if (err)
+ goto out;
+
+ err = avc_has_perm(sock_sid,
+ netlbl_sid,
+ SECCLASS_SOCKET,
+ SOCKET__RECV_MSG,
+ &ad);
+ if (err)
+ netlbl_skbuff_err(skb, err);
We'll need to resolve how this should interact with the security marking
patches under development by James, see the RFC at:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114516429530333&w=2
Patches should be forthcoming soon.
Those are intended to replace the existing SELinux netif/node/port
per-packet checks with a single per-packet check on each send/receive
based on the secmark assigned via iptables rules. Exactly how that
should interact with both the implicit packet labeling via IPSEC and
with your NetLabel code isn't entirely clear to me; ideally, they could
all leverage the secmark functionality and everything could devolve to a
single check on send/recv based on the secmark.
Ever since the SELinux Developers Summit this year I knew this was going
to be an issue, hence my comment in the code above. At this point I
don't have a clear enough picture of all three approaches to have a very
strong opinion. Although, I'll be honest here, I'll go with whatever
approach gets CIPSO support into the kernel.
One of my main design goals for NetLabel have been to minimize the
intrustion into the kernel network stack and the SELinux code. As a
result I think the NetLabel bits have turned out to be fairly generic in
the sense that I *believe* it could support many different types of
packet labeling based on either explicit tags (CIPSO, RIPSO, etc.),
implicit tags (IPsec SA labeling), or self assigned tags (the existing
netif/node/port checks, the secmark approach) in a way that is as LSM
generic as possibile (might not be a big issue now, but it may become
important in the future). However, the NetLabel hooks could also just
as easily be folded into another mechanism like secmark (I don't think
the existing XFRM code is generic enough to support NetLabel), although
I haven't looked at the secmark stuff in too much detail yet.
Regardless, I agree this is something that needs to be discussed as
there are a lot of different approaches that will need to be reconciled.
Do you have a suggestion of the right place to discuss this - the
netdev list, the SELinux list, here?
diff -purN kernel-2.6.16/security/selinux/ss/services.c
kernel-2.6.16-cipso_05032006/security/selinux/ss/services.c
--- kernel-2.6.16/security/selinux/ss/services.c 2006-05-02
10:41:02.000000000 -0400
+++ kernel-2.6.16-cipso_05032006/security/selinux/ss/services.c 2006-05-02
13:22:45.000000000 -0400
+int security_netlbl_sid(struct sk_buff *skb, u32 *sid)
+{
+ int ret_val = -EIDRM;
+ struct netlbl_lsm_secattr secattr;
+ struct context *ctx;
+ struct context ctx_new;
+
+ if (!ss_initialized)
+ return 0;
+
+ context_init(&ctx_new);
+ netlbl_secattr_init(&secattr);
+ secattr.lsm_type = NETLBL_LSM_SELINUX;
+ ret_val = netlbl_skbuff_read(skb, &secattr);
+ if (ret_val != 0)
+ goto netlbl_sid_failure;
+
+ if (secattr.set_selinux_sid)
+ *sid = secattr.lsm.selinux.sid;
Where do you check that the SID is defined/valid?
I don't. The NetLabel module treats the SID as an opaque value and just
returns the value that SELinux gives to it via the call to
netlbl_cache_add() further down in this function (the cache is
invalidated on policy loads/reloads). In the future the NetLabel code
made provide a SID when Selopt is used but that code still only exists
in my head.
Is there a SID verification function I should be calling? I always
thought that invalid SIDs were just mapped to an unlabled context, so
even if something went wrong and the SID was garbage/incorrect the worst
that would happen would be the packet would be "unlabeled", no?
+ } else if (secattr.set_mls_lvl) {
+ ctx = sidtab_search(&sidtab, SECINITSID_NETMSG);
+ if (ctx == NULL)
+ goto netlbl_sid_failure;
+
+ ret_val = context_cpy(&ctx_new, ctx);
+ if (ret_val != 0)
+ goto netlbl_sid_failure;
+ mls_context_destroy(&ctx_new);
+
+ if (mls_import_lvl(&ctx_new,
+ secattr.mls_lvl,
+ secattr.mls_lvl) != 0)
+ goto netlbl_sid_failure;
+ if (secattr.set_mls_cat) {
+ if (mls_import_cat(&ctx_new,
+ secattr.mls_cat,
+ secattr.mls_cat_len,
+ secattr.mls_cat,
+ secattr.mls_cat_len) != 0)
+ goto netlbl_sid_failure;
+ }
Where do you check that the MLS field values are defined and form a
legal combination under the policy, e.g. mls_context_isvalid?
Hmmm, good point. I made the mistake of thinking that the
sidtab_context_to_sid() function would verify the context like
security_context_to_sid(), but now that I am looking at it a bit closer
I realize I was wrong. Sorry about that.
I'll add the following before the call to sidtab_context_to_sid():
ret_val = mls_context_isvalid(policydb, &ctx_new);
if (ret_val != 0)
goto netlbl_sid_failure;
diff -purN kernel-2.6.16/security/selinux/xfrm.c
kernel-2.6.16-cipso_05032006/security/selinux/xfrm.c
--- kernel-2.6.16/security/selinux/xfrm.c 2006-05-02 10:40:28.000000000
-0400
+++ kernel-2.6.16-cipso_05032006/security/selinux/xfrm.c 2006-05-02
14:20:51.000000000 -0400
@@ -317,21 +317,12 @@ int selinux_xfrm_sock_rcv_skb(u32 isec_s
struct xfrm_state *x = sp->xvec[i];
if (x && selinux_authorizable_xfrm(x))
- goto accept;
+ return 0;
}
+ return -EPERM;
}
- /* check SELinux sock for unlabelled access */
- rc = avc_has_perm(isec_sid, SECINITSID_UNLABELED, SECCLASS_ASSOCIATION,
- ASSOCIATION__RECVFROM, NULL);
- if (rc)
- goto drop;
-
-accept:
- return 0;
-
-drop:
- return rc;
+ return -EIDRM;
}
Not clear what you are doing here with these different error codes. I
do agree that we want to unify the different per-packet checks.
Basically I was trying to be a bit more informative in case somebody
ever wants to know the different between a failed IPsec authorization,
-EPERM, and a packet that didn't even pass though IPsec, -EIDRM. At
least that is how I understand the code to work, please correct me if
I'm wrong. I figured it was cheap to provide more information so why
not do it?
Thanks for your comments, I appreciate the feedback.
--
paul moore
linux security @ hp
--
redhat-lspp mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/redhat-lspp
