11:20 a.m.
Hi,
Is there any objection to my sending the two netlink patches I recently
sent out to lkml? Just to refresh memory, the one (audit-fix-
permchecks.diff) adds some message length checks and moves audit control
message authorization to netlink message send, while the other (audit-
loginuid.patch) changes the SET_LOGINUID behavior to set loginuid for
the sending process (as expected) rather than whichever process happens
to end up handling the message.
thanks,
-serge
--
Serge Hallyn <serue(a)us.ibm.com>
12:58 p.m.
Sounds good, thanks. I'll wait to send anything out then.
-serge
On Wed, 2004-12-15 at 12:43 -0500, Steve Grubb wrote:
On Wednesday 15 December 2004 12:20, Serge Hallyn wrote:
> Is there any objection to my sending the two netlink patches I recently
> sent out to lkml?
I'm still looking at it. I think I do see one change to the permchecks patch.
I'll get back with you on it.
-Steve Grubb
--
Linux-audit mailing list
Linux-audit(a)redhat.com
http://www.redhat.com/mailman/listinfo/linux-audit
--
Serge Hallyn <serue(a)us.ibm.com>
11:56 a.m.
Did you run any specific tests against the patches at IBM?
Regards,
Peter
On Wed, 2004-12-15 at 13:58, Serge Hallyn wrote:
Sounds good, thanks. I'll wait to send anything out then.
-serge
On Wed, 2004-12-15 at 12:43 -0500, Steve Grubb wrote:
> On Wednesday 15 December 2004 12:20, Serge Hallyn wrote:
> > Is there any objection to my sending the two netlink patches I recently
> > sent out to lkml?
>
> I'm still looking at it. I think I do see one change to the permchecks patch.
> I'll get back with you on it.
>
> -Steve Grubb
>
> --
> Linux-audit mailing list
> Linux-audit(a)redhat.com
> http://www.redhat.com/mailman/listinfo/linux-audit
>
1:41 p.m.
The SourceForge project (http://sourceforge.net/projects/audit/) seems
to be exclusively for userspace audit development. We're going to
want a place to put the entire patch-set for the kernel audit piece..
Perhaps we can expand the definition of the SourceForge project to
include both the userspace and kernelspace pieces?
-Tim
On Wed, 15 Dec 2004 12:56:53 -0500, Peter Martuccelli <peterm(a)redhat.com> wrote:
Did you run any specific tests against the patches at IBM?
Regards,
Peter
On Wed, 2004-12-15 at 13:58, Serge Hallyn wrote:
> Sounds good, thanks. I'll wait to send anything out then.
>
> -serge
>
> On Wed, 2004-12-15 at 12:43 -0500, Steve Grubb wrote:
> > On Wednesday 15 December 2004 12:20, Serge Hallyn wrote:
> > > Is there any objection to my sending the two netlink patches I recently
> > > sent out to lkml?
> >
> > I'm still looking at it. I think I do see one change to the permchecks
patch.
> > I'll get back with you on it.
> >
> > -Steve Grubb
> >
> > --
> > Linux-audit mailing list
> > Linux-audit(a)redhat.com
> > http://www.redhat.com/mailman/listinfo/linux-audit
> >
--
Linux-audit mailing list
Linux-audit(a)redhat.com
http://www.redhat.com/mailman/listinfo/linux-audit
--
- Timothy R. Chavez
1:52 p.m.
On Wednesday 15 December 2004 14:41, Timothy R. Chavez wrote:
We're going to want a place to put the entire patch-set for the
kernel
audit piece..
These kernel pieces, are you talking about a place where people can download
patches? Or cvs?
Ideally, these patches should be short lived and pushed upstream.
-Steve Grubb
3:12 p.m.
I for one think a simple patch repository would be nice. CVS isn't as
useful, as we presumably want to separate patchsets (touching
overlapping kernel files) by functionality.
-serge
On Wed, 2004-12-15 at 14:52 -0500, Steve Grubb wrote:
On Wednesday 15 December 2004 14:41, Timothy R. Chavez wrote:
> We're going to want a place to put the entire patch-set for the kernel
> audit piece..
These kernel pieces, are you talking about a place where people can download
patches? Or cvs?
Ideally, these patches should be short lived and pushed upstream.
-Steve Grubb
--
Linux-audit mailing list
Linux-audit(a)redhat.com
http://www.redhat.com/mailman/listinfo/linux-audit
--
Serge Hallyn <serue(a)us.ibm.com>
2:02 p.m.
On Wednesday 15 December 2004 12:43, Steve Grubb wrote:
I'm still looking at it. I think I do see one change to the
permchecks
patch. I'll get back with you on it.
OK. For the permchecks patch, doesn't AUDIT_GET need to be handled in the
switch() in cap_netlink_audit_check & the dummy version similar to
AUDIT_LIST?
That's the only issue I have with this patch.
-Steve Grubb
3:49 p.m.
Good point. New patch is attached. If there are no more issues, I will
send it out to lkml tomorrow.
(My original thought was to wait until we discuss more in-depth whether
to move to non-netlink interface for control, but really that discussion
would not be fair if we start from a known bad netlink implementation :)
thanks,
-serge
On Wed, 2004-12-15 at 15:02 -0500, Steve Grubb wrote:
On Wednesday 15 December 2004 12:43, Steve Grubb wrote:
> I'm still looking at it. I think I do see one change to the permchecks
> patch. I'll get back with you on it.
OK. For the permchecks patch, doesn't AUDIT_GET need to be handled in the
switch() in cap_netlink_audit_check & the dummy version similar to
AUDIT_LIST?
That's the only issue I have with this patch.
-Steve Grubb
--
Linux-audit mailing list
Linux-audit(a)redhat.com
http://www.redhat.com/mailman/listinfo/linux-audit
--
Serge Hallyn <serue(a)us.ibm.com>
2:47 p.m.
On Wednesday 15 December 2004 16:49, Serge Hallyn wrote:
New patch is attached.
Missing.
If there are no more issues, I will send it out to lkml tomorrow.
This was the only issue I had.
(My original thought was to wait until we discuss more in-depth
whether
to move to non-netlink interface for control, but really that discussion
would not be fair if we start from a known bad netlink implementation :)
Right, I am about ready to discuss this. I'll probably send out e-mails in the
morning with my thoughts on the subject. I am making the assumption that the
permcheck patch is accepted.
-Steve Grubb
4:05 p.m.
On Wed, 2004-12-15 at 15:47 -0500, Steve Grubb wrote:
On Wednesday 15 December 2004 16:49, Serge Hallyn wrote:
> New patch is attached.
Missing.
Confounded!
(hopefully now)
Right, I am about ready to discuss this. I'll probably send out
e-mails in the
morning with my thoughts on the subject. I am making the assumption that the
permcheck patch is accepted.
Ok, sounds good.
thanks,
-serge
--
Serge Hallyn <serue(a)us.ibm.com>
3:26 p.m.
On Wed, 2004-12-15 at 17:05, Serge Hallyn wrote:
<snip>
+static int cap_netlink_audit_check (struct sk_buff *skb)
+{
+ int msgtype = netlink_get_msgtype(skb);
+
+ switch(msgtype) {
+ case 0: /* not an audit msg */
+
+ case AUDIT_GET:
+ case AUDIT_LIST:
+ return 0;
+
+ case AUDIT_SET:
+ case AUDIT_USER:
+ case AUDIT_LOGIN:
+
+ case AUDIT_ADD:
+ case AUDIT_DEL:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ return 0;
+
+ default: /* permission denied: bad msg */
+ return msgtype;
+ }
<snip>
Shouldn't this function return -EPERM in the default case, not the
msgtype?
Also, do we truly need separate dummy and commoncap implementations, or
can capability re-use the dummy function (as long as it internally calls
the top-level capable function)? Or do you plan on changing that to not
use the top-level capable function?
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
4:26 p.m.
* Stephen Smalley (sds(a)epoch.ncsc.mil) wrote:
On Wed, 2004-12-15 at 17:05, Serge Hallyn wrote:
<snip>
+static int cap_netlink_audit_check (struct sk_buff *skb)
+{
+ int msgtype = netlink_get_msgtype(skb);
+
+ switch(msgtype) {
+ case 0: /* not an audit msg */
+
+ case AUDIT_GET:
+ case AUDIT_LIST:
+ return 0;
+
+ case AUDIT_SET:
+ case AUDIT_USER:
+ case AUDIT_LOGIN:
+
+ case AUDIT_ADD:
+ case AUDIT_DEL:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ return 0;
+
+ default: /* permission denied: bad msg */
+ return msgtype;
+ }
<snip>
Shouldn't this function return -EPERM in the default case, not the
msgtype?
Should be -EINVAL according to original code.
Also, do we truly need separate dummy and commoncap implementations,
or
can capability re-use the dummy function (as long as it internally calls
the top-level capable function)? Or do you plan on changing that to not
use the top-level capable function?
I really dislike duplicating code. I agree it should be put in a
central location. Does it really need to be broken out into the
security framework? Why not place it in audit itself?
Just a simple helper:
int audit_netlink_ok(struct nlmsghdr *nlh)
{
int err = -EINVAL;
if (audit_bad_header(nlh))
goto out;
err = 0;
switch() {
ok:
break;
capable:
if (!capable())
err = -EPERM;
break;
default:
err = -EINVAL;
break;
}
out:
return err;
}
audit_recieve_msg(skb, nlh)
{
...
err = audit_netlink_ok(nlh);
if (err)
return err;
...
}
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
4:45 p.m.
Quoting Chris Wright (chrisw(a)osdl.org):
> Shouldn't this function return -EPERM in the default case,
not the
> msgtype?
Should be -EINVAL according to original code.
Ok.
I really dislike duplicating code. I agree it should be put in a
central location. Does it really need to be broken out into the
security framework? Why not place it in audit itself?
Just a simple helper:
int audit_netlink_ok(struct nlmsghdr *nlh)
{
int err = -EINVAL;
if (audit_bad_header(nlh))
goto out;
err = 0;
switch() {
ok:
break;
capable:
if (!capable())
err = -EPERM;
break;
default:
err = -EINVAL;
break;
}
out:
return err;
}
The problem with this is that audit admin != sysadmin, so we
instantly preventing linux from achieving, say, MRMLOSPP. But
if we just replace "if (!capable()) err = -EPERM" with a new
lsm hook, then we can still consolidate some of the code in
audit_netlink_ok(nlh).
thoughts?
thanks,
-serge
4:47 p.m.
* Serge E. Hallyn (serue(a)us.ibm.com) wrote:
The problem with this is that audit admin != sysadmin, so we
instantly preventing linux from achieving, say, MRMLOSPP. But
if we just replace "if (!capable()) err = -EPERM" with a new
lsm hook, then we can still consolidate some of the code in
audit_netlink_ok(nlh).
thoughts?
CAP_SYS_AUDIT?
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
5:07 p.m.
* Chris Wright (chrisw(a)osdl.org) wrote:
CAP_SYS_AUDIT?
OK, well, Posix (withdrawn draft) specifies CAP_AUDIT_CONTROL and
CAP_AUDIT_WRITE.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
6:49 a.m.
On Wed, 2004-12-15 at 18:07, Chris Wright wrote:
* Chris Wright (chrisw(a)osdl.org) wrote:
> CAP_SYS_AUDIT?
OK, well, Posix (withdrawn draft) specifies CAP_AUDIT_CONTROL and
CAP_AUDIT_WRITE.
Given the shortage of available capability bits, I'd think you would
only want to take at most one for audit. You can always provide
finer-grained controls via other security modules, as in SELinux's
checking upon netlink_send.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
10:25 a.m.
Hi,
I believe a single CAP_AUDIT_CONTROL bit should suffice for defining an
MRMLOSPP-compliant audit role. I will send out a new patch asap which
also nixes cap_netlink_audit_send and just leaves the code in dummy.
Does this seem sufficient? Or do you (Chris) object to having this test
in the netlink send codepath? As far as I can see, the only legitimate
alternative would be to in fact move audit control to a different
(pseudo-fs?) interface.
thanks,
-serge
On Thu, 2004-12-16 at 07:49 -0500, Stephen Smalley wrote:
On Wed, 2004-12-15 at 18:07, Chris Wright wrote:
> * Chris Wright (chrisw(a)osdl.org) wrote:
> > CAP_SYS_AUDIT?
>
> OK, well, Posix (withdrawn draft) specifies CAP_AUDIT_CONTROL and
> CAP_AUDIT_WRITE.
Given the shortage of available capability bits, I'd think you would
only want to take at most one for audit. You can always provide
finer-grained controls via other security modules, as in SELinux's
checking upon netlink_send.
--
Serge Hallyn <serue(a)us.ibm.com>
9:29 a.m.
On Thu, 2004-12-16 at 11:25, Serge Hallyn wrote:
Hi,
I believe a single CAP_AUDIT_CONTROL bit should suffice for defining an
MRMLOSPP-compliant audit role. I will send out a new patch asap which
also nixes cap_netlink_audit_send and just leaves the code in dummy.
Does this seem sufficient? Or do you (Chris) object to having this test
in the netlink send codepath? As far as I can see, the only legitimate
alternative would be to in fact move audit control to a different
(pseudo-fs?) interface.
For just a capability check, you can check on the receive path based on
NETLINK_CREDS(skb)->eff_cap, as long as the security modules set all of
the capability bits in that field properly (commoncap already does so,
and SELinux and dummy could easily be changed to do so). In contrast,
we don't have that option for SELinux permissions, because we don't have
any way to convey either the sender security context or the computed
permissions in NETLINK_CREDS without extending that structure.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
10:55 a.m.
Well, the attached (compiled, but untested) patch does move the code
into audit.c the way Chris wanted, adding only one line to
dummy_netlink_send and cap_netlink_send. But it also would require some
changes to SELinux, I assume, to at least handle the new CAP_AUDIT bit.
By extending the NETLINK_CREDS(skb), I assume you mean adding a void
*security? Perhaps that's actually the cleaner way to go, if only
because it lets the actual receiving subsystem perform the check, rather
than netlink. That's the part I don't like about my patch. While in
this case it's just audit, that may change, and soon
security_netlink_send becomes a central location for checks for all
sorts of protocols...
So does extending NETLINK_CREDS seem (politically) feasible? Or does
the attached patch seem acceptable?
-serge
On Thu, 2004-12-16 at 10:29 -0500, Stephen Smalley wrote:
On Thu, 2004-12-16 at 11:25, Serge Hallyn wrote:
> Hi,
>
> I believe a single CAP_AUDIT_CONTROL bit should suffice for defining an
> MRMLOSPP-compliant audit role. I will send out a new patch asap which
> also nixes cap_netlink_audit_send and just leaves the code in dummy.
>
> Does this seem sufficient? Or do you (Chris) object to having this test
> in the netlink send codepath? As far as I can see, the only legitimate
> alternative would be to in fact move audit control to a different
> (pseudo-fs?) interface.
For just a capability check, you can check on the receive path based on
NETLINK_CREDS(skb)->eff_cap, as long as the security modules set all of
the capability bits in that field properly (commoncap already does so,
and SELinux and dummy could easily be changed to do so). In contrast,
we don't have that option for SELinux permissions, because we don't have
any way to convey either the sender security context or the computed
permissions in NETLINK_CREDS without extending that structure.
--
Serge Hallyn <serue(a)us.ibm.com>
10:03 a.m.
On Thu, 2004-12-16 at 11:55, Serge Hallyn wrote:
Well, the attached (compiled, but untested) patch does move the code
into audit.c the way Chris wanted, adding only one line to
dummy_netlink_send and cap_netlink_send. But it also would require some
changes to SELinux, I assume, to at least handle the new CAP_AUDIT bit.
Primarily just a policy change in this case, as the SELinux module just
uses CAP_TO_MASK() to convert the capability to an access vector.
Likely no impact on targeted policy, as unconfined_domain is allowed the
full capability set. Under strict, likely only need an update to
auditd.te; I think sysadm_t is already covered.
By extending the NETLINK_CREDS(skb), I assume you mean adding a void
*security? Perhaps that's actually the cleaner way to go, if only
because it lets the actual receiving subsystem perform the check, rather
than netlink. That's the part I don't like about my patch. While in
this case it's just audit, that may change, and soon
security_netlink_send becomes a central location for checks for all
sorts of protocols...
The problem is that if that security field is dynamically allocated,
then you have the standard lifecycle management issues, so you need
other hooks to ensure that it is properly freed. If we had a fixed size
field like the existing eff_cap field but for our own use, we could
compute an access vector based on the sender security context and store
it there for use by the receiving code. But that obviously won't do for
stacking.
So does extending NETLINK_CREDS seem (politically) feasible?
I doubt it, but one never knows. Note btw that the netlink patches
shouldn't just go to lkml; they need to be routed through the proper
maintainers (likely netdev and DaveM, might want to check where the
original LSM netlink patches were submitted).
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
1:15 p.m.
On Thu, 2004-12-16 at 11:03 -0500, Stephen Smalley wrote:
> By extending the NETLINK_CREDS(skb), I assume you mean adding a
void
> *security? Perhaps that's actually the cleaner way to go, if only
> because it lets the actual receiving subsystem perform the check, rather
> than netlink. That's the part I don't like about my patch. While in
> this case it's just audit, that may change, and soon
> security_netlink_send becomes a central location for checks for all
> sorts of protocols...
The problem is that if that security field is dynamically allocated,
then you have the standard lifecycle management issues, so you need
other hooks to ensure that it is properly freed. If we had a fixed size
Right, adding those hooks would take a pretty invasive patch, and given
that skb->security was rejected, we can't just hook into skb lifecycle
management.
Perhaps the last patch I sent out (with most code in audit.c, but hooked
into netlink_send security checks) should go to the maintainers you
listed for comments?
I will wait until tomorrow in case anyone feels we should be trying
something different first.
thanks,
-serge
--
Serge Hallyn <serue(a)us.ibm.com>
6:36 a.m.
On Wed, 2004-12-15 at 17:26, Chris Wright wrote:
I really dislike duplicating code. I agree it should be put in a
central location. Does it really need to be broken out into the
security framework? Why not place it in audit itself?
Just a simple helper:
int audit_netlink_ok(struct nlmsghdr *nlh)
{
int err = -EINVAL;
if (audit_bad_header(nlh))
goto out;
err = 0;
switch() {
ok:
break;
capable:
if (!capable())
err = -EPERM;
break;
default:
err = -EINVAL;
break;
}
out:
return err;
}
audit_recieve_msg(skb, nlh)
{
...
err = audit_netlink_ok(nlh);
if (err)
return err;
...
}
That is still on the receive path, where you can't use capable() because
it is based on current which isn't necessarily the same as the sender.
Now, it is true that one could replace the existing capable() checks in
the audit code with explicit checks of NETLINK_CREDS(skb)->eff_cap.
That would allow you to keep the checks in the audit receive-side code.
We would need to change selinux_netlink_send to set the entire
capability set rather than just CAP_NET_ADMIN, but that would be easy to
do.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
10:15 p.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
The attached patch implements the permissions check the way I think
Chris and Stephen were suggesting. It does not yet set the netlink
eff_caps in selinux_netlink_send(). Other than that, does this patch
seem reasonable? Is this preferable to the sender side check? Do
we want to add some audit read checks, and split CAP_AUDIT into two
or three capabilities?
thanks,
-serge
Quoting Stephen Smalley (sds(a)epoch.ncsc.mil):
On Wed, 2004-12-15 at 17:26, Chris Wright wrote:
> I really dislike duplicating code. I agree it should be put in a
> central location. Does it really need to be broken out into the
> security framework? Why not place it in audit itself?
>
> Just a simple helper:
>
> int audit_netlink_ok(struct nlmsghdr *nlh)
> {
> int err = -EINVAL;
>
> if (audit_bad_header(nlh))
> goto out;
>
> err = 0;
> switch() {
> ok:
> break;
> capable:
> if (!capable())
> err = -EPERM;
> break;
> default:
> err = -EINVAL;
> break;
> }
> out:
> return err;
> }
>
> audit_recieve_msg(skb, nlh)
> {
> ...
> err = audit_netlink_ok(nlh);
> if (err)
> return err;
> ...
> }
That is still on the receive path, where you can't use capable() because
it is based on current which isn't necessarily the same as the sender.
Now, it is true that one could replace the existing capable() checks in
the audit code with explicit checks of NETLINK_CREDS(skb)->eff_cap.
That would allow you to keep the checks in the audit receive-side code.
We would need to change selinux_netlink_send to set the entire
capability set rather than just CAP_NET_ADMIN, but that would be easy to
do.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
7:05 a.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
On Mon, 2004-12-20 at 23:15, Serge E. Hallyn wrote:
The attached patch implements the permissions check the way I think
Chris and Stephen were suggesting. It does not yet set the netlink
eff_caps in selinux_netlink_send(). Other than that, does this patch
seem reasonable? Is this preferable to the sender side check? Do
we want to add some audit read checks, and split CAP_AUDIT into two
or three capabilities?
The basic approach seems reasonable to me. The untested patch below
should change SELinux to fit with this approach; it calls the secondary
module to get the initial settings for the eff_cap field, then obtains
the SELinux allowed capability permissions and intersects them. For
generality and consistency, I would suggest computing the entire eff_cap
set in the dummy module as well, so that a receiver can check any
capability bit; you might be able to re-use the dummy_capget logic for
that purpose.
With regard to sender vs. receiver side checking, given that you are
only checking a capability and netlink already conveys the capability
set, there seems to be little reason to not keep the code on the
receiver side, and that keeps the audit code more encapsulated. SELinux
doesn't have that option, unfortunately, since netlink doesn't convey
SELinux SIDs.
With regard to a check on read-like operations, that does seem
desirable, as you don't want to let arbitrary processes list the audit
filters or get the audit daemon's pid.
Index: linux-2.6/security/selinux/hooks.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/hooks.c,v
retrieving revision 1.141
diff -u -p -r1.141 hooks.c
--- linux-2.6/security/selinux/hooks.c 6 Dec 2004 19:57:39 -0000 1.141
+++ linux-2.6/security/selinux/hooks.c 21 Dec 2004 12:50:46 -0000
@@ -3540,12 +3540,19 @@ static inline int selinux_nlmsg_perm(str
static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb)
{
- int err = 0;
+ struct task_security_struct *tsec;
+ struct av_decision avd;
+ int err;
- if (capable(CAP_NET_ADMIN))
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB(skb).eff_cap = 0;
+ err = secondary_ops->netlink_send(sk, skb);
+ if (err)
+ return err;
+
+ tsec = current->security;
+ err = avc_has_perm_noaudit(tsec->sid, tsec->sid, SECCLASS_CAPABILITY, ~0,
&avd);
+ if (err)
+ return err;
+ cap_mask(NETLINK_CB(skb).eff_cap, avd.allowed);
if (policydb_loaded_version >= POLICYDB_VERSION_NLCLASS)
err = selinux_nlmsg_perm(sk, skb);
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
7:20 a.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
On Tue, 2004-12-21 at 08:05, Stephen Smalley wrote:
With regard to a check on read-like operations, that does seem
desirable, as you don't want to let arbitrary processes list the audit
filters or get the audit daemon's pid.
Note btw that if you do end up with separate capabilities for audit read
vs. audit write operations, then you will have essentially duplicated
the SELinux nlmsg_read/nlmsg_write checking for netlink audit sockets,
and we could possibly drop the netlink audit socket case out of
selinux_nlmsg_lookup and not need to maintain the nlmsg_audit_perms
table. We would still need the checking for other kinds of netlink
sockets.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
10:28 a.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
The basic approach seems reasonable to me. The untested patch below
should change SELinux to fit with this approach; it calls the secondary
Thanks. The attached patch includes this patch, but I won't be able to
test the SELinux part until I get back to the office after next week. In
the meantime I'll also look into taking the audit checks out of
selinux_netlink_send() as you mentioned in the other email.
(And by then I'll be able to move back to a more modern kernel too)
-serge
--- linux-2.6.8.1/include/linux/capability.h 2004-08-14 05:55:09.000000000 -0500
+++ linux-2.6.8.1-audit/include/linux/capability.h 2004-12-23 13:19:02.000000000 -0600
@@ -284,6 +284,14 @@ typedef __u32 kernel_cap_t;
#define CAP_LEASE 28
+/* Allow reading of audit subsystem parameters */
+
+#define CAP_AUDIT_READ 29
+
+/* Allow control of audit subsystem */
+
+#define CAP_AUDIT_WRITE 30
+
#ifdef __KERNEL__
/*
* Bounding set
--- linux-2.6.8.1/include/linux/netlink.h 2004-08-14 05:56:00.000000000 -0500
+++ linux-2.6.8.1-audit/include/linux/netlink.h 2004-12-20 03:40:10.000000000 -0600
@@ -118,6 +118,7 @@ extern int netlink_attach(int unit, int
extern void netlink_detach(int unit);
extern int netlink_post(int unit, struct sk_buff *skb);
extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int
len));
+extern int netlink_get_msgtype(struct sk_buff *skb);
extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int
nonblock);
extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
--- linux-2.6.8.1/kernel/audit.c 2004-08-14 05:56:24.000000000 -0500
+++ linux-2.6.8.1-audit/kernel/audit.c 2004-12-23 13:30:17.000000000 -0600
@@ -300,15 +300,60 @@ nlmsg_failure: /* Used by NLMSG_PUT */
kfree_skb(skb);
}
+/*
+ * This function causes two checks on the incoming audit control
+ * message. 1. netlink_get_msgtype() will perform a length check,
+ * and 2. we check for CAP_AUDIT perms on a message which might
+ * change audit params.
+ */
+int audit_netlink_ok(struct sk_buff *skb)
+{
+ int err = 0;
+ int msgtype;
+
+ msgtype = netlink_get_msgtype(skb);
+
+ switch(msgtype) {
+ case 0: /* not an audit msg */
+
+ case AUDIT_GET:
+ case AUDIT_LIST:
+ if (!cap_raised(NETLINK_CB (skb).eff_cap,
+ CAP_AUDIT_READ))
+ err = -EPERM;
+ break;
+
+ case AUDIT_SET:
+ case AUDIT_USER:
+ case AUDIT_LOGIN:
+
+ case AUDIT_ADD:
+ case AUDIT_DEL:
+ if (!cap_raised(NETLINK_CB (skb).eff_cap,
+ CAP_AUDIT_WRITE))
+ err = -EPERM;
+ break;
+
+ default: /* permission denied: bad msg */
+ err = -EINVAL;
+ }
+
+ return err;
+}
+
static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
{
u32 uid, pid, seq;
void *data;
struct audit_status *status_get, status_set;
struct audit_login *login;
- int err = 0;
+ int err;
struct audit_buffer *ab;
+ err = audit_netlink_ok(skb);
+ if (err)
+ return err;
+
pid = NETLINK_CREDS(skb)->pid;
uid = NETLINK_CREDS(skb)->uid;
seq = nlh->nlmsg_seq;
@@ -327,8 +372,8 @@ static int audit_receive_msg(struct sk_b
&status_set, sizeof(status_set));
break;
case AUDIT_SET:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
+ if (nlh->nlmsg_len < sizeof(struct audit_status))
+ return -EINVAL;
status_get = (struct audit_status *)data;
if (status_get->mask & AUDIT_STATUS_ENABLED) {
err = audit_set_enabled(status_get->enabled);
@@ -364,8 +409,8 @@ static int audit_receive_msg(struct sk_b
audit_log_end(ab);
break;
case AUDIT_LOGIN:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
+ if (nlh->nlmsg_len < sizeof(struct audit_login))
+ return -EINVAL;
login = (struct audit_login *)data;
ab = audit_log_start(NULL);
if (ab) {
@@ -384,9 +429,12 @@ static int audit_receive_msg(struct sk_b
login->loginuid);
#endif
break;
- case AUDIT_LIST:
case AUDIT_ADD:
case AUDIT_DEL:
+ if (nlh->nlmsg_len < sizeof(struct audit_rule))
+ return -EINVAL;
+ /* fallthrough */
+ case AUDIT_LIST:
#ifdef CONFIG_AUDITSYSCALL
err = audit_receive_filter(nlh->nlmsg_type, pid, uid, seq,
data);
@@ -394,7 +442,7 @@ static int audit_receive_msg(struct sk_b
err = -EOPNOTSUPP;
#endif
break;
- default:
+ default: /* no longer needed... */
err = -EINVAL;
break;
}
--- linux-2.6.8.1/kernel/auditsc.c 2004-08-14 05:55:48.000000000 -0500
+++ linux-2.6.8.1-audit/kernel/auditsc.c 2004-12-20 03:40:10.000000000 -0600
@@ -250,8 +250,6 @@ int audit_receive_filter(int type, int p
audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0);
break;
case AUDIT_ADD:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
if (!(entry = kmalloc(sizeof(*entry), GFP_KERNEL)))
return -ENOMEM;
if (audit_copy_rule(&entry->rule, data)) {
--- linux-2.6.8.1/net/netlink/af_netlink.c 2004-08-14 05:55:32.000000000 -0500
+++ linux-2.6.8.1-audit/net/netlink/af_netlink.c 2004-12-20 03:40:11.000000000 -0600
@@ -389,6 +389,18 @@ static int netlink_connect(struct socket
return err;
}
+int netlink_get_msgtype(struct sk_buff *skb)
+{
+ struct nlmsghdr *nlh = (struct nlmsghdr *)skb->data;
+
+ if (skb->len < NLMSG_SPACE(0))
+ return -EINVAL;
+
+ if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len)
+ return -EINVAL;
+ return nlh->nlmsg_type;
+}
+
static int netlink_getname(struct socket *sock, struct sockaddr *addr, int *addr_len, int
peer)
{
struct sock *sk = sock->sk;
@@ -1218,6 +1230,7 @@ MODULE_LICENSE("GPL");
MODULE_ALIAS_NETPROTO(PF_NETLINK);
+EXPORT_SYMBOL(netlink_get_msgtype);
EXPORT_SYMBOL(netlink_ack);
EXPORT_SYMBOL(netlink_broadcast);
EXPORT_SYMBOL(netlink_broadcast_deliver);
--- linux-2.6.8.1/security/dummy.c 2004-08-14 05:54:51.000000000 -0500
+++ linux-2.6.8.1-audit/security/dummy.c 2004-12-22 03:01:50.000000000 -0600
@@ -722,10 +722,12 @@ static int dummy_sem_semop (struct sem_a
static int dummy_netlink_send (struct sock *sk, struct sk_buff *skb)
{
+ NETLINK_CB (skb).eff_cap = 0;
if (current->euid == 0)
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB (skb).eff_cap = 0;
+ NETLINK_CB (skb).eff_cap |= (~0 & ~CAP_TO_MASK(CAP_SETPCAP)
+ & ~CAP_FS_MASK);
+ if (current->fsuid == 0)
+ NETLINK_CB (skb).eff_cap |= CAP_FS_MASK;
return 0;
}
--- linux-2.6.8.1/security/selinux/hooks.c 2004-08-14 05:56:22.000000000 -0500
+++ linux-2.6.8.1-audit/security/selinux/hooks.c 2004-12-22 02:30:41.000000000 -0600
@@ -3585,12 +3585,21 @@ static inline int selinux_nlmsg_perm(str
static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb)
{
- int err = 0;
+ struct task_security_struct *tsec;
+ struct av_decision avd;
+ int err;
- if (capable(CAP_NET_ADMIN))
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB(skb).eff_cap = 0;
+ err = secondary_ops->netlink_send(sk, skb);
+ if (err)
+ return err;
+
+ tsec = current->security;
+ err = avc_has_perm_noaudit(tsec->sid, tsec->sid, SECCLASS_CAPABILITY,
+ ~0, &avd);
+ if (err)
+ return 0;
+
+ cap_mask(NETLINK_CB(skb).eff_cap, avd.allowed);
if (policydb_loaded_version >= POLICYDB_VERSION_NLCLASS)
err = selinux_nlmsg_perm(sk, skb);
8:03 a.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
On Fri, 2004-12-24 at 11:28, Serge E. Hallyn wrote:
> The basic approach seems reasonable to me. The untested patch
below
> should change SELinux to fit with this approach; it calls the secondary
Thanks. The attached patch includes this patch, but I won't be able to
test the SELinux part until I get back to the office after next week. In
the meantime I'll also look into taking the audit checks out of
selinux_netlink_send() as you mentioned in the other email.
I'd hold off on removing the SELinux audit checks for now, even if they
are redundant. I think we'll need to phase them out gradually while
adjusting policy at the same time.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
8:48 a.m.
New subject: Audit perms check on recv (Re: Two netlink patches)
On Fri, 2004-12-24 at 11:28, Serge E. Hallyn wrote:
--- linux-2.6.8.1/security/dummy.c 2004-08-14 05:54:51.000000000
-0500
+++ linux-2.6.8.1-audit/security/dummy.c 2004-12-22 03:01:50.000000000 -0600
@@ -722,10 +722,12 @@ static int dummy_sem_semop (struct sem_a
static int dummy_netlink_send (struct sock *sk, struct sk_buff *skb)
{
+ NETLINK_CB (skb).eff_cap = 0;
if (current->euid == 0)
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB (skb).eff_cap = 0;
+ NETLINK_CB (skb).eff_cap |= (~0 & ~CAP_TO_MASK(CAP_SETPCAP)
+ & ~CAP_FS_MASK);
+ if (current->fsuid == 0)
+ NETLINK_CB (skb).eff_cap |= CAP_FS_MASK;
return 0;
}
Is this preferable to calling dummy_capget()? That requires fake
inheritable and permitted capability sets (or changing dummy_capget to
not set them if NULL), but avoids duplicated logic.
+ tsec = current->security;
+ err = avc_has_perm_noaudit(tsec->sid, tsec->sid, SECCLASS_CAPABILITY,
+ ~0, &avd);
+ if (err)
+ return 0;
This doesn't seem right to me (will return success without masking out
capabilities denied by SELinux if any capabilitites are denied), but on
second glance, neither does my patch (will return failure if any
capabilities are denied). Correct options would seem to be:
1) Call security_compute_av() instead to compute the access vector,
which returns 0 unless an input argument is invalid and just sets the
access vectors, or
2) Set avd.allowed to 0 prior to the call to avc_has_perm_noaudit(),
then ignore the return value of avc_has_perm_noaudit(), just cast it to
void.
The former is cleaner, but loses the benefit of cached decisions.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
4:25 p.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
The attached patch addresses Stephen's comments about re-using
dummy_capget code and properly checking capabilities in
selinux_netlink_send.
To review, it
1. adds two new capabilities, CAP_AUDIT_READ and CAP_AUDIT_WRITE
2. changes dummy.c and selinux/hooks.c to set the
netlink_audit message's eff_cap field to accurately reflect
the capabilities of the sender,
3. checks for the capabilities required for the requested audit
action in the eff_cap field at audit_msg_recv
4. adds checks to ensure that the message lengths are at least
long enough to hold the structures they claim to hold.
thanks,
-serge
Index: linux-2.6.10-rc3-mm1/include/linux/capability.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/capability.h 2004-10-18 16:53:44.000000000
-0500
+++ linux-2.6.10-rc3-mm1/include/linux/capability.h 2004-12-27 11:27:11.000000000 -0600
@@ -284,6 +284,14 @@ typedef __u32 kernel_cap_t;
#define CAP_LEASE 28
+/* Allow reading of audit subsystem parameters */
+
+#define CAP_AUDIT_READ 29
+
+/* Allow control of audit subsystem */
+
+#define CAP_AUDIT_WRITE 30
+
#ifdef __KERNEL__
/*
* Bounding set
Index: linux-2.6.10-rc3-mm1/include/linux/netlink.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/netlink.h 2004-12-27 11:24:14.000000000 -0600
+++ linux-2.6.10-rc3-mm1/include/linux/netlink.h 2004-12-27 11:27:11.000000000 -0600
@@ -120,6 +120,7 @@ extern int netlink_attach(int unit, int
extern void netlink_detach(int unit);
extern int netlink_post(int unit, struct sk_buff *skb);
extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int
len));
+extern int netlink_get_msgtype(struct sk_buff *skb);
extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int
nonblock);
extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
Index: linux-2.6.10-rc3-mm1/kernel/audit.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/kernel/audit.c 2004-12-27 11:24:40.000000000 -0600
+++ linux-2.6.10-rc3-mm1/kernel/audit.c 2004-12-27 11:27:11.000000000 -0600
@@ -300,15 +300,60 @@ nlmsg_failure: /* Used by NLMSG_PUT */
kfree_skb(skb);
}
+/*
+ * This function causes two checks on the incoming audit control
+ * message. 1. netlink_get_msgtype() will perform a length check,
+ * and 2. we check for CAP_AUDIT perms on a message which might
+ * change audit params.
+ */
+int audit_netlink_ok(struct sk_buff *skb)
+{
+ int err = 0;
+ int msgtype;
+
+ msgtype = netlink_get_msgtype(skb);
+
+ switch(msgtype) {
+ case 0: /* not an audit msg */
+
+ case AUDIT_GET:
+ case AUDIT_LIST:
+ if (!cap_raised(NETLINK_CB (skb).eff_cap,
+ CAP_AUDIT_READ))
+ err = -EPERM;
+ break;
+
+ case AUDIT_SET:
+ case AUDIT_USER:
+ case AUDIT_LOGIN:
+
+ case AUDIT_ADD:
+ case AUDIT_DEL:
+ if (!cap_raised(NETLINK_CB (skb).eff_cap,
+ CAP_AUDIT_WRITE))
+ err = -EPERM;
+ break;
+
+ default: /* permission denied: bad msg */
+ err = -EINVAL;
+ }
+
+ return err;
+}
+
static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
{
u32 uid, pid, seq;
void *data;
struct audit_status *status_get, status_set;
struct audit_login *login;
- int err = 0;
+ int err;
struct audit_buffer *ab;
+ err = audit_netlink_ok(skb);
+ if (err)
+ return err;
+
pid = NETLINK_CREDS(skb)->pid;
uid = NETLINK_CREDS(skb)->uid;
seq = nlh->nlmsg_seq;
@@ -327,8 +372,8 @@ static int audit_receive_msg(struct sk_b
&status_set, sizeof(status_set));
break;
case AUDIT_SET:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
+ if (nlh->nlmsg_len < sizeof(struct audit_status))
+ return -EINVAL;
status_get = (struct audit_status *)data;
if (status_get->mask & AUDIT_STATUS_ENABLED) {
err = audit_set_enabled(status_get->enabled);
@@ -364,8 +409,8 @@ static int audit_receive_msg(struct sk_b
audit_log_end(ab);
break;
case AUDIT_LOGIN:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
+ if (nlh->nlmsg_len < sizeof(struct audit_login))
+ return -EINVAL;
login = (struct audit_login *)data;
ab = audit_log_start(NULL);
if (ab) {
@@ -384,9 +429,12 @@ static int audit_receive_msg(struct sk_b
login->loginuid);
#endif
break;
- case AUDIT_LIST:
case AUDIT_ADD:
case AUDIT_DEL:
+ if (nlh->nlmsg_len < sizeof(struct audit_rule))
+ return -EINVAL;
+ /* fallthrough */
+ case AUDIT_LIST:
#ifdef CONFIG_AUDITSYSCALL
err = audit_receive_filter(nlh->nlmsg_type, pid, uid, seq,
data);
@@ -394,7 +442,7 @@ static int audit_receive_msg(struct sk_b
err = -EOPNOTSUPP;
#endif
break;
- default:
+ default: /* no longer needed... */
err = -EINVAL;
break;
}
Index: linux-2.6.10-rc3-mm1/kernel/auditsc.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/kernel/auditsc.c 2004-10-18 16:54:54.000000000 -0500
+++ linux-2.6.10-rc3-mm1/kernel/auditsc.c 2004-12-27 11:27:11.000000000 -0600
@@ -250,8 +250,6 @@ int audit_receive_filter(int type, int p
audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0);
break;
case AUDIT_ADD:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
if (!(entry = kmalloc(sizeof(*entry), GFP_KERNEL)))
return -ENOMEM;
if (audit_copy_rule(&entry->rule, data)) {
Index: linux-2.6.10-rc3-mm1/net/netlink/af_netlink.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/net/netlink/af_netlink.c 2004-12-27 11:24:17.000000000
-0600
+++ linux-2.6.10-rc3-mm1/net/netlink/af_netlink.c 2004-12-27 11:27:11.000000000 -0600
@@ -518,6 +518,18 @@ static int netlink_connect(struct socket
return err;
}
+int netlink_get_msgtype(struct sk_buff *skb)
+{
+ struct nlmsghdr *nlh = (struct nlmsghdr *)skb->data;
+
+ if (skb->len < NLMSG_SPACE(0))
+ return -EINVAL;
+
+ if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len)
+ return -EINVAL;
+ return nlh->nlmsg_type;
+}
+
static int netlink_getname(struct socket *sock, struct sockaddr *addr, int *addr_len, int
peer)
{
struct sock *sk = sock->sk;
@@ -1511,6 +1523,7 @@ MODULE_LICENSE("GPL");
MODULE_ALIAS_NETPROTO(PF_NETLINK);
+EXPORT_SYMBOL(netlink_get_msgtype);
EXPORT_SYMBOL(netlink_ack);
EXPORT_SYMBOL(netlink_broadcast);
EXPORT_SYMBOL(netlink_dump_start);
Index: linux-2.6.10-rc3-mm1/security/dummy.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/dummy.c 2004-12-27 11:24:40.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/dummy.c 2004-12-27 17:24:10.000000000 -0600
@@ -741,10 +741,11 @@ static int dummy_sem_semop (struct sem_a
static int dummy_netlink_send (struct sock *sk, struct sk_buff *skb)
{
- if (current->euid == 0)
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB (skb).eff_cap = 0;
+ kernel_cap_t dummy_cap, eff_cap;
+
+ dummy_capget(current, &eff_cap, &dummy_cap, &dummy_cap);
+ NETLINK_CB (skb).eff_cap = eff_cap;
+
return 0;
}
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c 2004-12-27 11:24:40.000000000
-0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c 2004-12-27 17:17:29.000000000 -0600
@@ -3564,12 +3564,19 @@ static inline int selinux_nlmsg_perm(str
static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb)
{
- int err = 0;
+ struct task_security_struct *tsec;
+ struct av_decision avd;
+ int err;
- if (capable(CAP_NET_ADMIN))
- cap_raise (NETLINK_CB (skb).eff_cap, CAP_NET_ADMIN);
- else
- NETLINK_CB(skb).eff_cap = 0;
+ err = secondary_ops->netlink_send(sk, skb);
+ if (err)
+ return err;
+
+ tsec = current->security;
+
+ security_compute_av(tsec->sid, tsec->sid, SECCLASS_CAPABILITY, ~0,
+ &avd);
+ cap_mask(NETLINK_CB(skb).eff_cap, avd.allowed);
if (policydb_loaded_version >= POLICYDB_VERSION_NLCLASS)
err = selinux_nlmsg_perm(sk, skb);
6:58 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
On Mon, 2004-12-27 at 17:25, Serge E. Hallyn wrote:
+ security_compute_av(tsec->sid, tsec->sid,
SECCLASS_CAPABILITY, ~0,
+ &avd);
+ cap_mask(NETLINK_CB(skb).eff_cap, avd.allowed);
Sorry, on second thought, we likely don't want the performance and
locking overhead of security_compute_av() imposed on all netlink sends,
so I'd suggest the diff below relative to your patch to switch back to
using avc_has_perm_noaudit(), but clearing avd.allowed first to ensure a
well-defined value even upon error return and casting to void to avoid a
warning about ignoring the return value.
--- linux-2.6/security/selinux/hooks.c.orig 2004-12-28 07:55:06.526688392 -0500
+++ linux-2.6/security/selinux/hooks.c 2004-12-28 07:56:09.234155408 -0500
@@ -3551,8 +3551,9 @@ static int selinux_netlink_send(struct s
tsec = current->security;
- security_compute_av(tsec->sid, tsec->sid, SECCLASS_CAPABILITY, ~0,
- &avd);
+ avd.allowed = 0;
+ (void)avc_has_perm_noaudit(tsec->sid, tsec->sid,
+ SECCLASS_CAPABILITY, ~0, &avd);
cap_mask(NETLINK_CB(skb).eff_cap, avd.allowed);
if (policydb_loaded_version >= POLICYDB_VERSION_NLCLASS)
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
3:51 p.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
Serge E. Hallyn wrote:
The attached patch addresses Stephen's comments about re-using
dummy_capget code and properly checking capabilities in
selinux_netlink_send.
To review, it
1. adds two new capabilities, CAP_AUDIT_READ and CAP_AUDIT_WRITE
2. changes dummy.c and selinux/hooks.c to set the
netlink_audit message's eff_cap field to accurately reflect
the capabilities of the sender,
3. checks for the capabilities required for the requested audit
action in the eff_cap field at audit_msg_recv
4. adds checks to ensure that the message lengths are at least
long enough to hold the structures they claim to hold.
thanks,
-serge
It would seem that separate CAP_AUDIT_ADMIN/CAP_AUDIT_WRITE capabilities are
much more important than having a separate CAP_ADMIN_READ capability. The
CAP_AUDIT_WRITE capability should only allow a process to generate a userspace
audit message. I do not think we should impose a trust equivalence for programs
that can generate an audit message and programs that can modify the audit
subsystem configuration.
I think capability checks should map like this:
AUDIT_GET -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_SET -> CAP_AUDIT_ADMIN
AUDIT_LIST -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_ADD -> CAP_AUDIT_ADMIN
AUDIT_DEL -> CAP_AUDIT_ADMIN
AUDIT_USER -> CAP_AUDIT_WRITE
AUDIT_LOGIN -> CAP_AUDIT_ADMIN
The case of AUDIT_LOGIN has merit for a separate CAP_AUDIT_LOGIN capability
because this carries much more importance than AUDIT_USER, but we really should
not have the ability to mess with the rest of the configuration. However, this
action is as important to the reliability of the audit logs as the configuration
of the audit subsystem. I would prioritize this capability above CAP_AUDIT_READ
as well.
--
Darrel
3:58 p.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
On Tue, 2005-01-04 at 16:51, Darrel Goeddel wrote:
It would seem that separate CAP_AUDIT_ADMIN/CAP_AUDIT_WRITE
capabilities are
much more important than having a separate CAP_ADMIN_READ capability. The
CAP_AUDIT_WRITE capability should only allow a process to generate a userspace
audit message. I do not think we should impose a trust equivalence for programs
that can generate an audit message and programs that can modify the audit
subsystem configuration.
I think capability checks should map like this:
AUDIT_GET -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_SET -> CAP_AUDIT_ADMIN
AUDIT_LIST -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_ADD -> CAP_AUDIT_ADMIN
AUDIT_DEL -> CAP_AUDIT_ADMIN
AUDIT_USER -> CAP_AUDIT_WRITE
AUDIT_LOGIN -> CAP_AUDIT_ADMIN
The case of AUDIT_LOGIN has merit for a separate CAP_AUDIT_LOGIN capability
because this carries much more importance than AUDIT_USER, but we really should
not have the ability to mess with the rest of the configuration. However, this
action is as important to the reliability of the audit logs as the configuration
of the audit subsystem. I would prioritize this capability above CAP_AUDIT_READ
as well.
Interesting points, but capability bit space is _very_ limited, so I
think we have to be conservative about distinctions there. Finer-grained
distinctions could be easily accomodated via SELinux permission checks,
as you can always add a new class specifically for this purpose, but has
to be checked from the netlink_send hook.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
4:01 p.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
* Darrel Goeddel (dgoeddel(a)trustedcs.com) wrote:
Serge E. Hallyn wrote:
<snip>
> To review, it
>
> 1. adds two new capabilities, CAP_AUDIT_READ and CAP_AUDIT_WRITE
<snip>
It would seem that separate CAP_AUDIT_ADMIN/CAP_AUDIT_WRITE
capabilities are
much more important than having a separate CAP_ADMIN_READ capability. The
I already suggested CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE for a good
reason. These are documented by the Posix draft defining capabilities.
I see no good reason to stray from that.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
11:27 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
I'm sorry, I thought that by "we are already way off spec" you were
saying we shouldn't bother trying to follow the spec.
I'll come back with a new patch after I go read the draft, because the
meaning of CAP_AUDIT_CONTROL is not clear to me.
-serge
On Tue, 2005-01-04 at 14:01 -0800, Chris Wright wrote:
* Darrel Goeddel (dgoeddel(a)trustedcs.com) wrote:
> Serge E. Hallyn wrote:
<snip>
> > To review, it
> >
> > 1. adds two new capabilities, CAP_AUDIT_READ and CAP_AUDIT_WRITE
<snip>
> It would seem that separate CAP_AUDIT_ADMIN/CAP_AUDIT_WRITE capabilities are
> much more important than having a separate CAP_ADMIN_READ capability. The
I already suggested CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE for a good
reason. These are documented by the Posix draft defining capabilities.
I see no good reason to stray from that.
thanks,
-chris
--
Serge Hallyn <serue(a)us.ibm.com>
10:47 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
--- Serge Hallyn <serue(a)us.ibm.com> wrote:
I'm sorry, I thought that by "we are already way off
spec" you were
saying we shouldn't bother trying to follow the
spec.
Well, there's the capability spec, which Linux has
tried to keep with (pretty much) and the audit spec,
which seems to be a different kettle of fish.
I'll come back with a new patch after I go read the
draft, because the
meaning of CAP_AUDIT_CONTROL is not clear to me.
CAP_AUDIT_CONTROL is intended to be used for all
audit trail operations requiring privilege except
for application writing records to the audit trail.
=====
Casey Schaufler
casey(a)schaufler-ca.com
__________________________________
Do you Yahoo!?
Read only the mail you want - Yahoo! Mail SpamGuard.
http://promotions.yahoo.com/new_mail
11:25 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
* Serge Hallyn (serue(a)us.ibm.com) wrote:
I'm sorry, I thought that by "we are already way off
spec" you were
saying we shouldn't bother trying to follow the spec.
Ah, sorry. I meant we were way off spec already, but no need to add new
bits that are off spec if they are already specified in the draft.
I'll come back with a new patch after I go read the draft,
because the
meaning of CAP_AUDIT_CONTROL is not clear to me.
CAP_AUDIT_CONTROL is what you'd think of if it were CAP_AUDIT_ADMIN. It
means you can control the auditing subsytem (turn it on/off, etc).
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
10:40 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
Hi,
So to be clear, are the following associations correct?
AUDIT_GET: no capability
AUDIT_LIST: no capability
AUDIT_USER: CAP_AUDIT_WRITE
AUDIT_LOGIN: CAP_AUDIT_WRITE
AUDIT_SET: CAP_AUDIT_CONTROL
AUDIT_ADD: CAP_AUDIT_CONTROL
AUDIT_DEL: CAP_AUDIT_CONTROL
thanks,
-serge
On Wed, 2005-01-05 at 09:25 -0800, Chris Wright wrote:
* Serge Hallyn (serue(a)us.ibm.com) wrote:
> I'm sorry, I thought that by "we are already way off spec" you were
> saying we shouldn't bother trying to follow the spec.
Ah, sorry. I meant we were way off spec already, but no need to add new
bits that are off spec if they are already specified in the draft.
> I'll come back with a new patch after I go read the draft, because the
> meaning of CAP_AUDIT_CONTROL is not clear to me.
CAP_AUDIT_CONTROL is what you'd think of if it were CAP_AUDIT_ADMIN. It
means you can control the auditing subsytem (turn it on/off, etc).
thanks,
-chris
--
Serge Hallyn <serue(a)us.ibm.com>
9:38 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
On Thu, 2005-01-06 at 11:40, Serge Hallyn wrote:
Hi,
So to be clear, are the following associations correct?
AUDIT_GET: no capability
AUDIT_LIST: no capability
AUDIT_USER: CAP_AUDIT_WRITE
AUDIT_LOGIN: CAP_AUDIT_WRITE
AUDIT_SET: CAP_AUDIT_CONTROL
AUDIT_ADD: CAP_AUDIT_CONTROL
AUDIT_DEL: CAP_AUDIT_CONTROL
I actually got the impression (possibly wrong) from Casey's posting that
the desired associations were CAP_AUDIT_WRITE for AUDIT_USER only, and
CAP_AUDIT_CONTROL for all other operations, even AUDIT_GET and
AUDIT_LIST (and AUDIT_LOGIN). That allows applications to write records
to the audit trail without any other access. Of course, it means that
login would be able to arbitrarily control auditing, since it needs
AUDIT_LOGIN.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
10:11 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
Stephen Smalley wrote:
On Thu, 2005-01-06 at 11:40, Serge Hallyn wrote:
>Hi,
>
>So to be clear, are the following associations correct?
>
>AUDIT_GET: no capability
>AUDIT_LIST: no capability
>AUDIT_USER: CAP_AUDIT_WRITE
>AUDIT_LOGIN: CAP_AUDIT_WRITE
>AUDIT_SET: CAP_AUDIT_CONTROL
>AUDIT_ADD: CAP_AUDIT_CONTROL
>AUDIT_DEL: CAP_AUDIT_CONTROL
I actually got the impression (possibly wrong) from Casey's posting that
the desired associations were CAP_AUDIT_WRITE for AUDIT_USER only, and
CAP_AUDIT_CONTROL for all other operations, even AUDIT_GET and
AUDIT_LIST (and AUDIT_LOGIN). That allows applications to write records
to the audit trail without any other access. Of course, it means that
login would be able to arbitrarily control auditing, since it needs
AUDIT_LOGIN.
I agree with Stephen's assessment. AUDIT_LOGIN needs to be in a separate
"class" from AUDIT_USER because it controls the reliability of audit records by
setting the login id. SELinux will be great to further restrict the actions of
a process requiring CAP_AUDIT_CONTROL just to use AUDIT_LOGIN :)
--
Darrel
10:21 a.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
--- Stephen Smalley <sds(a)epoch.ncsc.mil> wrote:
I actually got the impression (possibly wrong) from
Casey's posting that
the desired associations were CAP_AUDIT_WRITE for
AUDIT_USER only, and
CAP_AUDIT_CONTROL for all other operations, even
AUDIT_GET and
AUDIT_LIST (and AUDIT_LOGIN).
This is correct.
That allows
applications to write records
to the audit trail without any other access.
This is correct.
Of
course, it means that
login would be able to arbitrarily control auditing,
since it needs
AUDIT_LOGIN.
Login is a critical component in the system
security policy enforcement. It can be expected
to undergo sufficient analysis and review to
ensure that abuse of the audit system is unlikely.
=====
Casey Schaufler
casey(a)schaufler-ca.com
__________________________________
Do you Yahoo!?
Meet the all-new My Yahoo! - Try it today!
http://my.yahoo.com
4:16 p.m.
New subject: New audit-perms patch [ Re: Audit perms check on recv ]
--- Darrel Goeddel <dgoeddel(a)trustedcs.com> wrote:
Serge E. Hallyn wrote:
> The attached patch addresses Stephen's comments
about re-using
> dummy_capget code and properly checking
capabilities in
> selinux_netlink_send.
>
> To review, it
>
> 1. adds two new capabilities, CAP_AUDIT_READ
and CAP_AUDIT_WRITE
> ...
It would seem that separate
CAP_AUDIT_ADMIN/CAP_AUDIT_WRITE capabilities are
much more important than having a separate
CAP_ADMIN_READ capability.
The POSIX Draft uses CAP_AUDIT_WRITE and
CAP_AUDIT_CONTROL, with the later required for
reading records. Irix does the same. A
capability for reading audit records seperate
from that required to disable their generation,
the logic went, is hardly necessary.
The
CAP_AUDIT_WRITE capability should only allow a
process to generate a userspace
audit message.
This is consistant with POSIX.
I think capability checks should map like this:
AUDIT_GET -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_SET -> CAP_AUDIT_ADMIN
AUDIT_LIST -> none (possibly CAP_AUDIT_ADMIN)
AUDIT_ADD -> CAP_AUDIT_ADMIN
AUDIT_DEL -> CAP_AUDIT_ADMIN
AUDIT_USER -> CAP_AUDIT_WRITE
AUDIT_LOGIN -> CAP_AUDIT_ADMIN
The case of AUDIT_LOGIN has merit for a separate
CAP_AUDIT_LOGIN capability
because this carries much more importance than
AUDIT_USER, but we really should
not have the ability to mess with the rest of the
configuration. However, this
action is as important to the reliability of the
audit logs as the configuration
of the audit subsystem. I would prioritize this
capability above CAP_AUDIT_READ
as well.
The granularity of capabilities should be carefully
policed. Data General had over 330 in DGUX. If it
is at all possible to get by with two, that would be
best.
=====
Casey Schaufler
casey(a)schaufler-ca.com
__________________________________
Do you Yahoo!?
Take Yahoo! Mail with you! Get it on your mobile phone.
http://mobile.yahoo.com/maildemo
4:42 p.m.
Quoting Stephen Smalley (sds(a)epoch.ncsc.mil):
On Wed, 2004-12-15 at 17:05, Serge Hallyn wrote:
<snip>
+static int cap_netlink_audit_check (struct sk_buff *skb)
+{
+ int msgtype = netlink_get_msgtype(skb);
+
+ switch(msgtype) {
+ case 0: /* not an audit msg */
+
+ case AUDIT_GET:
+ case AUDIT_LIST:
+ return 0;
+
+ case AUDIT_SET:
+ case AUDIT_USER:
+ case AUDIT_LOGIN:
+
+ case AUDIT_ADD:
+ case AUDIT_DEL:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ return 0;
+
+ default: /* permission denied: bad msg */
+ return msgtype;
+ }
<snip>
Shouldn't this function return -EPERM in the default case, not the
msgtype?
Yes it should, thanks.
Also, do we truly need separate dummy and commoncap implementations,
or
can capability re-use the dummy function (as long as it internally calls
the top-level capable function)? Or do you plan on changing that to not
use the top-level capable function?
I wasn't. Certainly from a capability.ko point of view we would want
PF_SUPERPRIV set if an AUDIT_ADD is done. On the other hand, asking all
security modules to authorize CAP_SYS_ADMIN for the audit role seems
misguided if we eventually want to create a separate audit role.
-serge
4:45 p.m.
* Serge E. Hallyn (serue(a)us.ibm.com) wrote:
I wasn't. Certainly from a capability.ko point of view we would
want
PF_SUPERPRIV set if an AUDIT_ADD is done. On the other hand, asking all
security modules to authorize CAP_SYS_ADMIN for the audit role seems
misguided if we eventually want to create a separate audit role.
role or capability? if latter, yes, we do. CAP_SYS_ADMIN is an
abomination ;-)
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
6:31 a.m.
On Wed, 2004-12-15 at 12:20, Serge Hallyn wrote:
Is there any objection to my sending the two netlink patches I
recently
sent out to lkml? Just to refresh memory, the one (audit-fix-
permchecks.diff) adds some message length checks and moves audit control
message authorization to netlink message send, while the other (audit-
loginuid.patch) changes the SET_LOGINUID behavior to set loginuid for
the sending process (as expected) rather than whichever process happens
to end up handling the message.
I'm still a bit concerned by the netlink autobind case. Two points:
1) Why reset pid to 0 and then proceed to find_task_by_pid rather than
failing immediately?
2) Won't this break the common usage of netlink by applications? I
think that we had to change libselinux to fall back on autobinding of
the netlink selinux socket because we were otherwise encountering
EADDRINUSE errors upon restarting a program due to deferred release of
the slot.
--
Stephen Smalley <sds(a)epoch.ncsc.mil>
National Security Agency
7289
days inactive
7311
days old
linux-audit@lists.linux-audit.osci.io
44 comments
9 participants
participants (9)
-
Casey Schaufler -
Chris Wright -
Darrel Goeddel -
Peter Martuccelli -
Serge E. Hallyn -
Serge Hallyn
-
Stephen Smalley -
Steve Grubb -
Timothy R. Chavez