[PATCH 5/6] audit: add restricted capability read-only netlink multicast socket

[Richard Guy Briggs]

Add a netlink multicast socket with one group to kaudit for best-effort
delivery to read-only userspace clients such as systemd, in addition to the
existing bidirectional unicast auditd userspace client.

Currently, auditd is intended to use the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE
capabilities, but actually uses CAP_NET_ADMIN.  The CAP_AUDIT_READ capability
is added for use by read-only AUDIT_NLGRP_READLOG netlink multicast group
clients to the kaudit subsystem.

This will safely give access to services such as systemd to consume audit logs
while ensuring write access remains restricted for integrity.

Signed-off-by: Richard Guy Briggs rbri...@redhat.com
---

(The seemingly wasteful skb_copy() is necessary because the original kaudit
unicast socket sends up messages with nlmsg_len set to the payload length
rather than the entire message length.  This breaks the convention used by
netlink.  The existing auditd daemon assumes this breakage.  Fixing this would
require co-ordinating a change in the established protocol between kaudit
kernel code and auditd userspace code.  There is no reason for new multicast
clients to continue with this breakage.)

 include/uapi/linux/audit.h  |  8 
 include/uapi/linux/capability.h |  5 -
 kernel/audit.c  | 40 +
 security/selinux/include/classmap.h |  2 +-
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..6296e5d9 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -357,6 +357,14 @@ enum {
 #define AUDIT_PERM_READ4
 #define AUDIT_PERM_ATTR8
 
+/* Multicast Netlink socket groups (default up to 32) */
+enum audit_nlgrps {
+   AUDIT_NLGRP_NONE,   /* Group 0 not used */
+   AUDIT_NLGRP_READLOG,/* best effort read only socket */
+   __AUDIT_NLGRP_MAX
+};
+#define AUDIT_NLGRP_MAX(__AUDIT_NLGRP_MAX - 1)
+
 struct audit_status {
__u32   mask;   /* Bit mask for valid entries */
__u32   enabled;/* 1 = enabled, 0 = disabled */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index ba478fa..f579a06 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -343,7 +343,10 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND36
 
-#define CAP_LAST_CAP CAP_BLOCK_SUSPEND
+/* Allowed to read the audit log */
+#define CAP_AUDIT_READ 37
+
+#define CAP_LAST_CAP CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) = 0  (x) = CAP_LAST_CAP)
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 02a5d9e..9eef05b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -418,6 +418,37 @@ static void kauditd_send_skb(struct sk_buff *skb)
 }
 
 /*
+ * kauditd_send_multicast_skb - send the skb to multicast userspace listeners
+ *
+ * This function doesn't consume an skb as might be expected since it has to
+ * copy it anyways.
+ */
+static void kauditd_send_multicast_skb(struct sk_buff *skb)
+{
+   struct sk_buff *copy;
+   struct nlmsghdr *nlh;
+
+   /*
+* The seemingly wasteful skb_copy() is necessary because the original
+* kaudit unicast socket sends up messages with nlmsg_len set to the
+* payload length rather than the entire message length.  This breaks
+* the standard set by netlink.  The existing auditd daemon assumes
+* this breakage.  Fixing this would require co-ordinating a change in
+* the established protocol between the kaudit kernel subsystem and
+* the auditd userspace code.  There is no reason for new multicast
+* clients to continue with this non-compliance.
+*/
+   copy = skb_copy(skb, GFP_KERNEL);
+   if (!copy)
+   return;
+
+   nlh = nlmsg_hdr(copy);
+   nlh-nlmsg_len = copy-len;
+
+   nlmsg_multicast(audit_sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
+}
+
+/*
  * flush_hold_queue - empty the hold queue if auditd appears
  *
  * If auditd just started, drain the queue of messages already
@@ -468,6 +499,12 @@ static int kauditd_thread(void *dummy)
skb = skb_dequeue(audit_skb_queue);
wake_up(audit_backlog_wait);
if (skb) {
+   /* Don't bump skb refcount for multicast send since
+* kauditd_send_multicast_skb() copies the skb anyway
+* due to audit unicast netlink protocol
+* non-compliance.
+*/
+   kauditd_send_multicast_skb(skb);
if (audit_pid)
kauditd_send_skb(skb);
else
@@ -951,6 +988,9 @@ static int __init audit_init(void)
int i;
struct netlink_kernel_cfg cfg = {
.input  = audit_receive

[PATCH 0/6] audit: add restricted capability read-only netlink multicast socket

[Richard Guy Briggs]

Hi,

This is a patch set Eric Paris and I have been working on to add a restricted
capability read-only netlink multicast socket to kaudit to enable 
userspace clients such as systemd to consume audit logs, in addition to the
bidirectional auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(bot uses CAP_NET_ADMIN).  The CAP_AUDIT_READ capability will be added for use
by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit
subsystem.

https://bugzilla.redhat.com/show_bug.cgi?id=887992

Feedback please!

Richard Guy Briggs (6):
  audit: refactor hold queue flush
  audit: flatten kauditd_thread wait queue code
  audit: move kaudit thread start from auditd registration to kaudit
init
  netlink: add send and receive capability requirement and capability
flags
  audit: add the first netlink multicast socket group
  audit: send multicast messages only if there are listeners

 include/linux/netlink.h |   4 +
 include/uapi/linux/audit.h  |   8 ++
 include/uapi/linux/capability.h |   5 +-
 kernel/audit.c  | 142 +---
 net/netlink/af_netlink.c|  35 +++--
 security/selinux/include/classmap.h |   2 +-
 6 files changed, 144 insertions(+), 52 deletions(-)

-- 
1.8.0.2

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 3/6] audit: move kaudit thread start from auditd registration to kaudit init

[Richard Guy Briggs]

The kauditd_thread() task was started only after the auditd userspace daemon
registers itself with kaudit.  This was fine when only auditd consumed messages
from the kaudit netlink unicast socket.  With the addition of a multicast group
to that socket it is more convenient to have the thread start on init of the
kaudit kernel subsystem.

Signed-off-by: Richard Guy Briggs rbri...@redhat.com
---

This is a code clean up in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional audit userspace client.

 kernel/audit.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 1531efb..02a5d9e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -676,16 +676,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (err)
return err;
 
-   /* As soon as there's any sign of userspace auditd,
-* start kauditd to talk to it */
-   if (!kauditd_task)
-   kauditd_task = kthread_run(kauditd_thread, NULL, kauditd);
-   if (IS_ERR(kauditd_task)) {
-   err = PTR_ERR(kauditd_task);
-   kauditd_task = NULL;
-   return err;
-   }
-
loginuid = audit_get_loginuid(current);
sessionid = audit_get_sessionid(current);
security_task_getsecid(current, sid);
@@ -974,6 +964,10 @@ static int __init audit_init(void)
else
audit_sock-sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
 
+   kauditd_task = kthread_run(kauditd_thread, NULL, kauditd);
+   if (IS_ERR(kauditd_task))
+   return PTR_ERR(kauditd_task);
+
skb_queue_head_init(audit_skb_queue);
skb_queue_head_init(audit_skb_hold_queue);
audit_initialized = AUDIT_INITIALIZED;
-- 
1.8.0.2

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 6/6] audit: send multicast messages only if there are listeners

[Richard Guy Briggs]

Test first to see if there are any userspace multicast listeners bound to the
socket before starting the multicast send work.

Signed-off-by: Richard Guy Briggs rbri...@redhat.com
---
 kernel/audit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9eef05b..d153a6b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -428,6 +428,8 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
struct sk_buff *copy;
struct nlmsghdr *nlh;
 
+   if (!netlink_has_listeners(audit_sock, AUDIT_NLGRP_READLOG))
+   return;
/*
 * The seemingly wasteful skb_copy() is necessary because the original
 * kaudit unicast socket sends up messages with nlmsg_len set to the
-- 
1.8.0.2

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/6] audit: flatten kauditd_thread wait queue code

[Richard Guy Briggs]

The wait queue control code in kauditd_thread() was nested deeper than
necessary.  The function has been flattened for better legibility.

Signed-off-by: Richard Guy Briggs rbri...@redhat.com
---

This is a code clean up in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional audit userspace client.

 kernel/audit.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 4bf486c..1531efb 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -458,10 +458,11 @@ static void flush_hold_queue(void)
 
 static int kauditd_thread(void *dummy)
 {
-   struct sk_buff *skb;
-
set_freezable();
while (!kthread_should_stop()) {
+   struct sk_buff *skb;
+   DECLARE_WAITQUEUE(wait, current);
+
flush_hold_queue();
 
skb = skb_dequeue(audit_skb_queue);
@@ -471,19 +472,18 @@ static int kauditd_thread(void *dummy)
kauditd_send_skb(skb);
else
audit_printk_skb(skb);
-   } else {
-   DECLARE_WAITQUEUE(wait, current);
-   set_current_state(TASK_INTERRUPTIBLE);
-   add_wait_queue(kauditd_wait, wait);
-
-   if (!skb_queue_len(audit_skb_queue)) {
-   try_to_freeze();
-   schedule();
-   }
+   continue;
+   }
+   set_current_state(TASK_INTERRUPTIBLE);
+   add_wait_queue(kauditd_wait, wait);
 
-   __set_current_state(TASK_RUNNING);
-   remove_wait_queue(kauditd_wait, wait);
+   if (!skb_queue_len(audit_skb_queue)) {
+   try_to_freeze();
+   schedule();
}
+
+   __set_current_state(TASK_RUNNING);
+   remove_wait_queue(kauditd_wait, wait);
}
return 0;
 }
-- 
1.8.0.2

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 4/6] netlink: add send and receive capability requirement and capability flags

[Richard Guy Briggs]

Currently netlink socket permissions are controlled by the
NL_CFG_F_NONROOT_{RECV,SEND} flags in the kernel socket configuration or by the
CAP_NET_ADMIN capability of the client.  The former allows non-root users
access to the socket.  The latter allows all network admin clients access to
the socket.  It would be useful to be able to further restrict this access to
send or receive capabilities individually within specific subsystems with a
more targetted capability.  Two more flags, NL_CFG_F_CAPABILITY_{RECV,SEND},
have been added to specifically require a named capability should the subsystem
request it, allowing the client to drop other broad unneeded capabilities.

Signed-off-by: Richard Guy Briggs rbri...@redhat.com
---

This is a feature addition in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional unicast auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(bot uses CAP_NET_ADMIN).  The CAP_AUDIT_READ capability will be added for use
by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit
subsystem.

 include/linux/netlink.h  |  4 
 net/netlink/af_netlink.c | 35 +--
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index e0f746b..30daf11 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -31,6 +31,8 @@ extern void netlink_table_ungrab(void);
 
 #define NL_CFG_F_NONROOT_RECV  (1  0)
 #define NL_CFG_F_NONROOT_SEND  (1  1)
+#define NL_CFG_F_CAPABILITY_RECV (1  2)
+#define NL_CFG_F_CAPABILITY_SEND (1  3)
 
 /* optional Netlink kernel configuration parameters */
 struct netlink_kernel_cfg {
@@ -39,6 +41,8 @@ struct netlink_kernel_cfg {
void(*input)(struct sk_buff *skb);
struct mutex*cb_mutex;
void(*bind)(int group);
+   int cap_send_requires;
+   int cap_recv_requires;
 };
 
 extern struct sock *__netlink_kernel_create(struct net *net, int unit,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c0353d5..cce1fe3 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -127,6 +127,8 @@ struct netlink_table {
struct module   *module;
void(*bind)(int group);
int registered;
+   int cap_send_requires;
+   int cap_recv_requires;
 };
 
 static struct netlink_table *nl_table;
@@ -552,6 +554,8 @@ static int netlink_release(struct socket *sock)
nl_table[sk-sk_protocol].bind = NULL;
nl_table[sk-sk_protocol].flags = 0;
nl_table[sk-sk_protocol].registered = 0;
+   nl_table[sk-sk_protocol].cap_send_requires = 0;
+   nl_table[sk-sk_protocol].cap_recv_requires = 0;
}
} else if (nlk-subscriptions) {
netlink_update_listeners(sk);
@@ -611,8 +615,20 @@ retry:
 
 static inline int netlink_capable(const struct socket *sock, unsigned int flag)
 {
-   return (nl_table[sock-sk-sk_protocol].flags  flag) ||
-   ns_capable(sock_net(sock-sk)-user_ns, CAP_NET_ADMIN);
+   struct netlink_table *nlt = nl_table[sock-sk-sk_protocol];
+
+   switch (flag  nlt-flags) {
+   case NL_CFG_F_NONROOT_RECV:
+   case NL_CFG_F_NONROOT_SEND:
+   return true;
+   case NL_CFG_F_CAPABILITY_RECV:
+   return capable(nlt-cap_recv_requires);
+   case NL_CFG_F_CAPABILITY_SEND:
+   return capable(nlt-cap_send_requires);
+   default:
+   return capable(CAP_NET_ADMIN);
+   }
+   return false;
 }
 
 static void
@@ -677,7 +693,8 @@ static int netlink_bind(struct socket *sock, struct 
sockaddr *addr,
 
/* Only superuser is allowed to listen multicasts */
if (nladdr-nl_groups) {
-   if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV))
+   if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV |
+  NL_CFG_F_CAPABILITY_RECV))
return -EPERM;
err = netlink_realloc_groups(sk);
if (err)
@@ -739,7 +756,9 @@ static int netlink_connect(struct socket *sock, struct 
sockaddr *addr,
return -EINVAL;
 
/* Only superuser is allowed to send multicasts */
-   if (nladdr-nl_groups  !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
+   if (nladdr-nl_groups 
+   !netlink_capable(sock, NL_CFG_F_NONROOT_SEND |
+  NL_CFG_F_CAPABILITY_SEND))
return -EPERM;
 
if (!nlk-portid)
@@ -1262,7 +1281,8 @@ static int netlink_setsockopt(struct socket *sock, int 
level, int optname,
break;
case

Re: PCI-DSS: Log every root actions/keystrokes but avoid passwords

[Richard Guy Briggs]

On Tue, Mar 12, 2013 at 07:06:59AM -0400, Miloslav Trmac wrote:
 - Original Message -
  I am resurrecting this old thread from last summer because I ran into the 
  same
  issue and found the thread in the archives via Google. It would be very 
  nice if
  everything could be logged except passwords.
 
 There is work being done.  Sorry, I don't have more specifics as to
 availability, perhaps others do.

Hi Tracy,

I'm actually working on that right now.  I have a patch I am in the
process of testing.  It implements a new sysctl.  I'm working in
the upstream kernel, so it will likely be available in Linus' git tree
before anywhere else.  After that, likely fedora, then RHEL, but I'm a
bit new to that process.

I don't see a reason why I couldn't post that patch here when I've got
it ironed out.

 Mirek

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: PCI-DSS: Log every root actions/keystrokes but avoid passwords

[Richard Guy Briggs]

On Tue, Mar 12, 2013 at 05:09:15PM -0400, Steve Grubb wrote:
 On Tuesday, March 12, 2013 04:47:42 PM Richard Guy Briggs wrote:
  On Tue, Mar 12, 2013 at 07:06:59AM -0400, Miloslav Trmac wrote:
   - Original Message -
   
I am resurrecting this old thread from last summer because I ran into
the same issue and found the thread in the archives via Google. It
would be very nice if everything could be logged except passwords.
   
   There is work being done.  Sorry, I don't have more specifics as to
   availability, perhaps others do.
  
  Hi Tracy,
  
  I'm actually working on that right now.  I have a patch I am in the
  process of testing.  It implements a new sysctl.
 
 Why would this be done as a sysctl? Everything else in the audit system is 
 configured through the netlink API. I would think that we would want to have 
 it 
 configured by the same pam module that we currently use to enable tty 
 auditing. 
 So, why not make a new netlink command that pam can use?

The lazy and naive answer is that that was the approach that was
suggested by two developers much more familiar with this code than me (I
expect that to balance out with time.)

Now that you suggest this, I agree that approach makes a lot of sense.

The more technical answer might be that it is much more expedient to do
it with a sysctl since it involves fewer compiled entities to implement
and hence can be rolled out faster with less co-ordination of other
software projects.  After the kernel is recompiled (needed in any case)
it can be implemented with one line added to a file in /etc/sysctl.d/
while your approach requires adding code to audit and pam, waiting for
it to be released by their respective teams, then the user adding a
config option to the pam module invocation.  I agree that would be more
convenient for end users since it can be an option added in the same
place as the module is invoked.

I haven't seen a lot of requests for this feature yet, but it sounds
like there could be a lot of interest, so it may be worth doing
correctly, rather than as a quick fix.

Am I missing anything?

  I'm working in the upstream kernel, so it will likely be available in Linus'
  git tree before anywhere else.
 
 Normally audit patches are sent to this mail list for review. If there are no 
 objections then it can be pulled into an upstream tree.

I'll post this patch anyways.

 -Steve
 
  After that, likely fedora, then RHEL, but I'm a bit new to that process.
  
  I don't see a reason why I couldn't post that patch here when I've got
  it ironed out.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: PCI-DSS: Log every root actions/keystrokes but avoid passwords

[Richard Guy Briggs]

On Wed, Mar 13, 2013 at 12:43:58PM -0400, Miloslav Trmac wrote:
 - Original Message -
   Please do post the patch here when you have it worked out as I am
   very likely
   to miss it in the flood of kernel patches when it goes to/from
   Linus.
  
  Here you go.  Given Steve's good question, this control method may
  change.
 
 Isn't icanon _true_ when the data is echoed?  This patch would allow
 dropping the echoed data (i.e. commands), not the non-echoed data
 (i.e. passwords).
 (I might be mistaken and I haven't tested this.)

Apparently not.  This is what took me longer than I initially thought
necessary to get this working, rechecking my pam incantations along the
way.  I went back and actually removed my switch and just isolated
icanon in the decision to abort the function to confirm how it worked,
then inverted the test which is when it started working.  Eric was right
to start with.

 Mirek

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Wed, Apr 10, 2013 at 11:02:43AM -0700, Eric W. Biederman wrote:
 Richard Guy Briggs r...@redhat.com writes:
  On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
  @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct 
  audit_rule *rule)
 if (!gid_valid(f-gid))
 goto exit_free;
 break;
  +  case AUDIT_LOGINUID_SET:
  +  if ((f-op != Audit_not_equal)  (f-op != 
  Audit_equal))
  +  goto exit_free;
  +  if ((f-val != 0)  (f-val != 1))
 
  Why the extra comparison to 1?
 
  Are you anticipating already a userspace process making a call using the
  newof type AUDIT_LOGINUID_SET with a value of 1?
 
 Sorry I missed this question the first time.  I am anticipating
 AUDIT_LOGINUID_SET to return a value of 0 or 1 (a boolean) and so I
 allow the operations and constants that are valid for a boolean.
 
 In particuluar I allow the opeartions == !=  and the boolean constants 0 and 
 1.

Duh, of course...  sorry for being thick.

  diff --git a/kernel/auditsc.c b/kernel/auditsc.c
  index 3a11d34..27d0a50 100644
  --- a/kernel/auditsc.c
  +++ b/kernel/auditsc.c
  @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
 if (ctx)
 result = audit_uid_comparator(tsk-loginuid, 
  f-op, f-uid);
 break;
 
  (OT: I assume the if (ctx) is wrong in the AUDIT_LOGINUID case
  above.)
 
 Good question.  I didn't see that when I was preparing my patch.
 
 ctx is not necessary but I think ctx is set when a task is being audited
 so it may serve a useful function.  But I have to admit it that if(ctx)
 looks like a bug.

Thanks...

 Eric

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
 Andrew Morton a...@linux-foundation.org writes:
  On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs r...@redhat.com 
  wrote:
  audit rule additions containing -F auid!=4294967295 were failing with 
  EINVAL.
 
 The only case where this appears to make the least little bit of sense
 is if the goal of the test is to test to see if an audit logloginuid
 has been set at all.  In which case depending on a test against
 4294967295 is bogus because you are depending on an intimate internal
 kernel implementation detail.
 
 How about something like my untested patch below that add an explicit
 operation to test if loginuid has been set?

Sorry for the delay in testing this, I had another urgent bug and a
belligerent test box...

I like this approach better than mine now that I understand it.  I've
tested the patch below without any changes.  It works as expected with
my previous test case.  I don't know if a Signed-off-by: is appropriate
for me in this case, but I'll throw in a:

Tested-by: Richard Guy Briggs rbri...@redhat.com

and recommend a:

Reported-By: Steve Grubb sbr...@redhat.com


 Eric
 
 From: Eric W. Biederman ebied...@xmission.com
 Date: Tue, 9 Apr 2013 02:22:10 -0700
 Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
 
 audit rule additions containing -F auid!=4294967295 were failing
 with EINVAL.
 
 Apparently some userland audit rule sets want to know if loginuid uid
 has been set and are using a test for auid != 4294967295 to determine
 that.
 
 In practice that is a horrible way to ask if a value has been set,
 because it relies on subtle implementation details and will break
 every time the uid implementation in the kernel changes.
 
 So add a clean way to test if the audit loginuid has been set, and
 silently convert the old idiom to the cleaner and more comprehensible
 new idiom.
 
 Reported-By: Richard Guy Briggs r...@redhat.com wrote:
 Signed-off-by: Eric W. Biederman ebied...@xmission.com
 ---
  include/linux/audit.h  |5 +
  include/uapi/linux/audit.h |1 +
  kernel/auditfilter.c   |   29 +
  kernel/auditsc.c   |5 -
  4 files changed, 39 insertions(+), 1 deletions(-)
 
 diff --git a/include/linux/audit.h b/include/linux/audit.h
 index a9fefe2..8a1ddde 100644
 --- a/include/linux/audit.h
 +++ b/include/linux/audit.h
 @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
  #define audit_signals 0
  #endif /* CONFIG_AUDITSYSCALL */
  
 +static inline bool audit_loginuid_set(struct task_struct *tsk)
 +{
 + return uid_valid(audit_get_loginuid(tsk));
 +}
 +
  #ifdef CONFIG_AUDIT
  /* These are defined in audit.c */
   /* Public API */
 diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
 index 9f096f1..9554a19 100644
 --- a/include/uapi/linux/audit.h
 +++ b/include/uapi/linux/audit.h
 @@ -246,6 +246,7 @@
  #define AUDIT_OBJ_TYPE   21
  #define AUDIT_OBJ_LEV_LOW22
  #define AUDIT_OBJ_LEV_HIGH   23
 +#define AUDIT_LOGINUID_SET   24
  
   /* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
 diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
 index 540f986..6381d17 100644
 --- a/kernel/auditfilter.c
 +++ b/kernel/auditfilter.c
 @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct 
 audit_rule *rule)
   if (f-op == Audit_bad)
   goto exit_free;
  
 + /* Support legacy tests for a valid loginuid */
 + if ((f-type == AUDIT_LOGINUID)  (f-val == 4294967295)) {
 + f-type = AUDIT_LOGINUID_SET;
 + f-val = 0;
 + }
 +
   switch(f-type) {
   default:
   goto exit_free;
 @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct 
 audit_rule *rule)
   if (!gid_valid(f-gid))
   goto exit_free;
   break;
 + case AUDIT_LOGINUID_SET:
 + if ((f-op != Audit_not_equal)  (f-op != 
 Audit_equal))
 + goto exit_free;
 + if ((f-val != 0)  (f-val != 1))
 + goto exit_free;
 + break;
   case AUDIT_PID:
   case AUDIT_PERS:
   case AUDIT_MSGTYPE:
 @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct 
 audit_rule_data *data,
   f-gid = INVALID_GID;
   f-lsm_str = NULL;
   f-lsm_rule = NULL;
 +
 + /* Support legacy tests for a valid loginuid */
 + if ((f-type == AUDIT_LOGINUID)  (f-val == 4294967295)) {
 + f-type = AUDIT_LOGINUID_SET;
 + f-val = 0

Re: [PATCH] vfs: fix audit_inode call in O_CREAT case of do_last

[Richard Guy Briggs]

On Fri, Apr 12, 2013 at 03:16:32PM -0400, Jeff Layton wrote:
 Jiri reported a regression in auditing of open(..., O_CREAT) syscalls.
 In older kernels, creating a file with open(..., O_CREAT) created
 audit_name records that looked like this:
 
 type=PATH msg=audit(1360255720.628:64): item=1 name=/abc/foo inode=138810 
 dev=fd:00 mode=0100640 ouid=0 ogid=0 rdev=00:00 
 obj=unconfined_u:object_r:default_t:s0
 type=PATH msg=audit(1360255720.628:64): item=0 name=/abc/ inode=138635 
 dev=fd:00 mode=040750 ouid=0 ogid=0 rdev=00:00 
 obj=unconfined_u:object_r:default_t:s0
 
 ...in recent kernels though, they look like this:
 
 type=PATH msg=audit(1360255402.886:12574): item=2 name=(null) inode=264599 
 dev=fd:00 mode=0100640 ouid=0 ogid=0 rdev=00:00 
 obj=unconfined_u:object_r:default_t:s0
 type=PATH msg=audit(1360255402.886:12574): item=1 name=(null) inode=264598 
 dev=fd:00 mode=040750 ouid=0 ogid=0 rdev=00:00 
 obj=unconfined_u:object_r:default_t:s0
 type=PATH msg=audit(1360255402.886:12574): item=0 name=/abc/foo 
 inode=264598 dev=fd:00 mode=040750 ouid=0 ogid=0 rdev=00:00 
 obj=unconfined_u:object_r:default_t:s0
 
 Richard bisected to determine that the problems started with commit
 bfcec708, but the log messages have changed with some later
 audit-related patches.
 
 The problem is that this audit_inode call is passing in the parent of
 the dentry being opened, but audit_inode is being called with the parent
 flag false. This causes later audit_inode and audit_inode_child calls to
 match the wrong entry in the audit_names list.
 
 This patch simply sets the flag to properly indicate that this inode
 represents the parent. With this, the audit_names entries are back to
 looking like they did before.

This patch fixes the problem for me.

Tested-by: Richard Guy Briggs rbri...@redhat.com

 Cc: sta...@vger.kernel.org # v3.7+
 Cc: Richard Guy Briggs rbri...@redhat.com
 Reported-by: Jiri Jaburek jjabu...@redhat.com
 Signed-off-by: Jeff Layton jlay...@redhat.com
 ---
  fs/namei.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/fs/namei.c b/fs/namei.c
 index 57ae9c8..85e40d1 100644
 --- a/fs/namei.c
 +++ b/fs/namei.c
 @@ -2740,7 +2740,7 @@ static int do_last(struct nameidata *nd, struct path 
 *path,
   if (error)
   return error;
  
 - audit_inode(name, dir, 0);
 + audit_inode(name, dir, LOOKUP_PARENT);
   error = -EISDIR;
   /* trailing slashes? */
   if (nd-last.name[nd-last.len])
 -- 
 1.7.1
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: pam_tty_audit icanon log switch

[Richard Guy Briggs]

On Thu, Apr 11, 2013 at 04:43:45PM -0400, Eric Paris wrote:
 - Original Message -
  Hi folks,
  
  There's been a couple of requests to add a switch to pam_tty_audit to
  *not* log passwords when logging user commands.
 
  Here are two patches, the first to pam to add the switch to
  the pam_tty_audit module.  The second is to the kernel to add the
  necessary bits in audit and tty:
 
 Patches as attachments are little harder to comment on.  So inline preferred.

I agree.  I wasn't sure of the best way to present both userspace and
kernel patches in the same email so that any automatic patcher won't get
confused.  In this case, since it is an RFC, it isn't as critical, so
convenience for commenting overrides.

(more inline below)

 From 110971ad92ce8669f6dc18db9e6369e92afdd03e Mon Sep 17 00:00:00 2001
 From: Richard Guy Briggs r...@redhat.com
 Date: Thu, 21 Mar 2013 00:52:37 -0400
 Subject: [PATCH] tty: add an option to control logging of passwords with 
 pam_tty_audit
 To: linux-audit@redhat.com
 
 Most commands are entered one line at a time and processed as complete lines
 in non-canonical mode.  Commands that interactively require a password, enter
 canonical mode to do this.  This feature (icanon) can be used to avoid logging
 passwords by audit while still logging the rest of the command.
 
 Adding a member to the struct audit_tty_status passed in by pam_tty_audit
 allows control of canonical mode per task.
 
 Signed-off-by: Richard Guy Briggs r...@redhat.com
 ---
  drivers/tty/tty_audit.c|8 
  include/linux/sched.h  |1 +
  include/uapi/linux/audit.h |3 ++-
  kernel/audit.c |5 -
  4 files changed, 15 insertions(+), 2 deletions(-)
 
 [snip]
 
 --- a/include/uapi/linux/audit.h
 +++ b/include/uapi/linux/audit.h
 @@ -369,7 +369,8 @@ struct audit_status {
  };
  
  struct audit_tty_status {
 - __u32   enabled; /* 1 = enabled, 0 = disabled */
 + __u32   enabled;/* 1 = enabled, 0 = disabled */
 + __u32   log_icanon; /* 1 = enabled, 0 = disabled */
  };
  
  /* audit_rule_data supports filter rules with both integer and string
 diff --git a/kernel/audit.c b/kernel/audit.c
 index d596e53..98d43c6 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
  
   spin_lock_irq(tsk-sighand-siglock);
   s.enabled = tsk-signal-audit_tty != 0;
 + s.log_icanon = tsk-signal-audit_tty_log_icanon != 0;
   spin_unlock_irq(tsk-sighand-siglock);
  
   audit_send_reply(NETLINK_CB(skb).portid, seq,
 @@ -886,11 +887,13 @@ static int audit_receive_msg(struct sk_buff *skb, 
 struct nlmsghdr *nlh)
   if (nlh-nlmsg_len  sizeof(struct audit_tty_status))
   return -EINVAL;
   s = data;
 
 
 what happens if this comes from an old pam stack that didn't set/send 
 log_icanon?  We'd just be reading past the end of the allocation, right?  
 Maybe something like
 
 unsigned log_icanon
 if (nlmsg_len(nlh)  sizeof(struct audit_tty_status))
 log_icanon = 0;
 else
 log_icanon = s-log_icanon

Good point.  In fact it is more complicated than that.  The previous
statement takes care of that, simply bouncing the request, so no danger
of reading past the allocation.  The request could in fact be legal for
the previous version of userspace and it would not get to this point,
and would simply exit with -EINVAL, failing on what would have
previously been fine.  It could try and see if it matches the length of
the previous version of the struct and behave accordingly, but that is
a messy kludge.  Is it time to bump an audit API version number (I don't
find one.)?

 - if (s-enabled != 0  s-enabled != 1)
 + if (s-enabled != 0  s-enabled != 1
 +  s-log_icanon != 0  s-log_icanon != 1)
 
 
 Shouldn't this be
 if ((s-enabled != 0  s-enabled != 1) || log_icanon != 0  log_icanon != 
 1))

Missing a paren, but yes, you are correct.  Thank you:

 if ((s-enabled != 0  s-enabled != 1) || (log_icanon != 0  log_icanon != 
1))

   return -EINVAL;
  
   spin_lock_irq(tsk-sighand-siglock);
   tsk-signal-audit_tty = s-enabled != 0;
 + tsk-signal-audit_tty_log_icanon = s-log_icanon != 0;
   spin_unlock_irq(tsk-sighand-siglock);
   break;
   }
 -- 
 1.7.1

Here's an incremental diff, full replacement patch below that:

8=
diff --git a/kernel/audit.c b/kernel/audit.c
index 98d43c6..89b9b9c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -883,17 +883,28 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
case AUDIT_TTY_SET: {
struct audit_tty_status *s;
struct task_struct *tsk = current;
+   struct audit_tty_status_old

Re: pam_tty_audit icanon log switch

[Richard Guy Briggs]

On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
 Hello,
 
 - Original Message -
  Full replacement patch:
 
 I'm still convinced that icanon is not the correct condition, see
 https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .

That's a seperate issue.  :)

I'll come back to that...

  diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
  index 9f096f1..a863669 100644
  --- a/include/uapi/linux/audit.h
  +++ b/include/uapi/linux/audit.h
  @@ -369,7 +369,8 @@ struct audit_status {
   };
   
   struct audit_tty_status {
  -   __u32   enabled; /* 1 = enabled, 0 = disabled */
  +   __u32   enabled;/* 1 = enabled, 0 = disabled */
  +   __u32   log_icanon; /* 1 = enabled, 0 = disabled */
   };
 
 Also, would it make sense for the user-space API to be more general
 about expressing the intent (log passwords)?  I don't know, being
 precise about the exact effect of the option is also beneficial.

Hmmm, I'll have to ponder that...

 Mirek

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: pam_tty_audit icanon log switch

[Richard Guy Briggs]

On Thu, Apr 18, 2013 at 04:07:08PM -0400, Richard Guy Briggs wrote:
 On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
  Hello,
  
  - Original Message -
   Full replacement patch:
  
  I'm still convinced that icanon is not the correct condition, see
  https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
 
 That's a seperate issue.  :)
 
 I'll come back to that...

Ok, thank you for bringing me back to that.  And thank you for the test
case suggestions.  You are correct, !echo is needed too.  I've added it
back.

   diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
   index 9f096f1..a863669 100644
   --- a/include/uapi/linux/audit.h
   +++ b/include/uapi/linux/audit.h
   @@ -369,7 +369,8 @@ struct audit_status {
};

struct audit_tty_status {
   - __u32   enabled; /* 1 = enabled, 0 = disabled */
   + __u32   enabled;/* 1 = enabled, 0 = disabled */
   + __u32   log_icanon; /* 1 = enabled, 0 = disabled */
};
  
  Also, would it make sense for the user-space API to be more general
  about expressing the intent (log passwords)?  I don't know, being
  precise about the exact effect of the option is also beneficial.
 
 Hmmm, I'll have to ponder that...

I am inclined to leave it named as is for precision.  The reason for the
option is covered in the manpage.  Can you suggest a better wording for
the manpage if you don't think it is clear enough?  A comment in the
source code wouldn't hurt though, now that you mention it.

  Mirek
 
 - RGB

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: pam_tty_audit icanon log switch

[Richard Guy Briggs]

On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
 Hello,

Mirek,

 - Original Message -
  Full replacement patch:
 
 I'm still convinced that icanon is not the correct condition, see 
 https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .

As I indicated in a previous email, from my testing, you are correct.
This patch appended should address that.  Comments?

 Mirek

- RGB

 drivers/tty/tty_audit.c|8 
 include/linux/sched.h  |1 +
 include/uapi/linux/audit.h |3 ++-
 kernel/audit.c |5 -
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 6953dc8..dc1bf78 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -153,6 +153,7 @@ void tty_audit_fork(struct signal_struct *sig)
 {
spin_lock_irq(current-sighand-siglock);
sig-audit_tty = current-signal-audit_tty;
+   sig-audit_tty_log_icanon = current-signal-audit_tty_log_icanon;
spin_unlock_irq(current-sighand-siglock);
 }
 
@@ -292,10 +293,17 @@ void tty_audit_add_data(struct tty_struct *tty, unsigned 
char *data,
 {
struct tty_audit_buf *buf;
int major, minor;
+   int audit_log_tty_icanon;
 
if (unlikely(size == 0))
return;
 
+   spin_lock_irq(current-sighand-siglock);
+   audit_log_tty_icanon = current-signal-audit_tty_log_icanon;
+   spin_unlock_irq(current-sighand-siglock);
+   if (!audit_log_tty_icanon  icanon  !L_ECHO(tty))
+   return;
+
if (tty-driver-type == TTY_DRIVER_TYPE_PTY
 tty-driver-subtype == PTY_TYPE_MASTER)
return;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35d2b6..031aa39 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -606,6 +606,7 @@ struct signal_struct {
 #endif
 #ifdef CONFIG_AUDIT
unsigned audit_tty;
+   unsigned audit_tty_log_icanon;
struct tty_audit_buf *tty_audit_buf;
 #endif
 #ifdef CONFIG_CGROUPS
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..a863669 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
 };
 
 struct audit_tty_status {
-   __u32   enabled; /* 1 = enabled, 0 = disabled */
+   __u32   enabled;/* 1 = enabled, 0 = disabled */
+   __u32   log_icanon; /* 1 = enabled, 0 = disabled */
 };
 
 /* audit_rule_data supports filter rules with both integer and string
diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..98d43c6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
 
spin_lock_irq(tsk-sighand-siglock);
s.enabled = tsk-signal-audit_tty != 0;
+   s.log_icanon = tsk-signal-audit_tty_log_icanon != 0;
spin_unlock_irq(tsk-sighand-siglock);
 
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -886,11 +887,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (nlh-nlmsg_len  sizeof(struct audit_tty_status))
return -EINVAL;
s = data;
-   if (s-enabled != 0  s-enabled != 1)
+   if (s-enabled != 0  s-enabled != 1
+s-log_icanon != 0  s-log_icanon != 1)
return -EINVAL;
 
spin_lock_irq(tsk-sighand-siglock);
tsk-signal-audit_tty = s-enabled != 0;
+   tsk-signal-audit_tty_log_icanon = s-log_icanon != 0;
spin_unlock_irq(tsk-sighand-siglock);
break;
}
-- 
1.7.1

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: pam_tty_audit icanon log switch

[Richard Guy Briggs]

On Fri, Mar 22, 2013 at 08:19:31AM +0100, Tomas Mraz wrote:
 On Fri, 2013-03-22 at 01:46 -0400, Richard Guy Briggs wrote: 
  Hi folks,
  
  There's been a couple of requests to add a switch to pam_tty_audit to
  *not* log passwords when logging user commands.
  
  Most commands are entered one line at a time and processed as complete
  lines in non-canonical mode.  Commands that interactively require a
  password, enter canonical mode to do this.  This feature (icanon) can be
  used to avoid logging passwords by audit while still logging the rest of
  the command.
  
  Adding a member to the struct audit_tty_status passed in by
  pam_tty_audit allows control of canonical mode per task.
  
 
 For the upstream inclusion of the pam_tty_audit patch you will need to
 add a detection of the new member of the struct audit_tty_status in the
 configure.in and #ifdef the code properly. The new option can be kept
 even in the case the new member is not available, but it should log a
 warning into the syslog with pam_syslog() when used. The documentation
 should reflect the fact that the option might not be available on old
 kernels as well.

Tomas,

Please have a look at this patch and see if this addresses the issues
you raised:

---
 configure.in  |   15 +++
 modules/pam_tty_audit/Makefile.am |3 +++
 modules/pam_tty_audit/pam_tty_audit.8.xml |   14 ++
 modules/pam_tty_audit/pam_tty_audit.c |   23 ++-
 4 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/configure.in b/configure.in
index 515b301..c9c1c5f 100644
--- a/configure.in
+++ b/configure.in
@@ -386,6 +386,19 @@ if test x$WITH_LIBAUDIT != xno ; then
 fi
 if test ! -z $HAVE_AUDIT_TTY_STATUS ; then
 AC_DEFINE([HAVE_AUDIT_TTY_STATUS], 1, [Define to 1 if struct 
audit_tty_status exists.])
+
+AC_CHECK_MEMBER(
+[struct audit_tty_status.log_icanon],
+[
+ HAVE_AUDIT_TTY_STATUS_LOG_ICANON=yes
+ AC_DEFINE([HAVE_AUDIT_TTY_STATUS_LOG_ICANON], 1, 
[Define to 1 if struct audit_tty_status.log_icanon exists.])
+],
+[
+ HAVE_AUDIT_TTY_STATUS_LOG_ICANON=
+ AC_MSG_WARN([The struct 
audit_tty_status.log_icanon member is needed for the log_icanon option.  The 
log_icanon option is disabled.])
+],
+[[#include libaudit.h]]
+)
 fi
 else
LIBAUDIT=
@@ -393,6 +406,8 @@ fi
 AC_SUBST(LIBAUDIT)
 AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS],
   [test x$HAVE_AUDIT_TTY_STATUS = xyes])
+AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_ICANON],
+  [test x$HAVE_AUDIT_TTY_STATUS_LOG_ICANON = xyes])
 
 AC_CHECK_HEADERS(xcrypt.h crypt.h)
 AS_IF([test x$ac_cv_header_xcrypt_h = xyes],
diff --git a/modules/pam_tty_audit/Makefile.am 
b/modules/pam_tty_audit/Makefile.am
index 6378483..b67d2e5 100644
--- a/modules/pam_tty_audit/Makefile.am
+++ b/modules/pam_tty_audit/Makefile.am
@@ -16,6 +16,9 @@ XMLS = README.xml pam_tty_audit.8.xml
 securelibdir = $(SECUREDIR)
 
 AM_CFLAGS = -I$(top_srcdir)/libpam/include -I$(top_srcdir)/libpamc/include
+if HAVE_AUDIT_TTY_STATUS_LOG_ICANON
+  AM_CFLAGS += -DHAVE_AUDIT_TTY_STATUS_LOG_ICANON
+endif
 AM_LDFLAGS = -no-undefined -avoid-version -module
 if HAVE_VERSIONING
   AM_LDFLAGS += -Wl,--version-script=$(srcdir)/../modules.map
diff --git a/modules/pam_tty_audit/pam_tty_audit.8.xml 
b/modules/pam_tty_audit/pam_tty_audit.8.xml
index 447b845..f451f45 100644
--- a/modules/pam_tty_audit/pam_tty_audit.8.xml
+++ b/modules/pam_tty_audit/pam_tty_audit.8.xml
@@ -77,6 +77,18 @@
   /para
 /listitem
   /varlistentry
+  varlistentry
+term
+  optionlog_icanon/option
+/term
+listitem
+  para
+   Log keystrokes in ICANON mode.  By default, keystrokes in ICANON
+   mode are not logged to avoid logging passwords.  This option may not
+   be available on older kernels (3.9?).
+  /para
+/listitem
+  /varlistentry
 /variablelist
   /refsect1
 
@@ -161,6 +173,8 @@ session required pam_tty_audit.so disable=* enable=root
   para
 pam_tty_audit was written by Miloslav Trmaccaron;
lt;m...@redhat.comgt;.
+The log_icanon option was added by Richard Guy Briggs
+lt;r...@redhat.comgt;.
   /para
   /refsect1
 
diff --git a/modules/pam_tty_audit/pam_tty_audit.c 
b/modules/pam_tty_audit/pam_tty_audit.c
index 080f495..7be914b 100644
--- a/modules/pam_tty_audit/pam_tty_audit.c
+++ b/modules/pam_tty_audit/pam_tty_audit.c
@@ -201,6 +201,9 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int 
argc, const char **argv)
   struct audit_tty_status *old_status, new_status;
   const char

[PATCH 1/2] audit: use given values in tty_audit enable api

[Richard Guy Briggs]

In send/GET, we don't want the kernel to lie about what value is set.

In recv/SET, the values are already filtered and don't need cleansing.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..64354eb 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -872,7 +872,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
struct task_struct *tsk = current;
 
spin_lock_irq(tsk-sighand-siglock);
-   s.enabled = tsk-signal-audit_tty != 0;
+   s.enabled = tsk-signal-audit_tty;
spin_unlock_irq(tsk-sighand-siglock);
 
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -890,7 +890,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
return -EINVAL;
 
spin_lock_irq(tsk-sighand-siglock);
-   tsk-signal-audit_tty = s-enabled != 0;
+   tsk-signal-audit_tty = s-enabled;
spin_unlock_irq(tsk-sighand-siglock);
break;
}
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Thu, May 09, 2013 at 09:29:18AM -0400, Steve Grubb wrote:
 On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
  On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
   Andrew Morton a...@linux-foundation.org writes:
On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs r...@redhat.com 
 wrote:
audit rule additions containing -F auid!=4294967295 were failing with
EINVAL. 
   The only case where this appears to make the least little bit of sense
   is if the goal of the test is to test to see if an audit logloginuid
   has been set at all.  In which case depending on a test against
   4294967295 is bogus because you are depending on an intimate internal
   kernel implementation detail.
   
   How about something like my untested patch below that add an explicit
   operation to test if loginuid has been set?
  
  Sorry for the delay in testing this, I had another urgent bug and a
  belligerent test box...
  
  I like this approach better than mine now that I understand it.  I've
  tested the patch below without any changes.  It works as expected with
  my previous test case.  I don't know if a Signed-off-by: is appropriate
  for me in this case, but I'll throw in a:
  
  Tested-by: Richard Guy Briggs rbri...@redhat.com
  
  and recommend a:
  
  Reported-By: Steve Grubb sbr...@redhat.com
 
 If this is the approved patch, can it be put in stable? The audit system 
 hasn't worked as intended since January.

Most certainly.  I should have added that.  I only thought about that
after sending.

 Thanks,
 -Steve
 
   From: Eric W. Biederman ebied...@xmission.com
   Date: Tue, 9 Apr 2013 02:22:10 -0700
   Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
   
   audit rule additions containing -F auid!=4294967295 were failing
   with EINVAL.
   
   Apparently some userland audit rule sets want to know if loginuid uid
   has been set and are using a test for auid != 4294967295 to determine
   that.
   
   In practice that is a horrible way to ask if a value has been set,
   because it relies on subtle implementation details and will break
   every time the uid implementation in the kernel changes.
   
   So add a clean way to test if the audit loginuid has been set, and
   silently convert the old idiom to the cleaner and more comprehensible
   new idiom.
   
   Reported-By: Richard Guy Briggs r...@redhat.com wrote:
   Signed-off-by: Eric W. Biederman ebied...@xmission.com
   ---
   
include/linux/audit.h  |5 +
include/uapi/linux/audit.h |1 +
kernel/auditfilter.c   |   29 +
kernel/auditsc.c   |5 -
4 files changed, 39 insertions(+), 1 deletions(-)
   
   diff --git a/include/linux/audit.h b/include/linux/audit.h
   index a9fefe2..8a1ddde 100644
   --- a/include/linux/audit.h
   +++ b/include/linux/audit.h
   @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
   *t)
   
#define audit_signals 0
#endif /* CONFIG_AUDITSYSCALL */
   
   +static inline bool audit_loginuid_set(struct task_struct *tsk)
   +{
   + return uid_valid(audit_get_loginuid(tsk));
   +}
   +
   
#ifdef CONFIG_AUDIT
/* These are defined in audit.c */

 /* Public API */
   
   diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
   index 9f096f1..9554a19 100644
   --- a/include/uapi/linux/audit.h
   +++ b/include/uapi/linux/audit.h
   @@ -246,6 +246,7 @@
   
#define AUDIT_OBJ_TYPE   21
#define AUDIT_OBJ_LEV_LOW22
#define AUDIT_OBJ_LEV_HIGH   23
   
   +#define AUDIT_LOGINUID_SET   24
   
 /* These are ONLY useful when checking
 
  * at syscall exit time (AUDIT_AT_EXIT). */
   
   diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
   index 540f986..6381d17 100644
   --- a/kernel/auditfilter.c
   +++ b/kernel/auditfilter.c
   @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
   audit_rule *rule) 
 if (f-op == Audit_bad)
 
 goto exit_free;
   
   + /* Support legacy tests for a valid loginuid */
   + if ((f-type == AUDIT_LOGINUID)  (f-val == 4294967295)) {
   + f-type = AUDIT_LOGINUID_SET;
   + f-val = 0;
   + }
   +
   
 switch(f-type) {
 
 default:
 goto exit_free;
   
   @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
   audit_rule *rule) 
 if (!gid_valid(f-gid))
 
 goto exit_free;
 
 break;
   
   + case AUDIT_LOGINUID_SET:
   + if ((f-op != Audit_not_equal)  (f-op != 
   Audit_equal))
   + goto exit_free;
   + if ((f-val != 0)  (f-val

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Thu, May 09, 2013 at 09:52:47AM -0400, Richard Guy Briggs wrote:
 On Thu, May 09, 2013 at 09:29:18AM -0400, Steve Grubb wrote:
  On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
   On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
Andrew Morton a...@linux-foundation.org writes:
 On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs 
 r...@redhat.com 
  wrote:
 audit rule additions containing -F auid!=4294967295 were failing 
 with
 EINVAL. 
The only case where this appears to make the least little bit of sense
is if the goal of the test is to test to see if an audit logloginuid
has been set at all.  In which case depending on a test against
4294967295 is bogus because you are depending on an intimate internal
kernel implementation detail.

How about something like my untested patch below that add an explicit
operation to test if loginuid has been set?
   
   Sorry for the delay in testing this, I had another urgent bug and a
   belligerent test box...
   
   I like this approach better than mine now that I understand it.  I've
   tested the patch below without any changes.  It works as expected with
   my previous test case.  I don't know if a Signed-off-by: is appropriate
   for me in this case, but I'll throw in a:
   
   Tested-by: Richard Guy Briggs rbri...@redhat.com
   
   and recommend a:
   
   Reported-By: Steve Grubb sbr...@redhat.com
  
  If this is the approved patch, can it be put in stable? The audit system 
  hasn't worked as intended since January.
 
 Most certainly.  I should have added that.  I only thought about that
 after sending.

No worries, Eric added it in his set in his pull request to Linus.

  Thanks,
  -Steve
  
From: Eric W. Biederman ebied...@xmission.com
Date: Tue, 9 Apr 2013 02:22:10 -0700
Subject: [PATCH] audit: Make testing for a valid loginuid explicit.

audit rule additions containing -F auid!=4294967295 were failing
with EINVAL.

Apparently some userland audit rule sets want to know if loginuid uid
has been set and are using a test for auid != 4294967295 to determine
that.

In practice that is a horrible way to ask if a value has been set,
because it relies on subtle implementation details and will break
every time the uid implementation in the kernel changes.

So add a clean way to test if the audit loginuid has been set, and
silently convert the old idiom to the cleaner and more comprehensible
new idiom.

Reported-By: Richard Guy Briggs r...@redhat.com wrote:
Signed-off-by: Eric W. Biederman ebied...@xmission.com
---

 include/linux/audit.h  |5 +
 include/uapi/linux/audit.h |1 +
 kernel/auditfilter.c   |   29 +
 kernel/auditsc.c   |5 -
 4 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index a9fefe2..8a1ddde 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
*t)

 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */

+static inline bool audit_loginuid_set(struct task_struct *tsk)
+{
+   return uid_valid(audit_get_loginuid(tsk));
+}
+

 #ifdef CONFIG_AUDIT
 /* These are defined in audit.c */
 
/* Public API */

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..9554a19 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -246,6 +246,7 @@

 #define AUDIT_OBJ_TYPE 21
 #define AUDIT_OBJ_LEV_LOW  22
 #define AUDIT_OBJ_LEV_HIGH 23

+#define AUDIT_LOGINUID_SET 24

/* These are ONLY useful when checking

 * at syscall exit time 
(AUDIT_AT_EXIT). */

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 540f986..6381d17 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -349,6 +349,12 @@ static struct audit_entry 
*audit_rule_to_entry(struct
audit_rule *rule) 
if (f-op == Audit_bad)

goto exit_free;

+   /* Support legacy tests for a valid loginuid */
+   if ((f-type == AUDIT_LOGINUID)  (f-val == 
4294967295)) {
+   f-type = AUDIT_LOGINUID_SET;
+   f-val = 0;
+   }
+

switch(f-type) {

default:
goto exit_free;

@@ -377,6 +383,12 @@ static struct audit_entry 
*audit_rule_to_entry(struct

[PATCH] audit: cast decimal constant for invalid uid to unsigned

[Richard Guy Briggs]

SFR reported this 2013-05-15:

 After merging the final tree, today's linux-next build (i386 defconfig)
 produced this warning:

 kernel/auditfilter.c: In function 'audit_data_to_entry':
 kernel/auditfilter.c:426:3: warning: this decimal constant is unsigned only
 in ISO C90 [enabled by default]

 Introduced by commit 780a7654cee8 (audit: Make testing for a valid
 loginuid explicit) from Linus' tree.

Replace this decimal constant in the code with a macro to make it more readable
and add an unsigned cast to quiet the warning.

Cc: sta...@vger.kernel.org # v3.9
Cc: Eric Paris epa...@redhat.com
Cc: Stephen Rothwell s...@canb.auug.org.au
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/uapi/linux/audit.h |2 ++
 kernel/auditfilter.c   |2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 75cef3f..b7cb978 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -374,6 +374,8 @@ struct audit_tty_status {
__u32   log_passwd; /* 1 = enabled, 0 = disabled */
 };
 
+#define AUDIT_UID_UNSET (unsigned int)-1
+
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
  * AUDIT_LIST_RULES requests.
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bc6595f..0d40b51 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -423,7 +423,7 @@ static struct audit_entry *audit_data_to_entry(struct 
audit_rule_data *data,
f-lsm_rule = NULL;
 
/* Support legacy tests for a valid loginuid */
-   if ((f-type == AUDIT_LOGINUID)  (f-val == 4294967295)) {
+   if ((f-type == AUDIT_LOGINUID)  (f-val == AUDIT_UID_UNSET)) 
{
f-type = AUDIT_LOGINUID_SET;
f-val = 0;
}
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Richard Guy Briggs]

On Fri, May 24, 2013 at 12:11:44PM -0400, Eric Paris wrote:
 The audit_status structure was not designed with extensibility in mind.
 Define a new AUDIT_SET_FEATURE message type which takes a new structure
 of bits where things can be enabled/disabled/locked one at a time.  This
 structure should be able to grow in the future while maintaining forward
 and backward compatibility (based loosly on the ideas from capabilities
 and prctl)
 
 This does not actually add any features, but is just infrastructure to
 allow new on/off types of audit system features.

This is the sort of infrastructure that occured to me for the
audit_tty_status structure, when I implemented the password logging
switch...

 Signed-off-by: Eric Paris epa...@redhat.com

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [Pam-developers] [PATCH] pam_tty_audit: add an option to control logging of passwords: log_passwd

[Richard Guy Briggs]

On Mon, Jun 10, 2013 at 04:59:37PM -0400, Richard Guy Briggs wrote:
 On Wed, Jun 05, 2013 at 02:54:09AM +0400, Dmitry V. Levin wrote:
  On Thu, May 23, 2013 at 10:29:59AM -0400, Richard Guy Briggs wrote:
   Most commands are entered one line at a time and processed as complete 
   lines
   in non-canonical mode.  Commands that interactively require a password, 
   enter
   canonical mode with echo set to off to do this.  This feature (icanon and
   !echo) can be used to avoid logging passwords by audit while still 
   logging the
   rest of the command.
   
   Adding a member to the struct audit_tty_status passed in by pam_tty_audit
   allows control of logging passwords per task.
  
  Sorry for the long delay with review.  Please see my comments below.
 
 Ditto...

Please find a new patch at the end...

   --- a/configure.in
   +++ b/configure.in
   @@ -386,6 +386,19 @@ if test x$WITH_LIBAUDIT != xno ; then
fi
if test ! -z $HAVE_AUDIT_TTY_STATUS ; then
AC_DEFINE([HAVE_AUDIT_TTY_STATUS], 1, [Define to 1 if struct 
   audit_tty_status exists.])
   +
   +AC_CHECK_MEMBER(
   +[struct audit_tty_status.log_passwd],
   +[
   + HAVE_AUDIT_TTY_STATUS_LOG_PASSWD=yes
   + 
   AC_DEFINE([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD], 1, [Define to 1 if struct 
   audit_tty_status.log_passwd exists.])
   +],
   +[
   + HAVE_AUDIT_TTY_STATUS_LOG_PASSWD=
   + AC_MSG_WARN([The struct 
   audit_tty_status.log_passwd member is needed for the log_passwd option.  
   The log_passwd option is disabled.])
   +],
   +[[#include libaudit.h]]
   +)
fi
else
 LIBAUDIT=
   @@ -393,6 +406,8 @@ fi
AC_SUBST(LIBAUDIT)
AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS],
[test x$HAVE_AUDIT_TTY_STATUS = xyes])
   +AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD],
   +[test x$HAVE_AUDIT_TTY_STATUS_LOG_PASSWD = xyes])
  
  There is a shorter way to express this idea:
  
  AC_CHECK_MEMBER([struct audit_tty_status.log_passwd], [],
  AC_MSG_WARN([audit_tty_status.log_passwd is not available, 
  log_passwd option disabled.],
  [[#include libaudit.h]])
  ...
  AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD],
 [test x$ac_cv_member_audit_tty_status_log_passwd = xyes])
 
 Ok, so $ac_cv_member_audit_tty_status_log_passwd is set by
 AC_CHECK_MEMBER()?

Ok, I've used your suggestion, but switching to AC_CHECK_MEMBERS() (and
balancing the AC_MSG_WARN() parens).

   --- a/modules/pam_tty_audit/Makefile.am
   +++ b/modules/pam_tty_audit/Makefile.am
   @@ -16,6 +16,9 @@ XMLS = README.xml pam_tty_audit.8.xml
securelibdir = $(SECUREDIR)

AM_CFLAGS = -I$(top_srcdir)/libpam/include 
   -I$(top_srcdir)/libpamc/include
   +if HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
   +  AM_CFLAGS += -DHAVE_AUDIT_TTY_STATUS_LOG_PASSWD
   +endif
  
  This shouldn't be needed because of the side effect of AC_CHECK_MEMBER.
 
 I don't follow.  I found I needed this latter one because
 HAVE_AUDIT_TTY_STATUS_LOG_PASSWD wasn't being propagated to gcc when
 compiling the C module.

Removed due to feature/side effect of AC_CHECK_MEMBERS().

   +  else if (strcmp (argv[i], log_passwd) == 0)
   +#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
   +log_passwd = 1;
   +#else /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
   +pam_syslog (pamh, LOG_WARNING,
   +pam_tty_audit: The log_passwd option was not 
   available at compile time.);
  
  No need to prefix syslog messages with the module name,
  you can rely on pam_syslog.
 
 Thanks.

Removed.

   +#warning pam_tty_audit: The log_passwd option is not available.  Please 
   upgrade your kernel.
  
  I'm not sure the wording is correct: it's headers not the kernel
  that is subject of the configure check.
 
 I pondered this wording.  I originally used the header wording, but
 thought it better to refer to the kernel, presuming the header would be
 upgraded with a capable kernel.

I've changed the wording to Please upgrade your headers/kernel.

Thanks for your feedback Dmitry.

  -- 
  ldv

From: Richard Guy Briggs r...@redhat.com
Date: Thu, 21 Mar 2013 00:56:51 -0400
Subject: [PATCH] pam_tty_audit: add an option to control logging of passwords: 
log_passwd

Most commands are entered one line at a time and processed as complete lines
in non-canonical mode.  Commands that interactively require a password, enter
canonical mode with echo set to off to do this.  This feature (icanon and
!echo) can be used to avoid logging passwords by audit while still logging the
rest of the command.

Adding a member to the struct audit_tty_status passed in by pam_tty_audit
allows control of logging passwords per task

Re: Pam_tty_audit and passwords

[Richard Guy Briggs]

On Mon, Jun 17, 2013 at 11:56:01AM -0500, John C. A. Bambenek, GCIH, CISSP 
wrote:
 When is approximate timeframe of that functionality to be available to
 suppress logging of passwords in an RH provided package?

Since I'm new to the process, but I pulled it together, I'll take a stab
at answering this, but defer to others with more experience on the
process timing.

The kernel bits are upstream in the 3.10-rc1 kernel as of May 11th.
That would still need to be backported to a RHEL kernel.  It is more
likely to end up in Fedora first.  Now that you mention it, that is
something I should be able to backport to RHEL.

The userspace bits in pam just had another patch sent upstream last week
and I don't expect any issues arising from that.  There is a bz filed to
add it to RHEL7.  Tomas Mraz should be able to provide more information
about that timing.  I assume he would also be able to answer questions
about RHEL6 as well.  That discussion and review of that patch is on the
pam-develop...@lists.fedorahosted.org list.


 Thanks,
 J

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: auditd compilation..

[Richard Guy Briggs]

On Wed, Jun 26, 2013 at 10:33:36AM +0530, Shinoj Gangadharan wrote:
 Hi,
 
 I am trying to build the auditd rpm from the spec file included in
 audit-2.3.1.tar.gz but it has a dependency on system-units . I could not
 find system-units for RHEL6. Can you please let me know from where I can
 get this. I need the latest version of auditd as I believe it has an option
 to exclude password from logs.

If this is what I think you are seeking, what you are seeking is not in
auditd.

Are you talking about this?:
https://bugzilla.redhat.com/show_bug.cgi?id=725100
https://bugzilla.redhat.com/show_bug.cgi?id=966166
https://bugzilla.redhat.com/show_bug.cgi?id=976033

If so, the kernel bits are in upstream v3.10-rc1.  They are in the
process of being backported to rhel6.5.

The userspace bits are in upstream pam, but I've just discovered that is
broken.  I have a patch to fix it, which I'm about to submit, but I just
have to verify it.  I am also in the process of backporting the original
patch with fix to rhel6.5.

 Regards,
 
 Shinoj.


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: audit on the future execution of a binary.

[Richard Guy Briggs]

;
 + case AUDIT_EXE_CHILDREN:
 + {
 + struct task_struct *ptsk;
 + for (ptsk = tsk;
 +  ptsk-parent-pid  0;
 +  ptsk = find_task_by_vpid(ptsk-parent-pid)) {
 + if (audit_match_exe(ptsk, f)) {
 + ++result;
 + break;
 + }
 + }
 + }
 + break;
   case AUDIT_UID:
   result = audit_comparator(cred-uid, f-op, f-val);
   break;
 -- 
 1.7.7.3

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH] audit: update AUDIT_INODE filter rule to comparator function

[Richard Guy Briggs]

It appears this one comparison function got missed in f368c07d (and 9c937dcc).

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/auditsc.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3c8a601..cb23f7d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -566,7 +566,7 @@ static int audit_filter_rules(struct task_struct *tsk,
break;
case AUDIT_INODE:
if (name)
-   result = (name-ino == f-val);
+   result = audit_comparator(name-ino, f-op, 
f-val);
else if (ctx) {
list_for_each_entry(n, ctx-names_list, list) {
if (audit_comparator(n-ino, f-op, 
f-val)) {
-- 
1.7.1

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: audit on the future execution of a binary.

[Richard Guy Briggs]

On Sun, Jul 07, 2013 at 03:41:41PM -0700, Peter Moody wrote:
 
 On Wed, Jul 03 2013 at 19:48, Richard Guy Briggs wrote:
  On Thu, Aug 23, 2012 at 12:24:00PM -0700, Peter Moody wrote:
  This adds the ability audit the actions of a not-yet-running process,
  as well as the children of a not-yet-running process.
 
  Hi Peter,
 
  I've gone back over the discussion of this feature and some of the
  background in the past couple of years on this list...
 
  We've got a kernel deadline coming up in the next month if we want to
  get something included in RHEL7 if you have the interest and time to
  evolve this patch (the userspace patch can follow...).
 
  As has been discussed, passing in an inode reference is incomplete,
  since it would need to be qualified by a device reference at minimum.
  And even then, it isn't atomic and could change by the time the kernel
  even sees this rule request.
 
  So, the next step is to convert the path to a device/inode in the kernel.  
  If
  this is done at the time of registering the filter rule, if/when the
  rule is invalidated then the rule would be dropped, logged.  It also
  means that anything else also hardlinked to it would be acted upon.
 
  Going one step further, if instead we can arrange an fsnotify() hook on
  rule registration, we could act on that path when it is executed,
  renamed, unlinked (and destroyed if the refcount goes to zero), etc.
 
  So, it should be passed as a path, logging the rule addition with path
  only at first.  When the rule is triggered then log the requested path,
  effective path, device/inode along with the user context.
 
  The user, carefully crafting other rules can give other information.
 
  A watch on the containing directory (/usr/bin) could help in case that
  executable pathname disappears and re-appears since the containing
  directory is less likely to go away, but it will be noisy.
 
  Does all this make sense?
 
 Hey Richard,
 
 Sorry for the late reply, we had a short week last week.

No worries.

 This makes a lot of sense, yes. Unfortunately I think it's unlikely that
 I'll have a chance to work on this in time for your freeze b/c my wife
 is due on Friday and as much as I'd like to thin that I'll be able to
 get some free time during paternity leave to do some kernel hacking,
 everyone tells me I'm crazy to think that.

A bit delusional, yes.  First child, I gather.  Welcome to parenting.
It is quite a ride.  It can be a lot of fun.  :)

 I *think* I'm the only one who's been asking for this feature, so
 hopefully my not getting to it won't be putting anyone out.

What's your timeline and availability?

 Cheers,
 peter
 
  Let's deal later with namespaces, containers, mounts, chroots, bind
  mounts, etc...

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH] audit: listen in all network namespaces

[Richard Guy Briggs]

Convert audit from only listening in init_net to use register_pernet_subsys()
to dynamically manage the netlink socket list.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |   64 ++-
 kernel/audit.h |4 +++
 2 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..06e2676 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -64,6 +64,7 @@
 #include linux/freezer.h
 #include linux/tty.h
 #include linux/pid_namespace.h
+#include net/netns/generic.h
 
 #include audit.h
 
@@ -122,6 +123,7 @@ static atomic_taudit_lost = ATOMIC_INIT(0);
 
 /* The netlink socket. */
 static struct sock *audit_sock;
+int audit_net_id;
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
@@ -391,6 +393,7 @@ static void kauditd_send_skb(struct sk_buff *skb)
printk(KERN_ERR audit: *NO* daemon at audit_pid=%d\n, 
audit_pid);
audit_log_lost(auditd disappeared\n);
audit_pid = 0;
+   audit_sock = NULL;
/* we might get lucky and get this in the next auditd */
audit_hold_skb(skb);
} else
@@ -474,13 +477,15 @@ int audit_send_list(void *_dest)
struct audit_netlink_list *dest = _dest;
int pid = dest-pid;
struct sk_buff *skb;
+   struct net *net = get_net_ns_by_pid(pid);
+   struct audit_net *aunet = net_generic(net, audit_net_id);
 
/* wait for parent to finish and send an ACK */
mutex_lock(audit_cmd_mutex);
mutex_unlock(audit_cmd_mutex);
 
while ((skb = __skb_dequeue(dest-q)) != NULL)
-   netlink_unicast(audit_sock, skb, pid, 0);
+   netlink_unicast(aunet-nlsk, skb, pid, 0);
 
kfree(dest);
 
@@ -515,13 +520,15 @@ out_kfree_skb:
 static int audit_send_reply_thread(void *arg)
 {
struct audit_reply *reply = (struct audit_reply *)arg;
+   struct net *net = get_net_ns_by_pid(reply-pid);
+   struct audit_net *aunet = net_generic(net, audit_net_id);
 
mutex_lock(audit_cmd_mutex);
mutex_unlock(audit_cmd_mutex);
 
/* Ignore failure. It'll only happen if the sender goes away,
   because our timeout is set to infinite. */
-   netlink_unicast(audit_sock, reply-skb, reply-pid, 0);
+   netlink_unicast(aunet-nlsk , reply-skb, reply-pid, 0);
kfree(reply);
return 0;
 }
@@ -690,6 +697,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
audit_log_config_change(audit_pid, new_pid, 
audit_pid, 1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
+   audit_sock = NETLINK_CB(skb).sk;
}
if (status_get-mask  AUDIT_STATUS_RATE_LIMIT) {
err = audit_set_rate_limit(status_get-rate_limit);
@@ -886,24 +894,58 @@ static void audit_receive(struct sk_buff  *skb)
mutex_unlock(audit_cmd_mutex);
 }
 
-/* Initialize audit support at boot time. */
-static int __init audit_init(void)
+static int __net_init audit_net_init(struct net *net)
 {
-   int i;
struct netlink_kernel_cfg cfg = {
.input  = audit_receive,
};
 
+   struct audit_net *aunet = net_generic(net, audit_net_id);
+
+   pr_info(audit: initializing netlink socket in namespace\n);
+
+   aunet-nlsk = netlink_kernel_create(net, NETLINK_AUDIT, cfg);
+   if (aunet-nlsk == NULL)
+   return -ENOMEM;
+   if (!aunet-nlsk)
+   audit_panic(cannot initialize netlink socket in namespace);
+   else
+   aunet-nlsk-sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
+   return 0;
+}
+
+static void __net_exit audit_net_exit(struct net *net)
+{
+   struct audit_net *aunet = net_generic(net, audit_net_id);
+   struct sock *sock = aunet-nlsk;
+   if (sock == audit_sock) {
+   audit_pid = 0;
+   audit_sock = NULL;
+   }
+
+   rcu_assign_pointer(aunet-nlsk, NULL);
+   synchronize_net();
+   netlink_kernel_release(sock);
+}
+
+static struct pernet_operations __net_initdata audit_net_ops = {
+   .init = audit_net_init,
+   .exit = audit_net_exit,
+   .id = audit_net_id,
+   .size = sizeof(struct audit_net),
+};
+
+/* Initialize audit support at boot time. */
+static int __init audit_init(void)
+{
+   int i;
+
if (audit_initialized == AUDIT_DISABLED)
return 0;
 
-   printk(KERN_INFO audit: initializing netlink socket (%s)\n,
+   pr_info(audit: initializing netlink subsys (%s)\n,
   audit_default ? enabled : disabled);
-   audit_sock = netlink_kernel_create(init_net, NETLINK_AUDIT, cfg);
-   if (!audit_sock)
-   audit_panic(cannot initialize netlink socket);
-   else

[PATCH] audit: restore order of tty and ses fields in log output

[Richard Guy Briggs]

When being refactored from audit_log_start() to audit_log_task_info(), in
commit e23eb920 the tty and ses fields in the log output got transposed.
Restore to original order to avoid breaking search tools.

Cc: sta...@vger.kernel.org # v3.6
Cc: Eric Paris epa...@redhat.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..63b2dd5 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1590,7 +1590,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct 
task_struct *tsk)
audit_log_format(ab,
  ppid=%ld pid=%d auid=%u uid=%u gid=%u
  euid=%u suid=%u fsuid=%u
- egid=%u sgid=%u fsgid=%u ses=%u tty=%s,
+ egid=%u sgid=%u fsgid=%u tty=%s ses=%u,
 sys_getppid(),
 tsk-pid,
 from_kuid(init_user_ns, audit_get_loginuid(tsk)),
@@ -1602,7 +1602,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct 
task_struct *tsk)
 from_kgid(init_user_ns, cred-egid),
 from_kgid(init_user_ns, cred-sgid),
 from_kgid(init_user_ns, cred-fsgid),
-audit_get_sessionid(tsk), tty);
+tty, audit_get_sessionid(tsk));
 
get_task_comm(name, tsk);
audit_log_format(ab,  comm=);
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH] kaudit: prevent an older auditd shutdown from orphaning a newer auditd startup

[Richard Guy Briggs]

There have been reports of auditd restarts resulting in kaudit not being able
to find a newly registered auditd.  It results in reports such as:
kernel: [ 2077.233573] audit: *NO* daemon at audit_pid=1614
kernel: [ 2077.234712] audit: audit_lost=97 audit_rate_limit=0 
audit_backlog_limit=320
kernel: [ 2077.234718] audit: auditd disappeared
(previously mis-spelled dissapeared)

One possible cause is a race between the shutdown of an older auditd and a
newer one.  If the newer one sets the daemon pid to itself in kauditd before
the older one has cleared the daemon pid, the newer daemon pid will be erased.
This could be caused by an automated system, or by manual intervention, but in
either case, there is no use in having the older daemon clear the daemon pid
reference since its old pid is no longer being referenced.  This patch will
prevent that specific case, returning an error of EACCES.

The case for preventing a newer auditd from registering itself if there is an
existing auditd is a more difficult case that is beyond the scope of this
patch.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..7cb7486 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -686,6 +686,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (status_get-mask  AUDIT_STATUS_PID) {
int new_pid = status_get-pid;
 
+   if ((!new_pid)  (task_tgid_vnr(current) != audit_pid))
+   return -EACCES;
if (audit_enabled != AUDIT_OFF)
audit_log_config_change(audit_pid, new_pid, 
audit_pid, 1);
audit_pid = new_pid;
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: listen in all network namespaces

[Richard Guy Briggs]

On Wed, Jul 17, 2013 at 11:54:21AM +0800, Gao feng wrote:
 Hi, Richard
 
 On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
  Convert audit from only listening in init_net to use 
  register_pernet_subsys()
  to dynamically manage the netlink socket list.
  
  Signed-off-by: Richard Guy Briggs r...@redhat.com
  ---
 
 Right now audit still can't be used in uninit pid/user namespace,
 Consider this, when user in uninit pid/user namespace is allowed
 to setup/run audit subsystem, since the kernel thread always runs
 in init pid namespace, so we can't get right net namespace through
 get_net_ns_by_pid, The audit information will be sent to incorrect
 net namespace by kernel thread.
 
 In my opinion, This patch is limited and nonextensile.
 
 Maybe you should check the patchset [Part1 PATCH 00/22] Add namespace 
 support for audit
 I sent in 06/19/2013, In my solution, audit kernel side netlink sockets 
 belongs
 to user namespace, and the user space audit netlink sockets will find the 
 audit
 kernel socket through current_net_ns()-user_ns-audit.sock.

I already looked at your 48-patch and 22-patch sets and the threads of
comments.  The concerns expressed in that thread haven't been fully
addressed yet by you.

 The [PATCH 04/22] netlink: Add compare function for netlink_table of this 
 patchset
 has been merged in linux mainline. I think if you look at my patchset, you 
 will find
 the [PATCH 03/22] and [PATCH 05/22] will achieve the same aim of your patch.

I don't have any specific issues with patch 04/22.

For patch 05/22, I would have just stopped with comparing the two net
namespace pointers.

As for patch 03/22...

The init user namespace doesn't have a one-to-one mapping to network
namespace, so this won't solve the problem I was trying to solve.

In the initial user namespace, I can have as many network namespaces as
I want.  I want kaudit to listen in all of them.  There is already a
conservative check to make sure that audit won't permit changes from
any non-initial user namespace (or pid space):
kernel/audit.c:583:audit_netlink_ok():
if ((current_user_ns() != init_user_ns) ||
(task_active_pid_ns(current) != init_pid_ns))
return -EPERM;
This check needs to be revisited to allow some loosening of this policy,
but it was sound to start off too restrictive.
(https://bugzilla.redhat.com/show_bug.cgi?id=947530)

The certification issues surrounding non-initial user namespaces haven't
been adequately resolved yet, not having yet seen a followup patchset,
so we can combine these ideas once those issues have been addressed.

I agree we will need to be careful how the specific target socket and
portid are selected once we end up in other pid namespaces.  For now,
are there specific concerns with this patch or better ways to
future-proof the selection of kaudit sockets and portids?

 Thanks!

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: listen in all network namespaces

[Richard Guy Briggs]

On Mon, Jul 22, 2013 at 11:20:57AM +0800, Gao feng wrote:
 On 07/20/2013 05:15 AM, Richard Guy Briggs wrote:
  On Wed, Jul 17, 2013 at 11:54:21AM +0800, Gao feng wrote:
  Hi, Richard
 
  On 07/17/2013 04:32 AM, Richard Guy Briggs wrote:
  Convert audit from only listening in init_net to use 
  register_pernet_subsys()
  to dynamically manage the netlink socket list.
 
  Signed-off-by: Richard Guy Briggs r...@redhat.com
  ---
 
  Right now audit still can't be used in uninit pid/user namespace,
  Consider this, when user in uninit pid/user namespace is allowed
  to setup/run audit subsystem, since the kernel thread always runs
  in init pid namespace, so we can't get right net namespace through
  get_net_ns_by_pid, The audit information will be sent to incorrect
  net namespace by kernel thread.
 
  In my opinion, This patch is limited and nonextensile.
 
  Maybe you should check the patchset [Part1 PATCH 00/22] Add namespace 
  support for audit
  I sent in 06/19/2013, In my solution, audit kernel side netlink sockets 
  belongs
  to user namespace, and the user space audit netlink sockets will find the 
  audit
  kernel socket through current_net_ns()-user_ns-audit.sock.
  
  I already looked at your 48-patch and 22-patch sets and the threads of
  comments.  The concerns expressed in that thread haven't been fully
  addressed yet by you.
  
 
 Sorry, I think I had addressed all the problems in thar thread, maybe I missed
 some, please help me to point it out, fell free to keep on discussing with me
 in that thread.

There are several branches to that thread that went unresolved.  I
haven't seen a followup patchset that attempts to address them:

https://www.redhat.com/archives/linux-audit/2013-June/msg00046.html
https://www.redhat.com/archives/linux-audit/2013-June/msg00056.html
https://www.redhat.com/archives/linux-audit/2013-June/msg00048.html
https://www.redhat.com/archives/linux-audit/2013-June/msg00050.html

But coming back to Eric Paris' original response and subsequent example,
neither have been addressed adequately:
https://www.redhat.com/archives/linux-audit/2013-June/msg00035.html
https://www.redhat.com/archives/linux-audit/2013-June/msg00039.html

and neither has the concern about making LSPP certification impossible.

  The [PATCH 04/22] netlink: Add compare function for netlink_table of 
  this patchset
  has been merged in linux mainline. I think if you look at my patchset, you 
  will find
  the [PATCH 03/22] and [PATCH 05/22] will achieve the same aim of your 
  patch.
  
  I don't have any specific issues with patch 04/22.
  
  For patch 05/22, I would have just stopped with comparing the two net
  namespace pointers.
  
  As for patch 03/22...
  
  The init user namespace doesn't have a one-to-one mapping to network
  namespace, so this won't solve the problem I was trying to solve.
 
 If your problem is auditctl is unavailable in uninit net namespace, I
 think my solution can solve this problem, since two audit netlink sockets
 can communicate with each other when the net namespaces they belong to are
 created by the same user namespace.

I don't follow how this is possible.

 Maybe I misunderstand what is your problem here.
 
  In the initial user namespace, I can have as many network namespaces as
  I want.  I want kaudit to listen in all of them.  There is already a
  conservative check to make sure that audit won't permit changes from
  any non-initial user namespace (or pid space):
  kernel/audit.c:583:audit_netlink_ok():
  if ((current_user_ns() != init_user_ns) ||
  (task_active_pid_ns(current) != init_pid_ns))
  return -EPERM;
  This check needs to be revisited to allow some loosening of this policy,
  but it was sound to start off too restrictive.
  (https://bugzilla.redhat.com/show_bug.cgi?id=947530)
 
 Yes, it was too restrictive, but I can't see what the help from this patch to
 solve this problem.

It hasn't been solved yet.  It is one of the next in line.

  The certification issues surrounding non-initial user namespaces haven't
  been adequately resolved yet, not having yet seen a followup patchset,
  so we can combine these ideas once those issues have been addressed.
  
  I agree we will need to be careful how the specific target socket and
  portid are selected once we end up in other pid namespaces.  For now,
  are there specific concerns with this patch or better ways to
  future-proof the selection of kaudit sockets and portids?
 
 I my solution, even there are many net namespaces belong to the same user 
 namespace,
 there will only be one audit kernel side netlink socket, so all of the user 
 space
 audit netlink sockets in these net namespaces will find out/communicate with 
 this
 kernel audit socket.

I will need to go back and have a second look to see how this works.

 and the kaudit sockets, portid belong to the user namespace,they are the one 
 and only
 in each user

Re: [PATCH] audit: printk USER_AVC messages when audit isn't enabled

[Richard Guy Briggs]

On Thu, Jul 25, 2013 at 06:02:55PM -0700, Tyler Hicks wrote:
 When the audit=1 kernel parameter is absent and auditd is not running,
 AUDIT_USER_AVC messages are being silently discarded.
 
 AUDIT_USER_AVC messages should be sent to userspace using printk(), as
 mentioned in the commit message of
 4a4cd633b575609b741a1de7837223a2d9e1c34c (AUDIT: Optimise the
 audit-disabled case for discarding user messages).
 
 When audit_enabled is 0, audit_receive_msg() discards all user messages
 except for AUDIT_USER_AVC messages. However, audit_log_common_recv_msg()
 refuses to allocate an audit_buffer if audit_enabled is 0. The fix is to
 special case AUDIT_USER_AVC messages in both functions.
 
 Signed-off-by: Tyler Hicks tyhi...@canonical.com
 Cc: Al Viro v...@zeniv.linux.org.uk
 Cc: Eric Paris epa...@redhat.com
 Cc: linux-audit@redhat.com
 ---
 
 It looks like commit 50397bd1e471391d27f64efad9271459c913de87 ([AUDIT] clean
 up audit_receive_msg()) introduced this bug, so I think that this patch 
 should
 also get the tag:
 
   Cc: sta...@kernel.org # v2.6.25+
 
 Al and Eric, I'll leave that up to you two.

Hi Tyler,

This patch looks entirely reasonable to me.

Acked-by: Richard Guy Briggs rbri...@redhat.com

 Here's my test matrix showing where messages end up as a result of a call to
 libaudit's audit_log_user_avc_message():
 
   |   unpatched   patched
 +
 w/o audit=1  |   *dropped*   syslog
 w/o auditd|
   |
 w/ audit=1   |   syslog  syslog
 w/o auditd|
   |
 w/o audit=1  |   audit.log   audit.log
 w/ auditd |
   |
 w/ audit=1   |   audit.log   audit.log
 w/ auditd |
 
 Thanks!
 
  kernel/audit.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/kernel/audit.c b/kernel/audit.c
 index 91e53d0..f4f2773 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -613,7 +613,7 @@ static int audit_log_common_recv_msg(struct audit_buffer 
 **ab, u16 msg_type)
   int rc = 0;
   uid_t uid = from_kuid(init_user_ns, current_uid());
  
 - if (!audit_enabled) {
 + if (!audit_enabled  msg_type != AUDIT_USER_AVC) {
   *ab = NULL;
   return rc;
   }
 -- 
 1.8.3.2
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 08/12] audit: anchor all pid references in the initial pid namespace

[Richard Guy Briggs]

Store and log all PIDs with reference to the initial PID namespace and
deprecate the use of the error-prone duplicity of task-pid and task-tgid.

Still only permit the audit logging daemon and control to operate from the
initial PID namespace, but allow processes to log from another PID namespace.

Cc: Eric W. Biederman ebied...@xmission.com
(informed by ebiederman's c776b5d2)

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 drivers/tty/tty_audit.c  |3 ++-
 kernel/audit.c   |   15 ++-
 kernel/auditfilter.c |   17 -
 kernel/auditsc.c |   16 +---
 security/apparmor/audit.c|2 +-
 security/integrity/integrity_audit.c |2 +-
 security/lsm_audit.c |   11 +++
 security/tomoyo/audit.c  |2 +-
 8 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index a4fdce7..a0ae01f 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -65,6 +65,7 @@ static void tty_audit_log(const char *description, int major, 
int minor,
 {
struct audit_buffer *ab;
struct task_struct *tsk = current;
+   pid_t pid = task_pid_nr_init_ns(tsk);
uid_t uid = from_kuid(init_user_ns, task_uid(tsk));
uid_t loginuid = from_kuid(init_user_ns, audit_get_loginuid(tsk));
u32 sessionid = audit_get_sessionid(tsk);
@@ -74,7 +75,7 @@ static void tty_audit_log(const char *description, int major, 
int minor,
char name[sizeof(tsk-comm)];
 
audit_log_format(ab, %s pid=%u uid=%u auid=%u ses=%u major=%d
- minor=%d comm=, description, tsk-pid, uid,
+ minor=%d comm=, description, pid, uid,
 loginuid, sessionid, major, minor);
get_task_comm(name, tsk);
audit_log_untrustedstring(ab, name);
diff --git a/kernel/audit.c b/kernel/audit.c
index fa1d5f5..8e4958b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -574,9 +574,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
msg_type)
 {
int err = 0;
 
-   /* Only support the initial namespaces for now. */
-   if ((current_user_ns() != init_user_ns) ||
-   (task_active_pid_ns(current) != init_pid_ns))
+   /* Only support initial user namespace for now. */
+   if ((current_user_ns() != init_user_ns))
return -EPERM;
 
switch (msg_type) {
@@ -594,6 +593,11 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
msg_type)
case AUDIT_TTY_SET:
case AUDIT_TRIM:
case AUDIT_MAKE_EQUIV:
+   /* Only support auditd and auditctl in initial pid namespace
+* for now. */
+   if ((task_active_pid_ns(current) != init_pid_ns))
+   return -EPERM;
+
if (!capable(CAP_AUDIT_CONTROL))
err = -EPERM;
break;
@@ -614,6 +618,7 @@ static int audit_log_common_recv_msg(struct audit_buffer 
**ab, u16 msg_type)
 {
int rc = 0;
uid_t uid = from_kuid(init_user_ns, current_uid());
+   pid_t pid = task_tgid_nr_init_ns(current);
 
if (!audit_enabled) {
*ab = NULL;
@@ -623,7 +628,7 @@ static int audit_log_common_recv_msg(struct audit_buffer 
**ab, u16 msg_type)
*ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return rc;
-   audit_log_format(*ab, pid=%d uid=%u, task_tgid_vnr(current), uid);
+   audit_log_format(*ab, pid=%d uid=%u, pid, uid);
audit_log_session_info(*ab);
audit_log_task_context(*ab);
 
@@ -1605,7 +1610,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct 
task_struct *tsk)
  euid=%u suid=%u fsuid=%u
  egid=%u sgid=%u fsgid=%u ses=%u tty=%s,
 task_ppid_nr_init_ns(tsk),
-tsk-pid,
+task_pid_nr_init_ns(tsk),
 from_kuid(init_user_ns, audit_get_loginuid(tsk)),
 from_kuid(init_user_ns, cred-uid),
 from_kgid(init_user_ns, cred-gid),
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 381d3de..2a60455 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -428,6 +428,19 @@ static struct audit_entry *audit_data_to_entry(struct 
audit_rule_data *data,
f-val = 0;
}
 
+   if ((f-type == AUDIT_PID) || (f-type == AUDIT_PPID)) {
+   struct pid *pid;
+   rcu_read_lock();
+   pid = find_vpid(f-val);
+   if (!pid) {
+   rcu_read_unlock();
+   err = -ESRCH;
+   goto

[PATCH 11/12] pid: rewrite task helper functions avoiding task-pid and task-tgid

[Richard Guy Briggs]

This stops these four task helper functions from using the deprecated and
error-prone task-pid and task-tgid.

(informed by ebiederman's ea5a4d01)
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/sched.h |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8e69807..46e739d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1579,7 +1579,7 @@ static inline int pid_alive(struct task_struct *p)
  */
 static inline int is_global_init(struct task_struct *tsk)
 {
-   return tsk-pid == 1;
+   return task_pid_nr(tsk) == 1;
 }
 
 extern struct pid *cad_pid;
@@ -1930,7 +1930,7 @@ extern struct task_struct *idle_task(int cpu);
  */
 static inline bool is_idle_task(const struct task_struct *p)
 {
-   return p-pid == 0;
+   return task_pid(p) == init_struct_pid;
 }
 extern struct task_struct *curr_task(int cpu);
 extern void set_curr_task(int cpu, struct task_struct *p);
@@ -2203,13 +2203,13 @@ static inline bool thread_group_leader(struct 
task_struct *p)
  */
 static inline int has_group_leader_pid(struct task_struct *p)
 {
-   return p-pid == p-tgid;
+   return task_pid(p) == task_tgid(p);
 }
 
 static inline
 int same_thread_group(struct task_struct *p1, struct task_struct *p2)
 {
-   return p1-tgid == p2-tgid;
+   return task_tgid(p1) == task_tgid(p2);
 }
 
 static inline struct task_struct *next_thread(const struct task_struct *p)
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 00/12] RFC: steps to make audit pid namespace-safe

[Richard Guy Briggs]

This patchset is a revival of some of Eric Biederman's work to make audit
pid-namespace-safe.

In a couple of places, audit was printing PIDs in the task's pid namespace
rather than relative to the audit daemon's pid namespace, which currently is
init_pid_ns.

It also allows processes to log audit user messages in their own pid
namespaces, which was not previously permitted.  Please see:
https://bugzilla.redhat.com/show_bug.cgi?id=947530
https://bugs.launchpad.net/ubuntu/+source/vsftpd/+bug/1160372
https://bugzilla.novell.com/show_bug.cgi?id=786024

Part of the cleanup here involves deprecating task-pid and task-tgid, which
are error-prone duplicates of the task-pids structure

The next step which I hope to add to this patchset will be to purge task-pid
and task-tgid from the rest of the kernel if possible.  Once that is done,
task_pid_nr_init_ns() and task_tgid_nr_init_ns() that were introduced in patch
05/12 and used in patches 06/12 and 08/12 could be replaced with task_pid_nr()
and task_tgid_nr().  Eric B. did take a stab at that, but checking all the
subtleties will be non-trivial.

Does anyone have any opinions or better yet hard data on cache line misses
between pid_nr(struct pid*) and pid_nr_ns(struct pid*, init_pid_ns)?  I'd
like to see pid_nr() use pid_nr_ns(struct pid*, init_pid_ns), or
pid_nr_init_ns() eliminated in favour of the original pid_nr().  pid_nr()
currently accesses the first level of the pid structure without having to
dereference the level number.  If there is an actual speed difference, it could
be worth keeping, otherwise, I'd prefer to simplify that code.

Eric also had a patch to add a printk option to format a struct pid pointer
which was PID namespace-aware.  I don't see the point, but I'll let him explain
it.

Discuss.

Eric W. Biederman (5):
  audit: Kill the unused struct audit_aux_data_capset
  audit: Simplify and correct audit_log_capset

Richard Guy Briggs (7):
  audit: fix netlink portid naming and types
  pid: get ppid pid_t of task in init_pid_ns safely
  audit: convert PPIDs to the inital PID namespace.
  pid: get pid_t of task in init_pid_ns correctly
  audit: store audit_pid as a struct pid pointer
  audit: anchor all pid references in the initial pid namespace
  pid: modify task_pid_nr to work without task-pid.
  pid: modify task_tgid_nr to work without task-tgid.
  pid: rewrite task helper functions avoiding task-pid and task-tgid
  pid: mark struct task const in helper functions

 drivers/tty/tty_audit.c  |3 +-
 include/linux/audit.h|8 ++--
 include/linux/pid.h  |6 +++
 include/linux/sched.h|   81 --
 kernel/audit.c   |   76 +++
 kernel/audit.h   |   12 +++---
 kernel/auditfilter.c |   35 +++
 kernel/auditsc.c |   36 ++-
 kernel/capability.c  |2 +-
 kernel/pid.c |4 +-
 security/apparmor/audit.c|7 +--
 security/integrity/integrity_audit.c |2 +-
 security/lsm_audit.c |   11 +++--
 security/tomoyo/audit.c  |2 +-
 14 files changed, 177 insertions(+), 108 deletions(-)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 12/12] pid: mark struct task const in helper functions

[Richard Guy Briggs]

It doesn't make any sense to recallers to pass in a non-const struct
task so update the function signatures to only require a const struct
task.

(informed by ebiederman's c76b2526)
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/sched.h |   44 ++--
 kernel/pid.c  |4 ++--
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 46e739d..a585b51 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1423,12 +1423,12 @@ static inline void set_numabalancing_state(bool enabled)
 }
 #endif
 
-static inline struct pid *task_pid(struct task_struct *task)
+static inline struct pid *task_pid(const struct task_struct *task)
 {
return task-pids[PIDTYPE_PID].pid;
 }
 
-static inline struct pid *task_tgid(struct task_struct *task)
+static inline struct pid *task_tgid(const struct task_struct *task)
 {
return task-group_leader-pids[PIDTYPE_PID].pid;
 }
@@ -1438,12 +1438,12 @@ static inline struct pid *task_tgid(struct task_struct 
*task)
  * the result of task_pgrp/task_session even if task == current,
  * we can race with another thread doing sys_setsid/sys_setpgid.
  */
-static inline struct pid *task_pgrp(struct task_struct *task)
+static inline struct pid *task_pgrp(const struct task_struct *task)
 {
return task-group_leader-pids[PIDTYPE_PGID].pid;
 }
 
-static inline struct pid *task_session(struct task_struct *task)
+static inline struct pid *task_session(const struct task_struct *task)
 {
return task-group_leader-pids[PIDTYPE_SID].pid;
 }
@@ -1468,50 +1468,50 @@ struct pid_namespace;
  *
  * see also pid_nr() etc in include/linux/pid.h
  */
-pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
+pid_t __task_pid_nr_ns(const struct task_struct *task, enum pid_type type,
struct pid_namespace *ns);
 
-static inline pid_t task_pid_nr(struct task_struct *tsk)
+static inline pid_t task_pid_nr(const struct task_struct *tsk)
 {
return pid_nr(task_pid(tsk));
 }
 
-static inline pid_t task_pid_nr_ns(struct task_struct *tsk,
+static inline pid_t task_pid_nr_ns(const struct task_struct *tsk,
struct pid_namespace *ns)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_PID, ns);
 }
 
-static inline pid_t task_pid_nr_init_ns(struct task_struct *tsk)
+static inline pid_t task_pid_nr_init_ns(const struct task_struct *tsk)
 {
return task_pid_nr_ns(tsk, init_pid_ns);
 }
 
-static inline pid_t task_pid_vnr(struct task_struct *tsk)
+static inline pid_t task_pid_vnr(const struct task_struct *tsk)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_PID, NULL);
 }
 
 
-static inline pid_t task_tgid_nr(struct task_struct *tsk)
+static inline pid_t task_tgid_nr(const struct task_struct *tsk)
 {
return pid_nr(task_tgid(tsk));
 }
 
-pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns);
+pid_t task_tgid_nr_ns(const struct task_struct *tsk, struct pid_namespace *ns);
 
-static inline pid_t task_tgid_nr_init_ns(struct task_struct *tsk)
+static inline pid_t task_tgid_nr_init_ns(const struct task_struct *tsk)
 {
return task_tgid_nr_ns(tsk, init_pid_ns);
 }
 
-static inline pid_t task_tgid_vnr(struct task_struct *tsk)
+static inline pid_t task_tgid_vnr(const struct task_struct *tsk)
 {
return pid_vnr(task_tgid(tsk));
 }
 
 
-static inline pid_t task_ppid_nr_ns(struct task_struct *tsk,
+static inline pid_t task_ppid_nr_ns(const struct task_struct *tsk,
struct pid_namespace *ns)
 {
pid_t pid;
@@ -1523,37 +1523,37 @@ static inline pid_t task_ppid_nr_ns(struct task_struct 
*tsk,
return pid;
 }
 
-static inline pid_t task_ppid_nr_init_ns(struct task_struct *tsk)
+static inline pid_t task_ppid_nr_init_ns(const struct task_struct *tsk)
 {
return task_ppid_nr_ns(tsk, init_pid_ns);
 }
 
 
-static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk,
+static inline pid_t task_pgrp_nr_ns(const struct task_struct *tsk,
struct pid_namespace *ns)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_PGID, ns);
 }
 
-static inline pid_t task_pgrp_vnr(struct task_struct *tsk)
+static inline pid_t task_pgrp_vnr(const struct task_struct *tsk)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_PGID, NULL);
 }
 
 
-static inline pid_t task_session_nr_ns(struct task_struct *tsk,
+static inline pid_t task_session_nr_ns(const struct task_struct *tsk,
struct pid_namespace *ns)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_SID, ns);
 }
 
-static inline pid_t task_session_vnr(struct task_struct *tsk)
+static inline pid_t task_session_vnr(const struct task_struct *tsk)
 {
return __task_pid_nr_ns(tsk, PIDTYPE_SID, NULL);
 }
 
 /* obsolete, do not use */
-static inline pid_t

[PATCH 10/12] pid: modify task_tgid_nr to work without task-tgid.

[Richard Guy Briggs]

task-tgid is an error prone construct and results in duplicate maintenance.
Start it's demise by modifying task_tgid_nr to not use it.

Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/sched.h |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index fb09b93..8e69807 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1495,7 +1495,7 @@ static inline pid_t task_pid_vnr(struct task_struct *tsk)
 
 static inline pid_t task_tgid_nr(struct task_struct *tsk)
 {
-   return tsk-tgid;
+   return pid_nr(task_tgid(tsk));
 }
 
 pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns);
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 01/12] audit: Kill the unused struct audit_aux_data_capset

[Richard Guy Briggs]

From: Eric W. Biederman ebied...@xmission.com

Signed-off-by: Eric W. Biederman ebied...@xmission.com
(cherry picked from commit 6904431d6b41190e42d6b94430b67cb4e7e6a4b7)
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/auditsc.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9845cb3..8c644c5 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -121,12 +121,6 @@ struct audit_aux_data_bprm_fcaps {
struct audit_cap_data   new_pcap;
 };
 
-struct audit_aux_data_capset {
-   struct audit_aux_data   d;
-   pid_t   pid;
-   struct audit_cap_data   cap;
-};
-
 struct audit_tree_refs {
struct audit_tree_refs *next;
struct audit_chunk *c[31];
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 03/12] pid: get ppid pid_t of task in init_pid_ns safely

[Richard Guy Briggs]

Added the functions
task_ppid()
task_ppid_nr_ns()
task_ppid_nr_init_ns()
to safely abstract the lookup of the PPID (real_parent's tgid) of a process,
including rcu locking, in any required pid namespace.  This provides an
alternative to sys_getppid(), which is relative to the child process' pid
namespace.

(informed by ebiederman's 6c621b7e)
Cc: sta...@vger.kernel.org
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/sched.h |   23 +++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d722490..ec04090 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1448,6 +1448,11 @@ static inline struct pid *task_session(struct 
task_struct *task)
return task-group_leader-pids[PIDTYPE_SID].pid;
 }
 
+static inline struct pid *task_ppid(struct task_struct *task)
+{
+   return task_tgid(rcu_dereference(current-real_parent));
+}
+
 struct pid_namespace;
 
 /*
@@ -1496,6 +1501,24 @@ static inline pid_t task_tgid_vnr(struct task_struct 
*tsk)
 }
 
 
+static inline pid_t task_ppid_nr_ns(struct task_struct *tsk,
+   struct pid_namespace *ns)
+{
+   pid_t pid;
+
+   rcu_read_lock();
+   pid = pid_nr_ns(task_ppid(current), ns);
+   rcu_read_unlock();
+
+   return pid;
+}
+
+static inline pid_t task_ppid_nr_init_ns(struct task_struct *tsk)
+{
+   return task_ppid_nr_ns(tsk, init_pid_ns);
+}
+
+
 static inline pid_t task_pgrp_nr_ns(struct task_struct *tsk,
struct pid_namespace *ns)
 {
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 07/12] audit: store audit_pid as a struct pid pointer

[Richard Guy Briggs]

- PID will be reported in the relevant querying PID namespace.

- Refuse to change the current audit_pid if the new value does not
  point to an existing process.

- Refuse to set the current audit_pid if the new value is not its own PID
  (unless it is being unset).

- Convert audit_pid into the initial pid namespace for reports

(informed by ebiederman's 5bf431da)
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c   |   25 +++--
 kernel/audit.h   |4 ++--
 kernel/auditsc.c |6 +++---
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 75b0989..fa1d5f5 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -93,7 +93,7 @@ static intaudit_failure = AUDIT_FAIL_PRINTK;
  * contains the pid of the auditd process and audit_nlk_portid contains
  * the portid to use to send netlink messages to that process.
  */
-intaudit_pid;
+struct pid *audit_pid;
 static __u32   audit_nlk_portid;
 
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
@@ -388,9 +388,11 @@ static void kauditd_send_skb(struct sk_buff *skb)
err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
if (err  0) {
BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
-   printk(KERN_ERR audit: *NO* daemon at audit_pid=%d\n, 
audit_pid);
+   pr_err(audit: *NO* daemon at audit_pid=%d\n
+  , pid_nr_init_ns(audit_pid));
audit_log_lost(auditd disappeared\n);
-   audit_pid = 0;
+   put_pid(audit_pid);
+   audit_pid = NULL;
/* we might get lucky and get this in the next auditd */
audit_hold_skb(skb);
} else
@@ -661,7 +663,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
case AUDIT_GET:
status_set.enabled   = audit_enabled;
status_set.failure   = audit_failure;
-   status_set.pid   = audit_pid;
+   status_set.pid   = pid_vnr(audit_pid);
status_set.rate_limit= audit_rate_limit;
status_set.backlog_limit = audit_backlog_limit;
status_set.lost  = atomic_read(audit_lost);
@@ -684,10 +686,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
return err;
}
if (status_get-mask  AUDIT_STATUS_PID) {
-   int new_pid = status_get-pid;
+   struct pid *new_pid = find_get_pid(status_get-pid);
+   if (status_get-pid  !new_pid)
+   return -ESRCH;
+
+   /* check that non-zero pid it is trying to set is its
+* own*/
+   if (status_get-pid 
+   (status_get-pid != task_pid_vnr(current)))
+   return -EPERM;
 
if (audit_enabled != AUDIT_OFF)
-   audit_log_config_change(audit_pid, new_pid, 
audit_pid, 1);
+   audit_log_config_change(audit_pid,
+   pid_nr_init_ns(new_pid),
+   
pid_nr_init_ns(audit_pid),
+   1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
}
diff --git a/kernel/audit.h b/kernel/audit.h
index 36edcf5..3932a3b 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -218,7 +218,7 @@ extern void audit_log_name(struct audit_context *context,
   struct audit_names *n, struct path *path,
   int record_num, int *call_panic);
 
-extern int audit_pid;
+extern struct pid *audit_pid;
 
 #define AUDIT_INODE_BUCKETS32
 extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
@@ -310,7 +310,7 @@ extern u32 audit_sig_sid;
 extern int __audit_signal_info(int sig, struct task_struct *t);
 static inline int audit_signal_info(int sig, struct task_struct *t)
 {
-   if (unlikely((audit_pid  t-tgid == audit_pid) ||
+   if (unlikely((audit_pid  task_tgid(t) == audit_pid) ||
 (audit_signals  !audit_dummy_context(
return __audit_signal_info(sig, t);
return 0;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 60a966d..2dcf67d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -739,7 +739,7 @@ static enum audit_state audit_filter_syscall(struct 
task_struct *tsk,
struct audit_entry *e;
enum audit_state state;
 
-   if (audit_pid  tsk-tgid == audit_pid)
+   if (audit_pid  task_tgid(tsk) == audit_pid)
return

Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task-pid and task-tgid

[Richard Guy Briggs]

On Thu, Aug 22, 2013 at 10:05:55PM +0200, Peter Zijlstra wrote:
 On Tue, Aug 20, 2013 at 05:32:03PM -0400, Richard Guy Briggs wrote:
  This stops these four task helper functions from using the deprecated and
  error-prone task-pid and task-tgid.
  
  (informed by ebiederman's ea5a4d01)
  Cc: Eric W. Biederman ebied...@xmission.com
  Signed-off-by: Richard Guy Briggs r...@redhat.com
  ---
   include/linux/sched.h |8 
   1 files changed, 4 insertions(+), 4 deletions(-)
  
  diff --git a/include/linux/sched.h b/include/linux/sched.h
  index 8e69807..46e739d 100644
  --- a/include/linux/sched.h
  +++ b/include/linux/sched.h
  @@ -1579,7 +1579,7 @@ static inline int pid_alive(struct task_struct *p)
*/
   static inline int is_global_init(struct task_struct *tsk)
   {
  -   return tsk-pid == 1;
  +   return task_pid_nr(tsk) == 1;
   }
   
   extern struct pid *cad_pid;
  @@ -1930,7 +1930,7 @@ extern struct task_struct *idle_task(int cpu);
*/
   static inline bool is_idle_task(const struct task_struct *p)
   {
  -   return p-pid == 0;
  +   return task_pid(p) == init_struct_pid;
   }
   extern struct task_struct *curr_task(int cpu);
   extern void set_curr_task(int cpu, struct task_struct *p);
 
 Why would you ever want to do this? It just makes these tests more
 expensive for no gain what so ff'ing ever.

Backups are generally considered a good idea, but in this case, I'd
quote:
A man with one watch knows what time it is. A man with two is
never certain.

Reminds me of the twist of a phrase frequently seen in the US gov:
Government Duplicity, Do Not Propagate   ;-)


Can you suggest a safe way to live with this duplicity?


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task-pid and task-tgid

[Richard Guy Briggs]

On Fri, Aug 23, 2013 at 08:36:21AM +0200, Peter Zijlstra wrote:
 On Thu, Aug 22, 2013 at 05:43:47PM -0400, Richard Guy Briggs wrote:
  On Thu, Aug 22, 2013 at 10:05:55PM +0200, Peter Zijlstra wrote:
   On Tue, Aug 20, 2013 at 05:32:03PM -0400, Richard Guy Briggs wrote:
This stops these four task helper functions from using the deprecated 
and
error-prone task-pid and task-tgid.

(informed by ebiederman's ea5a4d01)
Cc: Eric W. Biederman ebied...@xmission.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/sched.h |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8e69807..46e739d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1579,7 +1579,7 @@ static inline int pid_alive(struct task_struct *p)
  */
 static inline int is_global_init(struct task_struct *tsk)
 {
-   return tsk-pid == 1;
+   return task_pid_nr(tsk) == 1;
 }
 
 extern struct pid *cad_pid;
@@ -1930,7 +1930,7 @@ extern struct task_struct *idle_task(int cpu);
  */
 static inline bool is_idle_task(const struct task_struct *p)
 {
-   return p-pid == 0;
+   return task_pid(p) == init_struct_pid;
 }
 extern struct task_struct *curr_task(int cpu);
 extern void set_curr_task(int cpu, struct task_struct *p);
   
   Why would you ever want to do this? It just makes these tests more
   expensive for no gain what so ff'ing ever.
  
  Backups are generally considered a good idea, but in this case, I'd
  quote:
  A man with one watch knows what time it is. A man with two is
  never certain.
 
 Except that's not the case, with namespaces there's a clear hierarchy
 and the task_struct::pid is the one true value aka. root namespace.

Peter, I agonized over the access efficiency of dropping this one or the
duplicate in task_struct::pids and this one was far easier to drop in
terms of somehow always forcing
task-pids[PIDTYPE_PID].pid-numbers[0].nr to point to task-pid.

It should be possible to audit the kernel to make certain task-pid is
only ever written at the time of task creation and copied to its own
task-pids[PIDTYPE_PID].pid-numbers[0].nr at the time of task creation
so that the two values are consistent.  Continuously auditing the kernel
so this is the case would be a bit more of a challenge.

Would it be reasonable to suggest task_struct::pid only be accessed by
the existing inlined task_pid_nr() converted to const?

The goal is to gain assurance that any PIDs referred to in audit logs
are indisputable.

Would you be alright with doing away with task_struct::tgid?

 Furthermore idle threads really are special and it doesn't make sense to
 address them in any but the root namespace, doubly so because only
 kernel space does this.

If that is the case, and the above holds true, then we don't need the
second hunk, I agree.

 As for the init thread, that function is called is_global_init() for
 crying out loud, what numb nut doesn't get that that's supposed to be
 using the root namespace?

A process in another pid namespace?  If that's never going to happen,
then drop it.

 Seriously, you namespace guys should stop messing things up and
 confusing yourselves and others.

you namespace guys?  I'm not a namespace guy.  I'm a rusty kernel
network security guy taking on audit, trying to prepare it for moving
into a more and more namespace-enabled place of
containerization/light-virtualization.

We aren't ready for it yet, but there is demand to run additional auditd
daemons in other pid namespaces and some of this work is trying to move
in that direction.

This patchset certainly wasn't intended to be ready for adoption just
yet.  It was this sort of discussion I was hoping to have.


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task-pid and task-tgid

[Richard Guy Briggs]

On Fri, Aug 23, 2013 at 09:28:07PM +0200, Oleg Nesterov wrote:
 On 08/22, Richard Guy Briggs wrote:
 
  On Thu, Aug 22, 2013 at 10:05:55PM +0200, Peter Zijlstra wrote:
  
   Why would you ever want to do this? It just makes these tests more
   expensive for no gain what so ff'ing ever.
 
  Backups are generally considered a good idea, but in this case, I'd
  quote:
 
 And perhaps you are right. At least we can probably kill task-tgid.
 And I agree, it would be nice to kill task-pid.
 
 But: I also agree with Peter, we should try to not make the current
 code more expensive.

I don't disagree.  I was given some hope by Eric Biederman that it might
not cause cache-line misses...

 Anyway. Imho, you should not mix the different things in one series.
 If you want to fix audit, do not add the patches like 10/12 or 11/12
 into this series.

They were included to gain reassurance that PIDs reported in audit logs
were accurate.  If those assurances can be made, then I can limit my
work to audit itself.

 Or 3/12.

That is a cleanup to make clear what parts are actually pid-related and
what isn't.

 OK, I agree sys_getppid() in audit_log_task_info() looks
 strange at least. Just fix it using the helpers we already have and
 add the new helpers later. Or send the patch(es) which adds the new
 helpers first.

Patch 04/12 is that helper.  It is used in only two places in audit (and
once in apparmor), so I could have duplicated the code, but since it
needs rcu locking, an inline funciton in the audit namespace seemed
somewhat pointless when it could just as easily be shared with other
subsystems.

 Or task_pid_nr_init_ns()... For what? We already have task_pid_nr().
 Use the helper we already have, or introduce the new one first and
 change the current users of task_pid_nr().

If task_struct::pid is definitely not going away, then that whole part
is moot and we'll just use task_pid_nr() as is.

 In short. Fortunately you do not need to convince me, I do not
 maintain audit or namespaces ;) But imho this series looks a bit
 confusing.

It is incomplete.  The last step(s) were intended to purge
task_struct::pid (or abstract it using task_pid_nr()), which haven't
been submitted yet.  The whole point of the patchset was to give
confidence in the PIDs reported in any audit logs.

 Oleg.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 03/12] pid: get ppid pid_t of task in init_pid_ns safely

[Richard Guy Briggs]

On Tue, Aug 27, 2013 at 07:21:55PM +0200, Oleg Nesterov wrote:
 On 08/20, Richard Guy Briggs wrote:
 
  Added the functions
  task_ppid()
  task_ppid_nr_ns()
  task_ppid_nr_init_ns()
  to safely abstract the lookup of the PPID
 
 but it is not safe.
 
  +static inline struct pid *task_ppid(struct task_struct *task)
  +{
  +   return task_tgid(rcu_dereference(current-real_parent));
  ^^^
 task?

Yup, thanks for those two catches.

  +   rcu_read_unlock();
 
 And why this is safe?
 
 rcu_read_lock() can't help if tsk was already dead _before_ it takes
 the rcu lock. -real_parent can point the already freed/reused/unmapped
 memory.

Does it not bump a refcount if it is holding a pointer to it?  So the
parent task might be dead, but it won't cause a pointer dereference
issue.

 This is safe if, for example, the caller alredy holds rcu_read_lock()
 and tsk was found by find_task_by*(), or tsk is current.

Fair enough, I'll have a more careful look at this.  Thanks.

Most of the instances are current, but the one called from apparmour is
stored.  I've just learned that this is bad and someone else just chimed
in that they have a patch to remove it...

So what is a reliable way of keeping a reference to a task?  I had
assumed that the best way was to keep a pointer to its task_struct,
making sure its refcount had been bumped by something like
get_task_struct().  Another way would be to do the same with its struct
pid.  The third that I'm trying to avoid is using its init_pid_namespace
pid_t since it could refer to a completely different task if the pid_t
rolls over.

 Oleg.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

audit looks unmaintained? [was: Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task-pid and task-tgid]

[Richard Guy Briggs]

On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
 Btw. audit looks unmaintained... if you are going to take care of
 this code, perhaps you can look at
 
   http://marc.info/?l=linux-kernelm=137589907108485
   http://marc.info/?l=linux-kernelm=137590271809664

(I don't want to lose these refs...  First I've seen these.)

Why do you say this?  Could you elaborate?  Due to the state of the code
itself, or the lack of response from audit folks?  (Like most, I'm not
subscribed to that firehose, so I don't have archives that show
addressees.)  Most of the kernel audit folks are on
linux-audit@redhat.com list.

 Oleg.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 03/12] pid: get ppid pid_t of task in init_pid_ns safely

[Richard Guy Briggs]

On Fri, Aug 30, 2013 at 01:37:09PM -0700, John Johansen wrote:
 On 08/30/2013 12:56 PM, Richard Guy Briggs wrote:
  On Tue, Aug 27, 2013 at 07:21:55PM +0200, Oleg Nesterov wrote:
  On 08/20, Richard Guy Briggs wrote:
  Most of the instances are current, but the one called from apparmour is
  stored.  I've just learned that this is bad and someone else just chimed
  in that they have a patch to remove it...
 
 the apparmor case isn't actually stored long term. The stored task will be
 a parameter that was passed into an lsm hook and the buffer that it is
 stored in dies before the hook is done. Its temporarily stored in the
 struct so that it can be passed into the lsm_audit fn, and printed into an
 allocated audit buffer. The text version in the audit buffer is what will
 exist beyond the hook.
 
 There are three patches, I'll reply them below once I have finished rebasing
 them to apply to the current tree instead of my dev tree.

John, thanks for this context and fix.  That helps simplify things.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: user message limits

[Richard Guy Briggs]

On Wed, Jan 28, 2009 at 11:44:27AM -0600, LC Bruzenak wrote:
 On Wed, 2009-01-28 at 12:15 -0500, Steve Grubb wrote:
  Offhand, I don't remember why the kernel sets the limit so low. It could be 
  bumped some. How much, I don't know. 4K or 8K would seem fine.

I'm just reading this thread now, relating to
https://bugzilla.redhat.com/show_bug.cgi?id=1007069

  -Steve
 
 To me the primary thing is consistency with the input text size.
 Seems strange to successfully send in some data and be unable to
 retrieve it.
 
 A secondary concern is - what is the input limit? If the total input
 buffer size is 8K and some of that needs to be used internally (by audit
 lib), maybe it should be clamped at 7K? 

I did some tests.  I set the format string in the kernel to 8970
characters.  In my example, I had a kernel-generated prefix of 133
characters and userspace libaudit-appended suffix of 71 characters
included in the buffer.  I got a limit of 8937 characters for the
resulting message sent to auditd.  To fill the buffer, my text ended up
being 8733 octets with the message from userspace (including the
libaudit suffix) being 8804.  I then deliberately overflowed it and
stopped receiving messages from userspace when the text was 8798
octets, truncating most of the suffix.

This tells me the kernel or auditd limit is 8937.  I assume this depends
on netlink buffer structures, which could vary by message or by kernel.
It also tells me the auditctl-buffer limit is 8804.  Given the
kernel-generated prefix could be a couple hundred characters, can we set
format string to at least something less than 8937 - 133, if not 8937 -
250(or more)?  I'd suggest 8637 as a ballpark.  Then, set
MAX_AUDIT_MESSAGE_LENGTH less than that, maybe 200 above 8K.  8K could
be easily accomodated.

Steve, can you give a sense as to the maximum added by libaudit?  Do you
have a sense as to the maximum added by the kernel?

 I'm trying to avoid a lot of retry logic on the sending side, where if a
 failure occurs, we would truncate and resend until it passes. I guess if
 I were certain that 7K is always going to fit I could artificially clamp
 my own input there, but it seems better if it were universally constant.

As to the question of buffer size detection, that's a more involved
problem as noted with unreliable tests on kernel version...

 LC (Lenny) Bruzenak
 le...@magitekltd.com

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH] audit: format user messages to size of MAX_AUDIT_MESSAGE_LENGTH

[Richard Guy Briggs]

Messages of type AUDIT_USER_TTY were being formatted to 1024 octets,
truncating messages approaching MAX_AUDIT_MESSAGE_LENGTH (8970 octets).

Set the formatting to 8560 characters, given maximum estimates for prefix and
suffix budgets.

See the problem discussion:
https://www.redhat.com/archives/linux-audit/2009-January/msg00030.html

And the new size rationale:
https://www.redhat.com/archives/linux-audit/2013-September/msg00016.html

Test ~8k messages with:
auditctl -m $(for i in $(seq -w 001 820);do echo -n ${i}0__;done)

Reported-by: LC Bruzenak le...@magitekltd.com
Reported-by: Justin Stephenson jstep...@redhat.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..939cff1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -715,7 +715,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
}
audit_log_common_recv_msg(ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
-   audit_log_format(ab,  msg='%.1024s',
+   audit_log_format(ab,  msg='%.8560s',
 (char *)data);
else {
int size;
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 1/8] audit: avoid soft lockup due to audit_log_start() incorrect loop termination

[Richard Guy Briggs]

Commit 82919919 caused the wait for auditd timeout condition to loop endlessly
rather than fall through to the error recovery code.

 BUG: soft lockup - CPU#0 stuck for 22s! [preload:785]
 RIP: 0010:[810fb240]  [810fb240] audit_log_start+0xf0/0x460
 Call Trace:
  [810aca40] ? try_to_wake_up+0x310/0x310
  [81100fd1] audit_log_exit+0x51/0xcb0
  [811020b5] __audit_syscall_exit+0x275/0x2d0
  [816ec540] sysret_audit+0x17/0x21

If the timeout condition goes negative, the loop continues endlessly instead of
breaking out and going to the failure code and allow other processes to run
when auditd is unable to drain the queue fast enough.

This can easily be triggered by readahead-collector, in particular during
installations.  The readahead-collector (ab)uses the audit interface and
sometimes gets stuck in a 'stopped' state.

To trigger:
readahead-collector -f 
pkill -SIGSTOP readahead-collector
top

See:
https://lkml.org/lkml/2013/8/28/626
https://lkml.org/lkml/2013/9/2/471
https://lkml.org/lkml/2013/9/3/4

Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org
Signed-off-by: Dan Duval dan.du...@oracle.com
Signed-off-by: Chuck Anderson chuck.ander...@oracle.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..7b0e23a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1117,9 +1117,10 @@ struct audit_buffer *audit_log_start(struct 
audit_context *ctx, gfp_t gfp_mask,
 
sleep_time = timeout_start + audit_backlog_wait_time -
jiffies;
-   if ((long)sleep_time  0)
+   if ((long)sleep_time  0) {
wait_for_auditd(sleep_time);
-   continue;
+   continue;
+   }
}
if (audit_rate_check()  printk_ratelimit())
printk(KERN_WARNING
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 4/8] audit: efficiency fix 1: only wake up if queue shorter than backlog limit

[Richard Guy Briggs]

author: Dan Duval dan.du...@oracle.com

These and similar errors were seen on a patched 3.8 kernel when the
audit subsystem was overrun during boot:

  udevd[876]: worker [887] unexpectedly returned with status 0x0100
  udevd[876]: worker [887] failed while handling
'/devices/pci:00/:00:03.0/:40:00.0'
  udevd[876]: worker [880] unexpectedly returned with status 0x0100
  udevd[876]: worker [880] failed while handling
'/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1'

  udevadm settle - timeout of 180 seconds reached, the event queue
contains:
/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1 (3995)
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3F0D:00 (4034)

  audit: audit_backlog=258  audit_backlog_limit=256
  audit: audit_lost=1 audit_rate_limit=0 audit_backlog_limit=256

The change below increases the efficiency of the audit code and prevents it
from being overrun:

Only issue a wake_up in kauditd if the length of the skb queue is less than the
backlog limit.  Otherwise, threads waiting in wait_for_auditd() will simply
wake up, discover that the queue is still too long for them to proceed, and go
back to sleep.  This results in wasted context switches and machine cycles.
kauditd_thread() is the only function that removes buffers from audit_skb_queue
so we can't race.  If we did, the timeout in wait_for_auditd() would expire and
the waiting thread would continue.

See: https://lkml.org/lkml/2013/9/2/479

Signed-off-by: Dan Duval dan.du...@oracle.com
Signed-off-by: Chuck Anderson chuck.ander...@oracle.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 42c68db..25fab2d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -448,8 +448,10 @@ static int kauditd_thread(void *dummy)
flush_hold_queue();
 
skb = skb_dequeue(audit_skb_queue);
-   wake_up(audit_backlog_wait);
+
if (skb) {
+   if(skb_queue_len(audit_skb_queue) = 
audit_backlog_limit)
+   wake_up(audit_backlog_wait);
if (audit_pid)
kauditd_send_skb(skb);
else
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 6/8] audit: add boot option to override default backlog limit

[Richard Guy Briggs]

The default audit_backlog_limit is 64.  This was a reasonable limit at one time.

systemd causes so much audit queue activity on startup that auditd doesn't
start before the backlog queue has already overflowed by more than a factor of
2.  On a system with audit= not set on the kernel command line, this isn't an
issue since that history isn't kept for auditd when it is available.  On a
system with audit=1 set on the kernel command line, kaudit tries to keep that
history until auditd is able to drain the queue.

This default can be changed by the -b option in audit.rules once the system
has booted, but won't help with lost messages on boot.

One way to solve this would be to increase the default backlog queue size to
avoid losing any messages before auditd is able to consume them.  This would
be overkill to the embedded community and insufficient for some servers.

Another way to solve it might be to add a kconfig option to set the default
based on the system type.  An embedded system would get the current (or
smaller) default, while Workstations might get more than now and servers might
get more.

None of these solutions helps if a system's compiled default is too small to
see the lost messages without compiling a new kernel.

This patch adds a boot option (audit already has one to enable/disable it)
audit_backlog_limit=n that overrides the default to allow the system
administrator to set the backlog limit.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |   14 +-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 990d02f..acfa7a9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -944,9 +944,21 @@ static int __init audit_enable(char *str)
 
return 1;
 }
-
 __setup(audit=, audit_enable);
 
+/* Process kernel command-line parameter at boot time.  
audit_backlog_limit=n */
+static int __init audit_backlog_limit_set(char *str)
+{
+   int audit_backlog_limit_arg = simple_strtol(str, NULL, 0);
+   if ((audit_backlog_limit_arg = 0)  (audit_backlog_limit_arg  8192))
+   audit_backlog_limit = audit_backlog_limit_arg;
+
+   printk(KERN_INFO audit_backlog_limit: %d\n, audit_backlog_limit);
+
+   return 1;
+}
+__setup(audit_backlog_limit=, audit_backlog_limit_set);
+
 static void audit_buffer_free(struct audit_buffer *ab)
 {
unsigned long flags;
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/8] audit: reset audit backlog wait time after error recovery

[Richard Guy Briggs]

When the audit queue overflows and times out (audit_backlog_wait_time), the
audit queue overflow timeout is set to zero.  Once the audit queue overflow
timeout condition recovers, the timeout should be reset to the original value.

See also:
https://lkml.org/lkml/2013/9/2/473

Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Signed-off-by: Dan Duval dan.du...@oracle.com
Signed-off-by: Chuck Anderson chuck.ander...@oracle.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |5 -
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..772725e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -103,7 +103,8 @@ static int  audit_rate_limit;
 
 /* Number of outstanding audit_buffers allowed. */
 static int audit_backlog_limit = 64;
-static int audit_backlog_wait_time = 60 * HZ;
+#define AUDIT_BACKLOG_WAIT_TIME (60 * HZ)
+static int audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 static int audit_backlog_wait_overflow = 0;
 
 /* The identity of the user shutting down the audit system. */
@@ -1134,6 +1135,8 @@ struct audit_buffer *audit_log_start(struct audit_context 
*ctx, gfp_t gfp_mask,
return NULL;
}
 
+   audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
+
ab = audit_buffer_alloc(ctx, gfp_mask, type);
if (!ab) {
audit_log_lost(out of memory in audit_log_start);
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 0/8] Audit backlog queue fixes related to soft lockup

[Richard Guy Briggs]

This patchset covers a number of issues surrounding the audit backlog queue.
The original trigger was a bug in a patch to fix code that produced negative
sleep times (commit 82919919).  This bug caused soft lockups.  There were a
number of other issues raised surrounding this bug which have been attempted
to be addressed in this patchset.

The first patch fixes this obvious bug.

The 2nd fixes the lack of reset to the original wait time that was set to
zero under error conditions should the system recover from the conditions that
tickled the first bug.

The 3rd makes use of the remaining timeout to avoid re-checking the loop
conditions on error.

The 4th and 5th are efficiency fixes for waking up waiting processes only when
necessary.

The 6th adds a boot parameter to temporarily override the backlog queue length
in case more buffers are needed before auditd is available.

The 7th and 8th are to add a config option to make the backlog wait time
configurable from the hard-coded default.


Richard Guy Briggs (8):
  audit: avoid soft lockup due to audit_log_start() incorrect loop
termination
  audit: reset audit backlog wait time after error recovery
  audit: make use of remaining sleep time from wait_for_auditd
  audit: efficiency fix 1: only wake up if queue shorter than backlog
limit
  audit: efficiency fix 2: request exclusive wait since all need same
resource
  audit: add boot option to override default backlog limit
  audit: clean up AUDIT_GET/SET local variables and future-proof API
  audit: add audit_backlog_wait_time configuration option

 include/uapi/linux/audit.h |2 +
 kernel/audit.c |  111 +++-
 2 files changed, 80 insertions(+), 33 deletions(-)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 5/8] audit: efficiency fix 2: request exclusive wait since all need same resource

[Richard Guy Briggs]

author: Dan Duval dan.du...@oracle.com

These and similar errors were seen on a patched 3.8 kernel when the
audit subsystem was overrun during boot:

  udevd[876]: worker [887] unexpectedly returned with status 0x0100
  udevd[876]: worker [887] failed while handling
'/devices/pci:00/:00:03.0/:40:00.0'
  udevd[876]: worker [880] unexpectedly returned with status 0x0100
  udevd[876]: worker [880] failed while handling
'/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1'

  udevadm settle - timeout of 180 seconds reached, the event queue
contains:
/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input1/event1 (3995)
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3F0D:00 (4034)

  audit: audit_backlog=258  audit_backlog_limit=256
  audit: audit_lost=1 audit_rate_limit=0 audit_backlog_limit=256

The change below increases the efficiency of the audit code and prevents it
from being overrun:

Use add_wait_queue_exclusive() in wait_for_auditd() to put the
thread on the wait queue.  When kauditd dequeues an skb, all
of the waiting threads are waiting for the same resource, but
only one is going to get it, so there's no need to wake up
more than one waiter.

See: https://lkml.org/lkml/2013/9/2/479

Signed-off-by: Dan Duval dan.du...@oracle.com
Signed-off-by: Chuck Anderson chuck.ander...@oracle.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 25fab2d..990d02f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1061,7 +1061,7 @@ static unsigned long wait_for_auditd(unsigned long 
sleep_time)
unsigned long timeout = sleep_time;
DECLARE_WAITQUEUE(wait, current);
set_current_state(TASK_UNINTERRUPTIBLE);
-   add_wait_queue(audit_backlog_wait, wait);
+   add_wait_queue_exclusive(audit_backlog_wait, wait);
 
if (audit_backlog_limit 
skb_queue_len(audit_skb_queue)  audit_backlog_limit)
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] Audit: remove duplicate comments

[Richard Guy Briggs]

On Mon, Sep 23, 2013 at 08:16:07AM -0700, Casey Schaufler wrote:
 On 9/23/2013 12:55 AM, Gao feng wrote:
  Remove it.
 
 Why? Is it inaccurate? 

Yes.  It is a duplicate from 8 years ago that is no longer accurate.

  Signed-off-by: Gao feng gaof...@cn.fujitsu.com
  ---
   kernel/audit.c | 7 ---
   1 file changed, 7 deletions(-)
 
  diff --git a/kernel/audit.c b/kernel/audit.c
  index 91e53d0..f94db2a 100644
  --- a/kernel/audit.c
  +++ b/kernel/audit.c
  @@ -1067,13 +1067,6 @@ static void wait_for_auditd(unsigned long sleep_time)
  remove_wait_queue(audit_backlog_wait, wait);
   }
   
  -/* Obtain an audit buffer.  This routine does locking to obtain the
  - * audit buffer, but then no locking is required for calls to
  - * audit_log_*format.  If the tsk is a task that is currently in a
  - * syscall, then the syscall is marked as auditable and an audit record
  - * will be written at syscall exit.  If there is no associated task, tsk
  - * should be NULL. */
  -
   /**
* audit_log_start - obtain an audit buffer
* @ctx: audit_context (may be NULL)
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] audit: fix info leak in AUDIT_GET requests

[Richard Guy Briggs]

On Mon, Sep 30, 2013 at 10:04:24PM +0200, Mathias Krause wrote:
 We leak 4 bytes of kernel stack in response to an AUDIT_GET request as
 we miss to initialize the mask member of status_set. Fix that.

Thank you.

 Cc: Al Viro v...@zeniv.linux.org.uk
 Cc: Eric Paris epa...@redhat.com
 Cc: sta...@vger.kernel.org  # v2.6.6+
 Signed-off-by: Mathias Krause mini...@googlemail.com
 ---
  kernel/audit.c |1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/kernel/audit.c b/kernel/audit.c
 index 7b0e23a..e237712 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -659,6 +659,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
  
   switch (msg_type) {
   case AUDIT_GET:
 + status_set.mask  = 0;
   status_set.enabled   = audit_enabled;
   status_set.failure   = audit_failure;
   status_set.pid   = audit_pid;
 -- 
 1.7.10.4
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] audit: use nlmsg_len() to get message payload length

[Richard Guy Briggs]

On Mon, Sep 30, 2013 at 10:04:25PM +0200, Mathias Krause wrote:
 Using the nlmsg_len member of the netlink header to test if the message
 is valid is wrong as it includes the size of the netlink header itself.

Normally, yes.

The original kaudit unicast socket sends up messages with nlmsg_len set
to the payload length rather than the entire message length.  This
breaks the convention used by netlink.  This is set in audit_log_end().
(audit_make_reply looks like it needs a revisit...)
The existing auditd daemon assumes this breakage.  Fixing this would
require co-ordinating a change in the established protocol between
kaudit kernel code and auditd userspace code.

 Thereby allowing to send short netlink messages that pass those checks.
 
 Use nlmsg_len() instead to test for the right message length. The result
 of nlmsg_len() is guaranteed to be non-negative as the netlink message
 already passed the checks of nlmsg_ok().

Since both of these are downward-headed messages, you are correct.

 Also switch to min_t() to please checkpatch.pl.

Thank you for the catch.

 Cc: Al Viro v...@zeniv.linux.org.uk
 Cc: Eric Paris epa...@redhat.com
 Cc: sta...@vger.kernel.org  # v2.6.6+ for the 1st hunk, v2.6.23+ for the 2nd
 Signed-off-by: Mathias Krause mini...@googlemail.com
 ---
  kernel/audit.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/kernel/audit.c b/kernel/audit.c
 index e237712..10c7263 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -671,7 +671,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
status_set, sizeof(status_set));
   break;
   case AUDIT_SET:
 - if (nlh-nlmsg_len  sizeof(struct audit_status))
 + if (nlmsg_len(nlh)  sizeof(struct audit_status))
   return -EINVAL;
   status_get   = (struct audit_status *)data;
   if (status_get-mask  AUDIT_STATUS_ENABLED) {
 @@ -833,7 +833,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
  
   memset(s, 0, sizeof(s));
   /* guard against past and future API changes */
 - memcpy(s, data, min(sizeof(s), (size_t)nlh-nlmsg_len));
 + memcpy(s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));
   if ((s.enabled != 0  s.enabled != 1) ||
   (s.log_passwd != 0  s.log_passwd != 1))
   return -EINVAL;
 -- 
 1.7.10.4
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH] audit: update AUDIT_INODE filter rule to comparator function

[Richard Guy Briggs]

It appears this one comparison function got missed in f368c07d (and 9c937dcc).

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/auditsc.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 63223d6..065c7a1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -566,7 +566,7 @@ static int audit_filter_rules(struct task_struct *tsk,
break;
case AUDIT_INODE:
if (name)
-   result = (name-ino == f-val);
+   result = audit_comparator(name-ino, f-op, 
f-val);
else if (ctx) {
list_for_each_entry(n, ctx-names_list, list) {
if (audit_comparator(n-ino, f-op, 
f-val)) {
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: change pid to portid for audit_reply

[Richard Guy Briggs]

On Wed, Oct 23, 2013 at 07:25:23PM +0800, Gao feng wrote:
 The pid is not a suitable name for netlink port,
 change it to portid.

That is already in the works:
https://www.redhat.com/archives/linux-audit/2013-August/msg00023.html
https://lkml.org/lkml/2013/8/20/630

May I add your Signed-off-by: to that previous patch?

 more information, please see commit
 15e473046cb6e5d18a4d0057e61d76315230382b
 
 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 ---
  kernel/audit.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)
 
 diff --git a/kernel/audit.c b/kernel/audit.c
 index 7b0e23a..50fdcba 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -165,7 +165,7 @@ struct audit_buffer {
  };
  
  struct audit_reply {
 - int pid;
 + int portid;
   struct sk_buff *skb;
  };
  
 @@ -487,7 +487,7 @@ int audit_send_list(void *_dest)
   return 0;
  }
  
 -struct sk_buff *audit_make_reply(int pid, int seq, int type, int done,
 +struct sk_buff *audit_make_reply(int portid, int seq, int type, int done,
int multi, const void *payload, int size)
  {
   struct sk_buff  *skb;
 @@ -500,7 +500,7 @@ struct sk_buff *audit_make_reply(int pid, int seq, int 
 type, int done,
   if (!skb)
   return NULL;
  
 - nlh = nlmsg_put(skb, pid, seq, t, size, flags);
 + nlh = nlmsg_put(skb, portid, seq, t, size, flags);
   if (!nlh)
   goto out_kfree_skb;
   data = nlmsg_data(nlh);
 @@ -521,13 +521,13 @@ static int audit_send_reply_thread(void *arg)
  
   /* Ignore failure. It'll only happen if the sender goes away,
  because our timeout is set to infinite. */
 - netlink_unicast(audit_sock, reply-skb, reply-pid, 0);
 + netlink_unicast(audit_sock, reply-skb, reply-portid, 0);
   kfree(reply);
   return 0;
  }
  /**
   * audit_send_reply - send an audit reply message via netlink
 - * @pid: process id to send reply to
 + * @portid: the portid of netlink socket
   * @seq: sequence number
   * @type: audit message type
   * @done: done (last) flag
 @@ -538,7 +538,7 @@ static int audit_send_reply_thread(void *arg)
   * Allocates an skb, builds the netlink message, and sends it to the pid.
   * No failure notifications.
   */
 -static void audit_send_reply(int pid, int seq, int type, int done, int multi,
 +static void audit_send_reply(int portid, int seq, int type, int done, int 
 multi,
const void *payload, int size)
  {
   struct sk_buff *skb;
 @@ -549,11 +549,11 @@ static void audit_send_reply(int pid, int seq, int 
 type, int done, int multi,
   if (!reply)
   return;
  
 - skb = audit_make_reply(pid, seq, type, done, multi, payload, size);
 + skb = audit_make_reply(portid, seq, type, done, multi, payload, size);
   if (!skb)
   goto out;
  
 - reply-pid = pid;
 + reply-portid = portid;
   reply-skb = skb;
  
   tsk = kthread_run(audit_send_reply_thread, reply, audit_send_reply);
 -- 
 1.8.3.1
 
 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [BUG][PATCH] audit: audit_log_start running on auditd should not stop

[Richard Guy Briggs]

On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
 Hi Toshiyuki-san,

Toshiuki and Gao,

 On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
  The backlog cannot be consumed when audit_log_start is running on auditd
  even if audit_log_start calls wait_for_auditd to consume it.
  The situation is a deadlock because only auditd can consume the backlog.
  If the other process needs to send the backlog, it can be also stopped 
  by the deadlock.
  
  So, audit_log_start running on auditd should not stop.
  
  You can see the deadlock with the following reproducer:
   # auditctl -a exit,always -S all
   # reboot

 Hmm, I see, There may be other code paths that auditd can call 
 audit_log_start except
 audit_log_config_change. so it's better to handle this problem in 
 audit_log_start.
 
 but current task is only meaningful when gfp_mask  __GFP_WAIT is true.
 so maybe the below patch is what you want.

I have been following this thread with interest.  I like the general
evolution of this patch.  The first patch was a bit too abrupt, dropping
too much, but this one makes much more sense.  I would be tempted to
make the reserve even bigger.

I see that you should be using a kernel that has included commit
8ac1c8d5 (which made it into v3.12-rc3)
audit: fix endless wait in audit_log_start()
That was an obvious bug, but I was still concerned about the cause of
the initial wait.  There are other fixes and ideas in the works that
should alleviate some of the pressure to make the service more usable.
https://lkml.org/lkml/2013/9/18/453

I have tested with and without this v3 patch and I don't see any
significant difference with the reproducer provided above.  I'm also
testing with a reproducer of the endless wait bug (readahead-collector).

What are your expected results?  What are your actual results in each
case?  How are they different?

 diff --git a/kernel/audit.c b/kernel/audit.c
 index 7b0e23a..10b4545 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct 
 audit_context
 struct audit_buffer *ab = NULL;
 struct timespec t;
 unsigned intuninitialized_var(serial);
 -   int reserve;
 +   int reserve = 5; /* Allow atomic callers to go up to five
 +   entries over the normal backlog limit */
 +
 unsigned long timeout_start = jiffies;
 
 if (audit_initialized != AUDIT_INITIALIZED)
 @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct 
 audit_contex
 if (unlikely(audit_filter_type(type)))
 return NULL;
 
 -   if (gfp_mask  __GFP_WAIT)
 -   reserve = 0;
 -   else
 -   reserve = 5; /* Allow atomic callers to go up to five
 -   entries over the normal backlog limit */
 +   if (gfp_mask  __GFP_WAIT) {
 +   if (audit_pid  audit_pid == current-pid)
 +   gfp_mask = ~__GFP_WAIT;
 +   else
 +   reserve = 0;
 +   }
 
 while (audit_backlog_limit
 skb_queue_len(audit_skb_queue)  audit_backlog_limit + 
 reserv

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: Add cmdline to taskinfo output

[Richard Guy Briggs]

On Wed, Oct 23, 2013 at 01:40:42PM -0700, William Roberts wrote:
 From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
 From: William Roberts wrobe...@tresys.com
 Date: Tue, 22 Oct 2013 14:23:27 -0700
 Subject: [PATCH] audit: Add cmdline to taskinfo output

Hi William (Bill?),

 On some devices, the cmdline and task info vary. For instance, on
 Android, the cmdline is set to the package name, and the task info
 is the name of the VM, which is not very helpful.

Your patch doesn't apply to my tree for a couple of reasons.  The
funciton audit_log_task_info() was moved from kernel/auditsc.c to
kernel/audit.c in commit b24a30a7 included in v3.10-rc1.  We're up to
v3.12-rc6.

Please rebase, follow standard kernel coding style (or use a mailer that
won't mangle your patch), re-test and re-send.  I use git format-patch
and git send-email.  Thanks!

 Change-Id: I98a417c9ab3b95664c49aa1c7513cfd8296b6a2a
 Signed-off-by: William Roberts wrobe...@tresys.com
 ---
  fs/proc/base.c  |2 +-
  include/linux/proc_fs.h |1 +
  kernel/auditsc.c|   24 
  3 files changed, 26 insertions(+), 1 deletion(-)
 
 diff --git a/fs/proc/base.c b/fs/proc/base.c
 index 2f198da..25b73d3 100644
 --- a/fs/proc/base.c
 +++ b/fs/proc/base.c
 @@ -209,7 +209,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
   return mm_access(task, PTRACE_MODE_READ);
  }
 
 -static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 +int proc_pid_cmdline(struct task_struct *task, char *buffer)
  {
   int res = 0;
   unsigned int len;
 diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
 index 85c5073..d85ac14 100644
 --- a/include/linux/proc_fs.h
 +++ b/include/linux/proc_fs.h
 @@ -118,6 +118,7 @@ struct pid_namespace;
 
  extern int pid_ns_prepare_proc(struct pid_namespace *ns);
  extern void pid_ns_release_proc(struct pid_namespace *ns);
 +extern int proc_pid_cmdline(struct task_struct *task, char *buffer);
 
  /*
   * proc_tty.c
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index 27ad9dd..7f2bf41 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -67,6 +67,7 @@
  #include linux/syscalls.h
  #include linux/capability.h
  #include linux/fs_struct.h
 +#include linux/proc_fs.h
 
  #include audit.h
 
 @@ -1158,6 +1159,8 @@ static void audit_log_task_info(struct audit_buffer
 *ab, struct task_struct *tsk
   char name[sizeof(tsk-comm)];
   struct mm_struct *mm = tsk-mm;
   struct vm_area_struct *vma;
 + unsigned long page;
 + int len;
 
   /* tsk == current */
 
 @@ -1179,6 +1182,27 @@ static void audit_log_task_info(struct audit_buffer
 *ab, struct task_struct *tsk
   }
   up_read(mm-mmap_sem);
   }
 +
 + /* Get the process cmdline */
 + page = __get_free_page(GFP_TEMPORARY);
 + if (!page)
 + goto out;
 +
 + len = proc_pid_cmdline(tsk, (char *)page);
 + if (len = 0)
 + goto free;
 +
 + /*
 + * Ensure NULL terminated! Application could
 + * could be using setproctitle(3).
 + */
 + ((char *)page)[len-1] = '\0';
 +
 + audit_log_format(ab,  cmdline=);
 + audit_log_untrustedstring(ab, (char *)page);
 +free:
 + free_page(page);
 +out:
   audit_log_task_context(ab);
  }
 
 -- 
 1.7.9.5

 --
 Linux-audit mailing list
 Linux-audit@redhat.com
 https://www.redhat.com/mailman/listinfo/linux-audit


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [BUG][PATCH] audit: audit_log_start running on auditd should not stop

[Richard Guy Briggs]

On Thu, Oct 24, 2013 at 01:55:37PM +0800, Gao feng wrote:
 On 10/24/2013 03:55 AM, Richard Guy Briggs wrote:
  On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
  Hi Toshiyuki-san,
  
  Toshiuki and Gao,
  
  On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
  The backlog cannot be consumed when audit_log_start is running on auditd
  even if audit_log_start calls wait_for_auditd to consume it.
  The situation is a deadlock because only auditd can consume the backlog.
  If the other process needs to send the backlog, it can be also stopped 
  by the deadlock.
 
  So, audit_log_start running on auditd should not stop.
 
  You can see the deadlock with the following reproducer:
   # auditctl -a exit,always -S all
   # reboot
  
  Hmm, I see, There may be other code paths that auditd can call 
  audit_log_start except
  audit_log_config_change. so it's better to handle this problem in 
  audit_log_start.
 
  but current task is only meaningful when gfp_mask  __GFP_WAIT is true.
  so maybe the below patch is what you want.
  
  I have been following this thread with interest.  I like the general
  evolution of this patch.  The first patch was a bit too abrupt, dropping
  too much, but this one makes much more sense.  I would be tempted to
  make the reserve even bigger.
  
  I see that you should be using a kernel that has included commit
  8ac1c8d5 (which made it into v3.12-rc3)
  audit: fix endless wait in audit_log_start()
  That was an obvious bug, 
 
 include or not include?

You say you tested on v3.12-rc4, which does (should?) include this fix.

 The problem is, if the audit_backlog_limit is 3, but there are 5 tasks
 calling audit_log_start, so 2 tasks will wait auditd to consume
 audit_skb_queue. if before auditd consumes skbs, somebody want to kill
 auditd, and auditd will set the audit_pid to zero, this will triger an
 audit message. so auditd will wait for himself. and this waiting is endless,
 since auditd cann't consume audit_skb_queue any more.
 
 the commit 8ac1c8d5 prevent this problem happening. because if once a task is
 blocked over the audit_backlog_wait_time. the audit_backlog_wait_time will
 be set to zero(audit_backlog_wait_overflow which is zero). so the other tasks
 will not wait anymore. but I'm confused if this is what we expected? these
 audit messages will lost once any task is blocked over 
 audit_backlog_wait_time.

This is configurable.  The default is this behaviour, so that if there
is a problem with the audit subsystem, it will give a reasonable chance
for auditd to catch up, then give up, make noise in the syslog (and
configured colsoles) and then continue.  Other auditd configurations
allow the system to halt if this condition occurs.

There is another patch coming that will set that audit_backlog_wait_time
back to the original value once auditd has recovered.  It is not
upstream yet.

 So, AFAIK if commit 8ac1c8d5 exist, this patch is not necessary, but
 we still do something to fix the problem commit 8ac1c8d5 brings.

This is the condition in which I am interested.  Do you have a way to
clearly show this condition without your patch and a way to show it is
solved with your patch?  I'm looking for a clear reproducer.

 but I was still concerned about the cause of
  the initial wait.  There are other fixes and ideas in the works that
  should alleviate some of the pressure to make the service more usable.
  https://lkml.org/lkml/2013/9/18/453
  
  I have tested with and without this v3 patch and I don't see any
  significant difference with the reproducer provided above.  I'm also
  testing with a reproducer of the endless wait bug (readahead-collector).
  
  What are your expected results?  What are your actual results in each
  case?  How are they different?
  
  diff --git a/kernel/audit.c b/kernel/audit.c
  index 7b0e23a..10b4545 100644
  --- a/kernel/audit.c
  +++ b/kernel/audit.c
  @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct 
  audit_context
  struct audit_buffer *ab = NULL;
  struct timespec t;
  unsigned intuninitialized_var(serial);
  -   int reserve;
  +   int reserve = 5; /* Allow atomic callers to go up to five
  +   entries over the normal backlog limit */
  +
  unsigned long timeout_start = jiffies;
 
  if (audit_initialized != AUDIT_INITIALIZED)
  @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct 
  audit_contex
  if (unlikely(audit_filter_type(type)))
  return NULL;
 
  -   if (gfp_mask  __GFP_WAIT)
  -   reserve = 0;
  -   else
  -   reserve = 5; /* Allow atomic callers to go up to five
  -   entries over the normal backlog limit */
  +   if (gfp_mask  __GFP_WAIT) {
  +   if (audit_pid  audit_pid == current-pid)
  +   gfp_mask = ~__GFP_WAIT;
  +   else

Re: [PATCH] audit: Add cmdline to taskinfo output

[Richard Guy Briggs]

On Mon, Oct 28, 2013 at 06:48:48AM -0700, William Roberts wrote:
 On Wed, Oct 23, 2013 at 1:40 PM, William Roberts
 bill.c.robe...@gmail.comwrote:
 
  From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
  From: William Roberts wrobe...@tresys.com
  Date: Tue, 22 Oct 2013 14:23:27 -0700
  Subject: [PATCH] audit: Add cmdline to taskinfo output
 
  On some devices, the cmdline and task info vary. For instance, on
  Android, the cmdline is set to the package name, and the task info
  is the name of the VM, which is not very helpful.

snip

 A few notes on this moving forward:
 1. I forgot to put in the subject kernel v3.4.0, this only applies to that
 2. Yes I know gmail mangled the path (i'm working on some smtp issues right
 now)
 3. The main purpose of this is to figure out upstream acceptance, Richard
 Briggs has chimed in, and has no major objections
 4. This could be a dynamic on/off setting, which brings me to my question,
 of: What is the status of E.Paris's generic feature set/get patches fare?
 This is a great use case for those.

I've queued his patchset for linux-next to be pushed just after v3.12
release.


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: Add cmdline to taskinfo output

[Richard Guy Briggs]

On Mon, Oct 28, 2013 at 12:02:42PM -0700, William Roberts wrote:
 On Mon, Oct 28, 2013 at 9:30 AM, William Roberts
 bill.c.robe...@gmail.comwrote:
  On Mon, Oct 28, 2013 at 8:10 AM, Richard Guy Briggs r...@redhat.comwrote:
  On Mon, Oct 28, 2013 at 06:48:48AM -0700, William Roberts wrote:
   On Wed, Oct 23, 2013 at 1:40 PM, William Roberts 
   bill.c.robe...@gmail.comwrote:
   A few notes on this moving forward:
   4. This could be a dynamic on/off setting, which brings me to my
   question, of: What is the status of E.Paris's generic feature
   set/get patches fare?  This is a great use case for those.
 
  I've queued his patchset for linux-next to be pushed just after v3.12
  release.
 
 By any chance do you have a public tree I can configure you as a remote?

It is currently here,

git://toccata2.tricolour.ca/linux-2.6-rgb.git

with branches
audit-for-next
audit-for-linus

but in the future, an updated link should be findable here if it moves:

http://people.redhat.com/rbriggs/

  - RGB
 
  William C Roberts
 
 William C Roberts

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Format specifier issue when building kernel

[Richard Guy Briggs]

On Mon, Oct 28, 2013 at 04:31:30PM -0700, William Roberts wrote:
 On Mon, Oct 28, 2013 at 4:30 PM, William Roberts
 bill.c.robe...@gmail.comwrote:
 
  I've been working off of Richard Guy Brigs git repo on branch
  audit-for-next prepping my patch and I noticed a build warning:
 
  kernel/audit.c:832:8: warning: format ‘%A’ expects argument of type
  ‘double’, but argument 3 has type ‘char *’ [-Wformat]
 
  Looking at the code, it looks wrong:
 
  audit_log_format(ab,
  
 msg='%.AUDIT_MESSAGE_TEXT_MAXs',
  (char *)data);
 
 The issue appears on the % specifier in there, it picks it up as %.A, which
 is of type double. Is this what was intended?

Hmmm, that should have picked up a macro from 06051fbe in
audit-for-next.  It should be pre-processed to %.8560s.

 William C Roberts


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 1/3] audit: Kill the unused struct audit_aux_data_capset

[Richard Guy Briggs]

From: Eric W. Biederman ebied...@xmission.com

Signed-off-by: Eric W. Biederman ebied...@xmission.com
(cherry picked from commit 6904431d6b41190e42d6b94430b67cb4e7e6a4b7)
(cherry picked from commit 2b3a6c617396a9e6eedae9a56b2d9642da0216b6)
---
 kernel/auditsc.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 95293ab..24047f4 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -121,12 +121,6 @@ struct audit_aux_data_bprm_fcaps {
struct audit_cap_data   new_pcap;
 };
 
-struct audit_aux_data_capset {
-   struct audit_aux_data   d;
-   pid_t   pid;
-   struct audit_cap_data   cap;
-};
-
 struct audit_tree_refs {
struct audit_tree_refs *next;
struct audit_chunk *c[31];
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/3] audit: remove unused envc member of audit_aux_data_execve

[Richard Guy Briggs]

Get rid of write-only audit_aux_data_exeve structure member envc.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/auditsc.c |2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 24047f4..c9abaa0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -98,7 +98,6 @@ struct audit_aux_data {
 struct audit_aux_data_execve {
struct audit_aux_data   d;
int argc;
-   int envc;
struct mm_struct *mm;
 };
 
@@ -2132,7 +2131,6 @@ int __audit_bprm(struct linux_binprm *bprm)
return -ENOMEM;
 
ax-argc = bprm-argc;
-   ax-envc = bprm-envc;
ax-mm = bprm-mm;
ax-d.type = AUDIT_EXECVE;
ax-d.next = context-aux;
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 0/3] audit: Tidy up audit_context and stop bprm recursion

[Richard Guy Briggs]

This patchset is a clean up of the audit_aux_data and audit_context structures
and the audit_bprm() call that was needlessly recursing, allocating more
resources than necessary.

Eric W. Biederman (1):
  audit: Kill the unused struct audit_aux_data_capset

Richard Guy Briggs (2):
  audit: remove unused envc member of audit_aux_data_execve
  audit: call audit_bprm() only once to add AUDIT_EXECVE information

 fs/exec.c |5 +
 include/linux/audit.h |   13 +
 kernel/audit.h|3 +++
 kernel/auditsc.c  |   49 ++---
 4 files changed, 19 insertions(+), 51 deletions(-)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 3/3] audit: call audit_bprm() only once to add AUDIT_EXECVE information

[Richard Guy Briggs]

Move the audit_bprm() call from search_binary_handler() to exec_binprm().  This
allows us to get rid of the mm member of struct audit_aux_data_execve since
bprm-mm will equal current-mm.

This also mitigates the issue that -argc could be modified by the
load_binary() call in search_binary_handler().

audit_bprm() was being called to add an AUDIT_EXECVE record to the audit
context every time search_binary_handler() was recursively called.  Only one
reference is necessary.  Merge the remaining argc parameter into the union in
audit_context, removing a kmalloc.

Reported-by: Oleg Nesterov onest...@redhat.com
Cc: Eric Paris epa...@redhat.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 fs/exec.c |5 +
 include/linux/audit.h |   13 +
 kernel/audit.h|3 +++
 kernel/auditsc.c  |   41 ++---
 4 files changed, 19 insertions(+), 43 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 8875dd1..47d7edb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1385,10 +1385,6 @@ int search_binary_handler(struct linux_binprm *bprm)
if (retval)
return retval;
 
-   retval = audit_bprm(bprm);
-   if (retval)
-   return retval;
-
retval = -ENOENT;
  retry:
read_lock(binfmt_lock);
@@ -1436,6 +1432,7 @@ static int exec_binprm(struct linux_binprm *bprm)
 
ret = search_binary_handler(bprm);
if (ret = 0) {
+   audit_bprm(bprm);
trace_sched_process_exec(current, old_pid, bprm);
ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
current-did_exec = 1;
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 729a4d1..a757e6c 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -207,7 +207,7 @@ static inline int audit_get_sessionid(struct task_struct 
*tsk)
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
umode_t mode);
-extern int __audit_bprm(struct linux_binprm *bprm);
+extern void __audit_bprm(struct linux_binprm *bprm);
 extern int __audit_socketcall(int nargs, unsigned long *args);
 extern int __audit_sockaddr(int len, void *addr);
 extern void __audit_fd_pair(int fd1, int fd2);
@@ -236,11 +236,10 @@ static inline void audit_ipc_set_perm(unsigned long 
qbytes, uid_t uid, gid_t gid
if (unlikely(!audit_dummy_context()))
__audit_ipc_set_perm(qbytes, uid, gid, mode);
 }
-static inline int audit_bprm(struct linux_binprm *bprm)
+static inline void audit_bprm(struct linux_binprm *bprm)
 {
if (unlikely(!audit_dummy_context()))
-   return __audit_bprm(bprm);
-   return 0;
+   __audit_bprm(bprm);
 }
 static inline int audit_socketcall(int nargs, unsigned long *args)
 {
@@ -367,10 +366,8 @@ static inline void audit_ipc_obj(struct kern_ipc_perm 
*ipcp)
 static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
gid_t gid, umode_t mode)
 { }
-static inline int audit_bprm(struct linux_binprm *bprm)
-{
-   return 0;
-}
+static inline void audit_bprm(struct linux_binprm *bprm)
+{ }
 static inline int audit_socketcall(int nargs, unsigned long *args)
 {
return 0;
diff --git a/kernel/audit.h b/kernel/audit.h
index 123c9b7..b779642 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -197,6 +197,9 @@ struct audit_context {
int fd;
int flags;
} mmap;
+   struct {
+   int argc;
+   } execve;
};
int fds[2];
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index c9abaa0..dc1adee 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -95,12 +95,6 @@ struct audit_aux_data {
 /* Number of target pids per aux struct. */
 #define AUDIT_AUX_PIDS 16
 
-struct audit_aux_data_execve {
-   struct audit_aux_data   d;
-   int argc;
-   struct mm_struct *mm;
-};
-
 struct audit_aux_data_pids {
struct audit_aux_data   d;
pid_t   target_pid[AUDIT_AUX_PIDS];
@@ -1144,20 +1138,16 @@ static int audit_log_single_execve_arg(struct 
audit_context *context,
 }
 
 static void audit_log_execve_info(struct audit_context *context,
- struct audit_buffer **ab,
- struct audit_aux_data_execve *axi)
+ struct audit_buffer **ab)
 {
int i, len;
size_t len_sent = 0;
const char __user *p;
char *buf;
 
-   if (axi-mm != current-mm)
-   return; /* execve failed, no additional info */
-
-   p = (const char __user *)axi-mm-arg_start;
+   p = (const char __user *)current-mm-arg_start;
 
-   audit_log_format(*ab, argc=%d, axi-argc);
+   audit_log_format(*ab, argc=%d, context

Re: [PATCH v2] audit: remove useless code in audit_enable

[Richard Guy Briggs]

On Thu, Oct 31, 2013 at 02:31:01PM +0800, Gao feng wrote:
 Since kernel parameter is operated before
 initcall, so the audit_initialized must be
 AUDIT_UNINITIALIZED or DISABLED in audit_enable.

I've queued this patch.  Thanks!

 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 ---
  kernel/audit.c | 13 ++---
  1 file changed, 2 insertions(+), 11 deletions(-)
 
 change from v1:
 convert printk(KERN_INFO  to pr_info(.
 thanks Richard for pointing out.
 
 diff --git a/kernel/audit.c b/kernel/audit.c
 index 8378c5e..7c7c028 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -1038,17 +1038,8 @@ static int __init audit_enable(char *str)
   if (!audit_default)
   audit_initialized = AUDIT_DISABLED;
  
 - printk(KERN_INFO audit: %s, audit_default ? enabled : disabled);
 -
 - if (audit_initialized == AUDIT_INITIALIZED) {
 - audit_enabled = audit_default;
 - audit_ever_enabled |= !!audit_default;
 - } else if (audit_initialized == AUDIT_UNINITIALIZED) {
 - printk( (after initialization));
 - } else {
 - printk( (until reboot));
 - }
 - printk(\n);
 + pr_info(audit: %s\n, audit_default ?
 + enabled (after initialization) : disabled (until reboot));
  
   return 1;
  }
 -- 
 1.8.3.1
 

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 0/4][v2] audit: Tidy up audit_context and stop bprm recursion

[Richard Guy Briggs]

This patchset is a clean up of the audit_aux_data and audit_context structures
and the audit_bprm() call that was needlessly recursing, allocating more
resources than necessary.

Eric W. Biederman (1):
  audit: Kill the unused struct audit_aux_data_capset

Richard Guy Briggs (3):
  audit: remove unused envc member of audit_aux_data_execve
  audit: move audit_aux_data_execve contents into audit_context union
  audit: call audit_bprm() only once to add AUDIT_EXECVE information

 fs/exec.c |5 +
 include/linux/audit.h |   13 +
 kernel/audit.h|3 +++
 kernel/auditsc.c  |   49 ++---
 4 files changed, 19 insertions(+), 51 deletions(-)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 1/4][v2] audit: Kill the unused struct audit_aux_data_capset

[Richard Guy Briggs]

From: Eric W. Biederman ebied...@xmission.com

Signed-off-by: Eric W. Biederman ebied...@xmission.com
(cherry picked from commit 6904431d6b41190e42d6b94430b67cb4e7e6a4b7)
(cherry picked from commit 2b3a6c617396a9e6eedae9a56b2d9642da0216b6)
---
 kernel/auditsc.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 95293ab..24047f4 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -121,12 +121,6 @@ struct audit_aux_data_bprm_fcaps {
struct audit_cap_data   new_pcap;
 };
 
-struct audit_aux_data_capset {
-   struct audit_aux_data   d;
-   pid_t   pid;
-   struct audit_cap_data   cap;
-};
-
 struct audit_tree_refs {
struct audit_tree_refs *next;
struct audit_chunk *c[31];
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 3/4][v2] audit: move audit_aux_data_execve contents into audit_context union

[Richard Guy Briggs]

audit_bprm() was being called to add an AUDIT_EXECVE record to the audit
context every time search_binary_handler() was recursively called.  Only one
reference is necessary, so just update it.  Move the the contents of
audit_aux_data_execve into the union in audit_context, removing dependence on a
kmalloc along the way.

Reported-by: Oleg Nesterov onest...@redhat.com
Cc: Eric Paris epa...@redhat.com
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 include/linux/audit.h |4 ++--
 kernel/audit.h|4 
 kernel/auditsc.c  |   41 -
 3 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 729a4d1..fffefbd 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -207,7 +207,7 @@ static inline int audit_get_sessionid(struct task_struct 
*tsk)
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
umode_t mode);
-extern int __audit_bprm(struct linux_binprm *bprm);
+extern void __audit_bprm(struct linux_binprm *bprm);
 extern int __audit_socketcall(int nargs, unsigned long *args);
 extern int __audit_sockaddr(int len, void *addr);
 extern void __audit_fd_pair(int fd1, int fd2);
@@ -239,7 +239,7 @@ static inline void audit_ipc_set_perm(unsigned long qbytes, 
uid_t uid, gid_t gid
 static inline int audit_bprm(struct linux_binprm *bprm)
 {
if (unlikely(!audit_dummy_context()))
-   return __audit_bprm(bprm);
+   __audit_bprm(bprm);
return 0;
 }
 static inline int audit_socketcall(int nargs, unsigned long *args)
diff --git a/kernel/audit.h b/kernel/audit.h
index 123c9b7..e7b94ab 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -197,6 +197,10 @@ struct audit_context {
int fd;
int flags;
} mmap;
+   struct {
+   int argc;
+   struct mm_struct*mm;
+   } execve;
};
int fds[2];
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index c9abaa0..eabe76a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -95,12 +95,6 @@ struct audit_aux_data {
 /* Number of target pids per aux struct. */
 #define AUDIT_AUX_PIDS 16
 
-struct audit_aux_data_execve {
-   struct audit_aux_data   d;
-   int argc;
-   struct mm_struct *mm;
-};
-
 struct audit_aux_data_pids {
struct audit_aux_data   d;
pid_t   target_pid[AUDIT_AUX_PIDS];
@@ -1144,20 +1138,19 @@ static int audit_log_single_execve_arg(struct 
audit_context *context,
 }
 
 static void audit_log_execve_info(struct audit_context *context,
- struct audit_buffer **ab,
- struct audit_aux_data_execve *axi)
+ struct audit_buffer **ab)
 {
int i, len;
size_t len_sent = 0;
const char __user *p;
char *buf;
 
-   if (axi-mm != current-mm)
+   if (context-execve.mm != current-mm)
return; /* execve failed, no additional info */
 
-   p = (const char __user *)axi-mm-arg_start;
+   p = (const char __user *)current-mm-arg_start;
 
-   audit_log_format(*ab, argc=%d, axi-argc);
+   audit_log_format(*ab, argc=%d, context-execve.argc);
 
/*
 * we need some kernel buffer to hold the userspace args.  Just
@@ -1171,7 +1164,7 @@ static void audit_log_execve_info(struct audit_context 
*context,
return;
}
 
-   for (i = 0; i  axi-argc; i++) {
+   for (i = 0; i  context-execve.argc; i++) {
len = audit_log_single_execve_arg(context, ab, i,
  len_sent, p, buf);
if (len = 0)
@@ -1274,6 +1267,9 @@ static void show_special(struct audit_context *context, 
int *call_panic)
audit_log_format(ab, fd=%d flags=0x%x, context-mmap.fd,
 context-mmap.flags);
break; }
+   case AUDIT_EXECVE: {
+   audit_log_execve_info(context, ab);
+   break; }
}
audit_log_end(ab);
 }
@@ -1320,11 +1316,6 @@ static void audit_log_exit(struct audit_context 
*context, struct task_struct *ts
 
switch (aux-type) {
 
-   case AUDIT_EXECVE: {
-   struct audit_aux_data_execve *axi = (void *)aux;
-   audit_log_execve_info(context, ab, axi);
-   break; }
-
case AUDIT_BPRM_FCAPS: {
struct audit_aux_data_bprm_fcaps *axs = (void *)aux;
audit_log_format(ab, fver=%x, axs-fcap_ver);
@@ -2121,21 +2112,13 @@ void __audit_ipc_set_perm(unsigned long qbytes, uid_t 
uid, gid_t gid, umode_t mo
context

Re: [PATCH] audit: Add cmdline to taskinfo output

[Richard Guy Briggs]

On Thu, Oct 31, 2013 at 08:24:11AM -0700, William Roberts wrote:
 On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb sgr...@redhat.com wrote:
  On Wednesday, October 30, 2013 01:18:13 PM William Roberts wrote:
   On Wed, Oct 30, 2013 at 12:42 PM, Steve Grubb sgr...@redhat.com wrote:
   I have compiled kernels in the past with custom COMM widths, but
   the memory footprint goes up, at least here were not keeping a
   bunch of possibly unused data around in the kernel plus we're not
   allocating anything on the common case of it being turned off.
 
  I don't like the idea of fields appearing and disappearing. The
  complaint is comm is meaningless. Let's fix that.
 
 Its not that the field is disappearing, its just whether or not you
 want the value printed out. cmdline=(null) vs cmdline=something.
 That's a trivial change of not making it dynamic which is what my
 first patch did but Richard Briggs suggested making it a dynamic
 feature and I was pretty ok with that.

Ok, so how about both fields are always present, but have some keyword
that is printed that indicates it is a duplicate of the other field?

Something like cmdline=(comm)

 William C Roberts

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/4][v2] audit: remove unused envc member of audit_aux_data_execve

[Richard Guy Briggs]

Get rid of write-only audit_aux_data_exeve structure member envc.

Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/auditsc.c |2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 24047f4..c9abaa0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -98,7 +98,6 @@ struct audit_aux_data {
 struct audit_aux_data_execve {
struct audit_aux_data   d;
int argc;
-   int envc;
struct mm_struct *mm;
 };
 
@@ -2132,7 +2131,6 @@ int __audit_bprm(struct linux_binprm *bprm)
return -ENOMEM;
 
ax-argc = bprm-argc;
-   ax-envc = bprm-envc;
ax-mm = bprm-mm;
ax-d.type = AUDIT_EXECVE;
ax-d.next = context-aux;
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Format specifier issue when building kernel

[Richard Guy Briggs]

On Mon, Oct 28, 2013 at 08:55:08PM -0700, William Roberts wrote:
 On Mon, Oct 28, 2013 at 6:43 PM, William Roberts
 bill.c.robe...@gmail.comwrote:
  On Mon, Oct 28, 2013 at 6:35 PM, Richard Guy Briggs r...@redhat.comwrote:
  On Mon, Oct 28, 2013 at 04:31:30PM -0700, William Roberts wrote:
   On Mon, Oct 28, 2013 at 4:30 PM, William Roberts
   bill.c.robe...@gmail.comwrote:
I've been working off of Richard Guy Brigs git repo on branch
audit-for-next prepping my patch and I noticed a build warning:
   
kernel/audit.c:832:8: warning: format ‘%A’ expects argument of type
‘double’, but argument 3 has type ‘char *’ [-Wformat]
   
Looking at the code, it looks wrong:
   
audit_log_format(ab,

   msg='%.AUDIT_MESSAGE_TEXT_MAXs',
(char *)data);
  
   The issue appears on the % specifier in there, it picks it up as
   %.A, which is of type double. Is this what was intended?
 
  Hmmm, that should have picked up a macro from 06051fbe in
  audit-for-next.  It should be pre-processed to %.8560s.
 
   William C Roberts
 
  - RGB
 
  The qoutes are wrong for that.
 
 Ok I see the value is not a string, but a numeric constant. Dont you need
 to do something like this:
 diff --git a/kernel/audit.c b/kernel/audit.c
 index bf4b1af..81dde3d 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -825,10 +825,12 @@ static int audit_receive_msg(struct sk_buff *skb,
 struct nlmsghdr *nlh)
 if (err)
 break;
 }
 +#define STR_HELPER(x) #x
 +#define STR(x) STR_HELPER(x)
 audit_log_common_recv_msg(ab, msg_type);
 if (msg_type != AUDIT_USER_TTY)
 audit_log_format(ab,
 -
 msg='%.AUDIT_MESSAGE_TEXT_MAXs',
 +
 msg='%.STR(AUDIT_MESSAGE_TEXT_MAX)s',
  (char *)data);
 else {
 int size;

Ugh.  That's not so easy to read...  Slightly longer, how about this?

diff --git a/kernel/audit.c b/kernel/audit.c
index 8378c5e..3f569d1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -824,11 +824,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
break;
}
audit_log_common_recv_msg(ab, msg_type);
-   if (msg_type != AUDIT_USER_TTY)
-   audit_log_format(ab,
- 
msg='%.AUDIT_MESSAGE_TEXT_MAXs',
-(char *)data);
-   else {
+   if (msg_type != AUDIT_USER_TTY) {
+   char fmt[64];
+   strcat(fmt,  msg='%.);
+   strcat(fmt, AUDIT_MESSAGE_TEXT_MAX);
+   strcat(fmt, s');
+   audit_log_format(ab, fmt, (char *)data);
+   } else {
int size;
 
audit_log_format(ab,  data=);

 Unless their is some gnu-magic I don't know about.
 
 William C Roberts

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Format specifier issue when building kernel

[Richard Guy Briggs]

On Thu, Oct 31, 2013 at 12:25:55PM -0700, William Roberts wrote:
  +   if (msg_type != AUDIT_USER_TTY) {
  +   char fmt[64];
  +   strcat(fmt,  msg='%.);
  +   strcat(fmt, AUDIT_MESSAGE_TEXT_MAX);
  +   strcat(fmt, s');
  +   audit_log_format(ab, fmt, (char *)data);
  +   } else {
 
 I am ok with this. In fact I was going to do this the first time, but I
 thought their would be some explicit reason to avoid the additional
 run time overhead as the concat could be made at compile time.

Ok, this was in danger of starting with fmt in an unknown state.  Latest
patch: 

diff --git a/kernel/audit.c b/kernel/audit.c
@@ -148,6 +148,8 @@ DEFINE_MUTEX(audit_cmd_mutex);
  * should be at least that large. */
 #define AUDIT_BUFSIZ 1024
 
+char usermsg_format[64] = ;
+
 /* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
  * audit_freelist.  Doing so eliminates many kmalloc/kfree calls. */
 #define AUDIT_MAXFREE  (2*NR_CPUS)
@@ -714,11 +716,15 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
break;
}
audit_log_common_recv_msg(ab, msg_type);
-   if (msg_type != AUDIT_USER_TTY)
-   audit_log_format(ab,
- 
msg='%.AUDIT_MESSAGE_TEXT_MAXs',
+   if (msg_type != AUDIT_USER_TTY) {
+   if (unlikely(usermsg_format[0] == 0))
+   snprintf(usermsg_format,
+   sizeof(usermsg_format),
+msg=\'%%.%ds\', 
+   AUDIT_MESSAGE_TEXT_MAX);
+   audit_log_format(ab, usermsg_format,
 (char *)data);
-   else {
+   } else {
int size;
 
audit_log_format(ab,  data=);

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Format specifier issue when building kernel

[Richard Guy Briggs]

On Fri, Nov 01, 2013 at 12:34:55PM -0400, Steve Grubb wrote:
 On Friday, November 01, 2013 12:24:55 PM Richard Guy Briggs wrote:
  On Thu, Oct 31, 2013 at 12:25:55PM -0700, William Roberts wrote:
+   if (msg_type != AUDIT_USER_TTY) {
+   char fmt[64];
+   strcat(fmt,  msg='%.);
+   strcat(fmt, AUDIT_MESSAGE_TEXT_MAX);
+   strcat(fmt, s');
+   audit_log_format(ab, fmt, (char *)data);
+   } else {
   
   I am ok with this. In fact I was going to do this the first time, but I
   thought their would be some explicit reason to avoid the additional
   run time overhead as the concat could be made at compile time.
  
  Ok, this was in danger of starting with fmt in an unknown state.  Latest
  patch:
  
  diff --git a/kernel/audit.c b/kernel/audit.c
  @@ -148,6 +148,8 @@ DEFINE_MUTEX(audit_cmd_mutex);
* should be at least that large. */
   #define AUDIT_BUFSIZ 1024
  
  +char usermsg_format[64] = ;
 
 You might want this ^^^  to be static so its not global in scope.

Yup, good point.  Thanks.

 -Steve
 
   /* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
* audit_freelist.  Doing so eliminates many kmalloc/kfree calls. */
   #define AUDIT_MAXFREE  (2*NR_CPUS)
  @@ -714,11 +716,15 @@ static int audit_receive_msg(struct sk_buff *skb,
  struct nlmsghdr *nlh) break;
  }
  audit_log_common_recv_msg(ab, msg_type);
  -   if (msg_type != AUDIT_USER_TTY)
  -   audit_log_format(ab,
  - 
  msg='%.AUDIT_MESSAGE_TEXT_MAXs',
  +   if (msg_type != AUDIT_USER_TTY) {
  +   if (unlikely(usermsg_format[0] == 0))
  +   snprintf(usermsg_format,
  +   sizeof(usermsg_format),
  +msg=\'%%.%ds\',
  +   AUDIT_MESSAGE_TEXT_MAX);
  +   audit_log_format(ab, usermsg_format,
   (char *)data);
  -   else {
  +   } else {
  int size;
  
  audit_log_format(ab,  data=);
  
  - RGB
  
  --
  Richard Guy Briggs rbri...@redhat.com
  Senior Software Engineer
  Kernel Security
  AMER ENG Base Operating Systems
  Remote, Ottawa, Canada
  Voice: +1.647.777.2635
  Internal: (81) 32635
  Alt: +1.613.693.0684x3545
  
  --
  Linux-audit mailing list
  Linux-audit@redhat.com
  https://www.redhat.com/mailman/listinfo/linux-audit
 

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Format specifier issue when building kernel

[Richard Guy Briggs]

On Fri, Nov 01, 2013 at 12:38:15PM -0400, Richard Guy Briggs wrote:
 On Fri, Nov 01, 2013 at 12:34:55PM -0400, Steve Grubb wrote:
  On Friday, November 01, 2013 12:24:55 PM Richard Guy Briggs wrote:
   On Thu, Oct 31, 2013 at 12:25:55PM -0700, William Roberts wrote:
   +char usermsg_format[64] = ;
  
  You might want this ^^^  to be static so its not global in scope.
 
 Yup, good point.  Thanks.

Better yet, make it local to that if statement, but Eric Paris has a far
more readable and elegant solution:

diff --git a/kernel/audit.c b/kernel/audit.c
@@ -864,8 +864,8 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
}
audit_log_common_recv_msg(ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
-   audit_log_format(ab,
- 
msg='%.AUDIT_MESSAGE_TEXT_MAXs',
+   audit_log_format(ab,  msg='%.*s',
+AUDIT_MESSAGE_TEXT_MAX,
 (char *)data);
else {
int size;

I forgot about the * format specifier...

  -Steve
  
   - RGB
 
 - RGB

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 5/5] audit: change the type of oldloginuid from kuid_t to unsigned long

[Richard Guy Briggs]

On Fri, Nov 01, 2013 at 07:34:46PM +0800, Gao feng wrote:
 The type of oldloginuid should be unsigned long.

Can you say why unsigned long rather than int returned from
audit_get_sessionid() or unsigned int expected by
audit_log_set_loginuid()?

Kees: For that matter, why does audit_get_sessionid() return int rather
than unsigned int from task_struct?  That was introduced in commit
9321d526.

 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 ---
  kernel/auditsc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index ceb396f..9f871ad 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -2018,7 +2018,8 @@ int audit_set_loginuid(kuid_t loginuid)
  {
   struct task_struct *task = current;
   unsigned int sessionid = -1;
 - kuid_t oldloginuid, oldsessionid;
 + kuid_t oldloginuid;
 + unsigned long oldsessionid;
   int rc;
  
   oldloginuid = audit_get_loginuid(current);
 -- 
 1.8.3.1

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Richard Guy Briggs]

On Fri, May 24, 2013 at 12:11:44PM -0400, Eric Paris wrote:
 The audit_status structure was not designed with extensibility in mind.
 Define a new AUDIT_SET_FEATURE message type which takes a new structure
 of bits where things can be enabled/disabled/locked one at a time.  This
 structure should be able to grow in the future while maintaining forward
 and backward compatibility (based loosly on the ideas from capabilities
 and prctl)
 
 This does not actually add any features, but is just infrastructure to
 allow new on/off types of audit system features.

However, it does surprisingly disable one!

 diff --git a/kernel/audit.c b/kernel/audit.c
 index f2f4666..3acbbc8 100644
 --- a/kernel/audit.c
 +++ b/kernel/audit.c
 @@ -699,7 +798,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
   if (status_get-mask  AUDIT_STATUS_BACKLOG_LIMIT)
   err = 
 audit_set_backlog_limit(status_get-backlog_limit);
   break;
 - case AUDIT_USER:
 + case AUDIT_GET_FEATURE:
 + err = audit_get_feature(skb);
 + if (err)
 + return err;
 + break;
 + case AUDIT_SET_FEATURE:
 + err = audit_set_feature(skb);
 + if (err)
 + return err;
 + break;
   case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
   case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
   if (!audit_enabled  msg_type != AUDIT_USER_AVC)

Can I assume that the removal of the AUDIT_USER case line was
accidental?  It has broken USER type AUDIT messages.


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH v2] audit: fix incorrect type of sessionid

[Richard Guy Briggs]

On Sat, Nov 02, 2013 at 02:45:02PM +0800, Gao feng wrote:
 The type of task-sessionid is unsigned int, the return
 type of audit_get_sessionid should be consistent with it.
 
 And this patch also changes the type of oldsessionid to
 unsigned int.

Looks good, thanks!

 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 ---
  include/linux/audit.h | 4 ++--
  kernel/auditsc.c  | 3 ++-
  2 files changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/include/linux/audit.h b/include/linux/audit.h
 index 7b31bec..01b40f7 100644
 --- a/include/linux/audit.h
 +++ b/include/linux/audit.h
 @@ -202,7 +202,7 @@ static inline kuid_t audit_get_loginuid(struct 
 task_struct *tsk)
   return tsk-loginuid;
  }
  
 -static inline int audit_get_sessionid(struct task_struct *tsk)
 +static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
  {
   return tsk-sessionid;
  }
 @@ -360,7 +360,7 @@ static inline kuid_t audit_get_loginuid(struct 
 task_struct *tsk)
  {
   return INVALID_UID;
  }
 -static inline int audit_get_sessionid(struct task_struct *tsk)
 +static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
  {
   return -1;
  }
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index ceb396f..e4aaa9d 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -2018,7 +2018,8 @@ int audit_set_loginuid(kuid_t loginuid)
  {
   struct task_struct *task = current;
   unsigned int sessionid = -1;
 - kuid_t oldloginuid, oldsessionid;
 + kuid_t oldloginuid;
 + unsigned int oldsessionid;
   int rc;
  
   oldloginuid = audit_get_loginuid(current);
 -- 
 1.8.3.1
 

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC Part1 PATCH 00/20 v2] Add namespace support for audit

[Richard Guy Briggs]

On Tue, Nov 05, 2013 at 04:56:55PM +0800, Gao feng wrote:
 On 11/05/2013 04:11 PM, Li Zefan wrote:
  On 2013/11/5 15:52, Gao feng wrote:
  On 11/05/2013 03:51 PM, Gao feng wrote:
  Ping...
 
  I want to catch up the merge window..
  
  Even if your patches are accepted by a certain maintainer immediately,
  he will in no doubt queue them for 3.14.
 
 Yes, you are right...
 But I still want to get some comments..

Definitely won't make it in in this merge window.  I agree it needs
discussion, but I won't start that for at least another week (I'm
currently at IETF in Vancouver.).


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] Dropped audit_log_abend()

[Richard Guy Briggs]

On Fri, Nov 08, 2013 at 10:44:53AM -0500, Eric Paris wrote:
 On Fri, 2013-11-08 at 09:57 +0530, Paul Davies C wrote:
  The audit_log_abend() is used only by the audit_core_dumps(). Thus there is 
  no
  need of maintaining the audit_log_abend() as a separate function.
  
  This patch drops the audit_log_abend() and pushes its functionalities back 
  to
  the audit_core_dumps(). Apart from that the reason field is also dropped
  from being logged since the reason can be deduced from the signal number.
  
  Signed-off-by: Paul Davies C pauldavi...@gmail.com
 
 Acked-by: Eric Paris epa...@redhat.com

Looks fine to me.  I'll queue it.

  ---
   kernel/auditsc.c |   10 ++
   1 file changed, 2 insertions(+), 8 deletions(-)
  
  diff --git a/kernel/auditsc.c b/kernel/auditsc.c
  index 9845cb3..f2aa62a 100644
  --- a/kernel/auditsc.c
  +++ b/kernel/auditsc.c
  @@ -2368,13 +2368,6 @@ static void audit_log_task(struct audit_buffer *ab)
  audit_log_untrustedstring(ab, current-comm);
   }
   
  -static void audit_log_abend(struct audit_buffer *ab, char *reason, long 
  signr)
  -{
  -   audit_log_task(ab);
  -   audit_log_format(ab,  reason=);
  -   audit_log_string(ab, reason);
  -   audit_log_format(ab,  sig=%ld, signr);
  -}
   /**
* audit_core_dumps - record information about processes that end 
  abnormally
* @signr: signal value
  @@ -2395,7 +2388,8 @@ void audit_core_dumps(long signr)
  ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
  if (unlikely(!ab))
  return;
  -   audit_log_abend(ab, memory violation, signr);
  +   audit_log_task(ab);
  +   audit_log_format(ab,  sig=%ld, signr);
  audit_log_end(ab);
   }
   

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: proposing [PATCH] audit: get rid of *NO* daemon at audit_pid=0 message

[Richard Guy Briggs]

On Mon, Nov 11, 2013 at 12:21:33PM -0500, Eric Paris wrote:
 On Wed, 2013-10-30 at 00:05 +0100, Mateusz Guzik wrote:
  Hello,
  
  I wrote a trivial patch for what I believe is a subsystem you maintain.
  
  I'm sending it privately first to ensure it looks ok at has proper
  recipients (I'm new to linux world, sorry :).
  
  'To' would be: linux-audit@redhat.com
  
  The rest is:
  
  From: Mateusz Guzik mgu...@redhat.com
  Date: Tue, 29 Oct 2013 23:51:52 +0100
  Subject: [PATCH] audit: get rid of *NO* daemon at audit_pid=0 message
  
  kauditd_send_skb is called after audit_pid was checked to be non-zero.
  
  However, it can be set to 0 due to auditd exiting while kauditd_send_skb
  is still executed and this can result in a spurious warning about missing
  auditd.
  
  Re-check audit_pid before printing the message.

I'm ok with this.  It can't hurt.  I'll queue it.

  Signed-off-by: Mateusz Guzik mgu...@redhat.com
  Cc: Eric Paris epa...@redhat.com
  Cc: linux-ker...@vger.kernel.org
 
 Acked-by: Eric Paris epa...@redhat.com
 
   kernel/audit.c | 8 +---
   1 file changed, 5 insertions(+), 3 deletions(-)
  
  diff --git a/kernel/audit.c b/kernel/audit.c
  index 7b0e23a..a91a965 100644
  --- a/kernel/audit.c
  +++ b/kernel/audit.c
  @@ -388,9 +388,11 @@ static void kauditd_send_skb(struct sk_buff *skb)
  err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
  if (err  0) {
  BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
  -   printk(KERN_ERR audit: *NO* daemon at audit_pid=%d\n, 
  audit_pid);
  -   audit_log_lost(auditd disappeared\n);
  -   audit_pid = 0;
  +   if (audit_pid) {
  +   printk(KERN_ERR audit: *NO* daemon at audit_pid=%d\n, 
  audit_pid);
  +   audit_log_lost(auditd disappeared\n);
  +   audit_pid = 0;
  +   }
  /* we might get lucky and get this in the next auditd */
  audit_hold_skb(skb);
  } else
  -- 
  1.8.3.1
  
  Is this ok?
  
  Thanks,

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

logging changes in tty logging status

[Richard Guy Briggs]

Hi Steve,

I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
case doesn't log a configuration change.  Should it?


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: logging changes in tty logging status

[Richard Guy Briggs]

On Wed, Nov 13, 2013 at 03:22:49PM -0500, Steve Grubb wrote:
 On Wednesday, November 13, 2013 03:04:18 PM Richard Guy Briggs wrote:
  Hi Steve,
  
  I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
  case doesn't log a configuration change.  Should it?
 
 Yes, it should. Any change in config should be recorded with subject, old 
 value, new value, and results. It should match other config change events.

So perhaps something like this, but should probably re-structure the
code to make it cleaner and re-factor a formatting function...

Any opinion on the labels/tags?

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..cba0109 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -829,18 +829,36 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
case AUDIT_TTY_SET: {
struct audit_tty_status s;
struct task_struct *tsk = current;
+   struct audit_buffer *ab;
 
memset(s, 0, sizeof(s));
/* guard against past and future API changes */
memcpy(s, data, min(sizeof(s), (size_t)nlh-nlmsg_len));
+   audit_log_common_recv_msg(ab, AUDIT_CONFIG_CHANGE);
+   audit_log_format(ab,  old.audit_tty_status.enabled=%d
+ old.audit_tty_status.log_passwd=%d,
+tsk-signal-audit_tty,
+tsk-signal-audit_tty_log_passwd);
+   audit_log_format(ab,  new.audit_tty_status.enabled=%d
+ new.audit_tty_status.log_passwd=%d,
+s.enabled, s.log_passwd);
if ((s.enabled != 0  s.enabled != 1) ||
(s.log_passwd != 0  s.log_passwd != 1))
-   return -EINVAL;
+{
+   audit_log_format(ab,  res=0);
+   audit_log_end(ab);
 
+   return -EINVAL;
+}
spin_lock(tsk-sighand-siglock);
tsk-signal-audit_tty = s.enabled;
tsk-signal-audit_tty_log_passwd = s.log_passwd;
spin_unlock(tsk-sighand-siglock);
+
+   audit_log_format(ab,  res=1);
+   audit_log_end(ab);
+
+
break;
}
default:

 -Steve

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] audit: Allow auditing of proc/self/cmdline value

[Richard Guy Briggs]

On Mon, Nov 18, 2013 at 04:41:19PM -0800, William Roberts wrote:
 Audit records will now contain a new field, cmdline.
 This is the value that is stored in proc/self/cmdline,
 and is useful for debugging when processes are being run
 via VM's. A primary example of this is Android, in which
 package names are set in this location, and thread names
 are set via PR_SET_NAME. The other benefit is this
 is not limited to 16 bytes as COMM historically has.

This patch looks good to me.

 Change-Id: I9bf0928a8aa249d22ecd55fa9cd27325dd394eb1
 Signed-off-by: William Roberts wrobe...@tresys.com
 ---
  fs/proc/base.c  |2 +-
  include/linux/proc_fs.h |1 +
  kernel/auditsc.c|   33 +
  3 files changed, 35 insertions(+), 1 deletion(-)
 
 diff --git a/fs/proc/base.c b/fs/proc/base.c
 index 2f198da..25b73d3 100644
 --- a/fs/proc/base.c
 +++ b/fs/proc/base.c
 @@ -209,7 +209,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
   return mm_access(task, PTRACE_MODE_READ);
  }
  
 -static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 +int proc_pid_cmdline(struct task_struct *task, char *buffer)
  {
   int res = 0;
   unsigned int len;
 diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
 index 85c5073..d85ac14 100644
 --- a/include/linux/proc_fs.h
 +++ b/include/linux/proc_fs.h
 @@ -118,6 +118,7 @@ struct pid_namespace;
  
  extern int pid_ns_prepare_proc(struct pid_namespace *ns);
  extern void pid_ns_release_proc(struct pid_namespace *ns);
 +extern int proc_pid_cmdline(struct task_struct *task, char *buffer);
  
  /*
   * proc_tty.c
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index 27ad9dd..45fd3d0 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -67,6 +67,7 @@
  #include linux/syscalls.h
  #include linux/capability.h
  #include linux/fs_struct.h
 +#include linux/proc_fs.h
  
  #include audit.h
  
 @@ -1153,6 +1154,37 @@ error_path:
  
  EXPORT_SYMBOL(audit_log_task_context);
  
 +static void audit_log_add_cmdline(struct audit_buffer *ab,
 +   struct task_struct *tsk)
 +{
 + int len;
 + unsigned long page;
 + char *msg = (null);
 +
 + audit_log_format(ab,  cmdline=);
 +
 + /* Get the process cmdline */
 + page = __get_free_page(GFP_TEMPORARY);
 + if (!page) {
 + audit_log_untrustedstring(ab, msg);
 + return;
 + }
 + len = proc_pid_cmdline(tsk, (char *)page);
 + if (len = 0) {
 + free_page(page);
 + audit_log_untrustedstring(ab, msg);
 + return;
 + }
 + /*
 +  * Ensure NULL terminated! Application could
 +  * could be using setproctitle(3).
 +  */
 + ((char *)page)[len-1] = '\0';
 + msg = (char *)page;
 + audit_log_untrustedstring(ab, msg);
 + free_page(page);
 +}
 +
  static void audit_log_task_info(struct audit_buffer *ab, struct task_struct 
 *tsk)
  {
   char name[sizeof(tsk-comm)];
 @@ -1179,6 +1211,7 @@ static void audit_log_task_info(struct audit_buffer 
 *ab, struct task_struct *tsk
   }
   up_read(mm-mmap_sem);
   }
 + audit_log_add_cmdline(ab, tsk);
   audit_log_task_context(ab);
  }
  
 -- 
 1.7.9.5
 

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] audit: Enable cacheing of cmdline in audit_context

[Richard Guy Briggs]

On Mon, Nov 18, 2013 at 04:41:20PM -0800, William Roberts wrote:
 Not enitrely sure if this is getting us any benefit, as in my
 environment, these contexts are getting free'd immediately.
 
 Change-Id: Ia0d432bc4aba8588840f0dc0026a1e9483e5b485
 Signed-off-by: William Roberts wrobe...@tresys.com
 ---
  kernel/auditsc.c |   48 +---
  1 file changed, 37 insertions(+), 11 deletions(-)
 
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index 45fd3d0..4b30c5d 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -270,6 +270,7 @@ struct audit_context {
   } mmap;
   };
   int fds[2];
 + unsigned long cmdline;

Would this be better declared to avoid all the casts?:

char *cmdline;

  #if AUDIT_DEBUG
   int put_count;
 @@ -1044,6 +1045,14 @@ static inline void audit_free_aux(struct audit_context 
 *context)
   }
  }
  
 +static inline void audit_cmdline_free(struct audit_context *ctx)
 +{
 + if (!ctx-cmdline)
 + return;
 + free_page(ctx-cmdline);
 + ctx-cmdline = 0;

ctx-cmdline = NULL;

 +}
 +
  static inline void audit_zero_context(struct audit_context *context,
 enum audit_state state)
  {
 @@ -1118,6 +1127,7 @@ static inline void audit_free_context(struct 
 audit_context *context)
   audit_free_aux(context);
   kfree(context-filterkey);
   kfree(context-sockaddr);
 + audit_cmdline_free(context);
   kfree(context);
   context  = previous;
   } while (context);
 @@ -1154,35 +1164,51 @@ error_path:
  
  EXPORT_SYMBOL(audit_log_task_context);
  
 -static void audit_log_add_cmdline(struct audit_buffer *ab,
 +static unsigned long audit_cmdline_get_page(struct audit_buffer *ab,

   static char* audit_cmdline_get_page(struct audit_buffer *ab,

 struct task_struct *tsk)
  {
   int len;
   unsigned long page;
 - char *msg = (null);
 -
 - audit_log_format(ab,  cmdline=);
  
   /* Get the process cmdline */
   page = __get_free_page(GFP_TEMPORARY);
   if (!page) {
 - audit_log_untrustedstring(ab, msg);
 - return;
 + return 0;

return NULL;

   }
   len = proc_pid_cmdline(tsk, (char *)page);
   if (len = 0) {
   free_page(page);
 - audit_log_untrustedstring(ab, msg);
 - return;
 + return 0;

return NULL;

   }
   /*
* Ensure NULL terminated! Application could
* could be using setproctitle(3).
*/
   ((char *)page)[len-1] = '\0';
 - msg = (char *)page;
 +
 + /* TODO: Re-alloc to something smaller then a page here? */
 + return page;
 +}
 +
 +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct 
 *tsk,
 +   struct audit_context *context)
 +{
 + char *msg = (null);
 +
 + audit_log_format(ab,  cmdline=);
 +
 + /* Already cached */
 + if (context-cmdline) {
 + msg = (char *)context-cmdline;

msg = context-cmdline;

 + goto out;
 + }
 + /* Not cached yet */
 + context-cmdline = audit_cmdline_get_page(ab, tsk);
 + if (!context-cmdline)
 + goto out;
 + msg = (char *)context-cmdline;

msg = context-cmdline;

 +out:
   audit_log_untrustedstring(ab, msg);
 - free_page(page);
  }
  
  static void audit_log_task_info(struct audit_buffer *ab, struct task_struct 
 *tsk)
 @@ -1211,7 +1237,6 @@ static void audit_log_task_info(struct audit_buffer 
 *ab, struct task_struct *tsk
   }
   up_read(mm-mmap_sem);
   }
 - audit_log_add_cmdline(ab, tsk);
   audit_log_task_context(ab);
  }
  
 @@ -1679,6 +1704,7 @@ static void audit_log_exit(struct audit_context 
 *context, struct task_struct *ts
  
  
   audit_log_task_info(ab, tsk);
 + audit_log_cmdline(ab, tsk, context);
   audit_log_key(ab, context-filterkey);
   audit_log_end(ab);
  
 -- 
 1.7.9.5
 

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] audit: Enable cacheing of cmdline in audit_context

[Richard Guy Briggs]

On Tue, Nov 19, 2013 at 07:44:33AM -0800, William Roberts wrote:
 On Tue, Nov 19, 2013 at 7:40 AM, Richard Guy Briggs r...@redhat.com wrote:
  On Mon, Nov 18, 2013 at 04:41:20PM -0800, William Roberts wrote:
  Not enitrely sure if this is getting us any benefit, as in my
  environment, these contexts are getting free'd immediately.
 
  Change-Id: Ia0d432bc4aba8588840f0dc0026a1e9483e5b485
  Signed-off-by: William Roberts wrobe...@tresys.com
  ---
   kernel/auditsc.c |   48 +---
   1 file changed, 37 insertions(+), 11 deletions(-)
 
  diff --git a/kernel/auditsc.c b/kernel/auditsc.c
  index 45fd3d0..4b30c5d 100644
  --- a/kernel/auditsc.c
  +++ b/kernel/auditsc.c
  @@ -270,6 +270,7 @@ struct audit_context {
} mmap;
};
int fds[2];
  + unsigned long cmdline;
 
  Would this be better declared to avoid all the casts?:
 
 This is indirectly coupled with my todo as well. So the value could be
 up to a page big, and I allocate and cache a page (which is returned
 as an usnigned long).
 But, often times, the value is much smaller. Should I just alloc a
 seperate and smaller buffer with kmalloc, and
 cache that as a char * to avoid the casts? Then free the page?

Do you know the size ahead of time?  If so, just allocate that.

 Either way, I have no objection to taking the interface to a char *, I
 battled that decision in my head while I was coding.

Perhaps I am being naive, but is there any benefit to referring to a
page as an unsigned long rather than as a char*?  If it is only ever
used as a char* (except by free_page()) can it hurt?

For that matter, is there a benefit to doing a __get_free_page() rather
than kmalloc()?

  char *cmdline;
 
   #if AUDIT_DEBUG
int put_count;
  @@ -1044,6 +1045,14 @@ static inline void audit_free_aux(struct 
  audit_context *context)
}
   }
 
  +static inline void audit_cmdline_free(struct audit_context *ctx)
  +{
  + if (!ctx-cmdline)
  + return;
  + free_page(ctx-cmdline);
  + ctx-cmdline = 0;
 
  ctx-cmdline = NULL;
 
  +}
  +
   static inline void audit_zero_context(struct audit_context *context,
  enum audit_state state)
   {
  @@ -1118,6 +1127,7 @@ static inline void audit_free_context(struct 
  audit_context *context)
audit_free_aux(context);
kfree(context-filterkey);
kfree(context-sockaddr);
  + audit_cmdline_free(context);
kfree(context);
context  = previous;
} while (context);
  @@ -1154,35 +1164,51 @@ error_path:
 
   EXPORT_SYMBOL(audit_log_task_context);
 
  -static void audit_log_add_cmdline(struct audit_buffer *ab,
  +static unsigned long audit_cmdline_get_page(struct audit_buffer *ab,
 
 static char* audit_cmdline_get_page(struct audit_buffer *ab,
 
  struct task_struct *tsk)
   {
int len;
unsigned long page;
  - char *msg = (null);
  -
  - audit_log_format(ab,  cmdline=);
 
/* Get the process cmdline */
page = __get_free_page(GFP_TEMPORARY);
if (!page) {
  - audit_log_untrustedstring(ab, msg);
  - return;
  + return 0;
 
  return NULL;
 
}
len = proc_pid_cmdline(tsk, (char *)page);
if (len = 0) {
free_page(page);
  - audit_log_untrustedstring(ab, msg);
  - return;
  + return 0;
 
  return NULL;
 
}
/*
 * Ensure NULL terminated! Application could
 * could be using setproctitle(3).
 */
((char *)page)[len-1] = '\0';
  - msg = (char *)page;
  +
  + /* TODO: Re-alloc to something smaller then a page here? */
  + return page;
  +}
  +
  +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct 
  *tsk,
  +   struct audit_context *context)
  +{
  + char *msg = (null);
  +
  + audit_log_format(ab,  cmdline=);
  +
  + /* Already cached */
  + if (context-cmdline) {
  + msg = (char *)context-cmdline;
 
  msg = context-cmdline;
 
  + goto out;
  + }
  + /* Not cached yet */
  + context-cmdline = audit_cmdline_get_page(ab, tsk);
  + if (!context-cmdline)
  + goto out;
  + msg = (char *)context-cmdline;
 
  msg = context-cmdline;
 
  +out:
audit_log_untrustedstring(ab, msg);
  - free_page(page);
   }
 
   static void audit_log_task_info(struct audit_buffer *ab, struct 
  task_struct *tsk)
  @@ -1211,7 +1237,6 @@ static void audit_log_task_info(struct audit_buffer 
  *ab, struct task_struct *tsk
}
up_read(mm-mmap_sem);
}
  - audit_log_add_cmdline(ab, tsk);
audit_log_task_context(ab);
   }
 
  @@ -1679,6 +1704,7

Re: [PATCH 1/1] Added exe field to audit core dump signal log

[Richard Guy Briggs]

On Thu, Nov 14, 2013 at 08:56:57AM +0530, Paul Davies C wrote:
 Currently when the coredump signals are logged by the audit system , the
 actual path to the executable is not logged. Without details of exe , the
 system admin may not have an exact idea on what program failed.
 
 This patch changes the audit_log_task() so that the path to the exe is also
 logged.

Out of curiosity, on which platform are you observing this?  This sounds
related to Bill Roberts' recent cmdline patches.

I also wonder how reliable this is, or whether it could have been 
changed from under us by deletion or rename after invocation.

This BZ sounds related:
https://bugzilla.redhat.com/show_bug.cgi?id=837856
https://bugzilla.redhat.com/show_bug.cgi?id=831684

 Signed-off-by: Paul Davies C pauldavi...@gmail.com
 ---
  kernel/auditsc.c |7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/kernel/auditsc.c b/kernel/auditsc.c
 index 9845cb3..988de72 100644
 --- a/kernel/auditsc.c
 +++ b/kernel/auditsc.c
 @@ -2353,6 +2353,7 @@ static void audit_log_task(struct audit_buffer *ab)
   kuid_t auid, uid;
   kgid_t gid;
   unsigned int sessionid;
 + struct mm_struct *mm = current-mm;
  
   auid = audit_get_loginuid(current);
   sessionid = audit_get_sessionid(current);
 @@ -2366,6 +2367,12 @@ static void audit_log_task(struct audit_buffer *ab)
   audit_log_task_context(ab);
   audit_log_format(ab,  pid=%d comm=, current-pid);
   audit_log_untrustedstring(ab, current-comm);
 + if (mm) {
 + down_read(mm-mmap_sem);
 + if (mm-exe_file)
 + audit_log_d_path(ab,  exe=, mm-exe_file-f_path);
 + up_read(mm-mmap_sem);
 + }
  }
  
  static void audit_log_abend(struct audit_buffer *ab, char *reason, long 
 signr)
 -- 
 1.7.9.5

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/1] Added exe field to audit core dump signal log

[Richard Guy Briggs]

On Wed, Nov 20, 2013 at 02:07:58PM -0800, William Roberts wrote:
 On Wed, Nov 20, 2013 at 2:03 PM, William Roberts
 bill.c.robe...@gmail.com wrote:
  On Wed, Nov 20, 2013 at 1:47 PM, Richard Guy Briggs r...@redhat.com wrote:
  On Thu, Nov 14, 2013 at 08:56:57AM +0530, Paul Davies C wrote:
  + if (mm) {
  + down_read(mm-mmap_sem);
  + if (mm-exe_file)
  + audit_log_d_path(ab,  exe=, 
  mm-exe_file-f_path);
  + up_read(mm-mmap_sem);
  + }
 
 snip
 One other thing that I know Steve Grubb is picky on, is the field
 still needs to be there even if mm is null. We can't have
 disappearing fields. On error conditions, I've been doing
 fieldname=(null) on my patches.

Agreed.

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Follow up on command line auditing

[Richard Guy Briggs]

On Mon, Dec 02, 2013 at 07:42:20AM -0800, William Roberts wrote:
 Changelog since last post:
 * Rebase on latest master
 
 [PATCH] audit: Audit proc cmdline value

Hi Bill,

I wasn't expecting that you would squash everything down into one patch.
I think it should be at least two.  I'm comfortable with the changes in
the audit subsystem.  Could those be one patch?  As for the changes to
proc (including base and util) those might be better as a seperate
patch.


- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Follow up on command line auditing

[Richard Guy Briggs]

On Mon, Dec 02, 2013 at 08:20:10AM -0800, William Roberts wrote:
 On Mon, Dec 2, 2013 at 8:07 AM, Richard Guy Briggs r...@redhat.com wrote:
  On Mon, Dec 02, 2013 at 07:42:20AM -0800, William Roberts wrote:
  Changelog since last post:
  * Rebase on latest master
 
  [PATCH] audit: Audit proc cmdline value
 
  Hi Bill,
 
  I wasn't expecting that you would squash everything down into one patch.
  I think it should be at least two.  I'm comfortable with the changes in
  the audit subsystem.  Could those be one patch?  As for the changes to
  proc (including base and util) those might be better as a seperate
  patch.
 
 Richard,
 Ok so what do you think the best way forward is? I don't want to duplicate
 code from proc/base.c. I would need to export proc_pid_cmdline()
 in the first patch or re-implement it in the audit subsystem, followed
 by a patch
 to merge the functionality. What would you prefer?

I would split them into 3 patches:

1) implement the length and copy funcitons:
 include/linux/mm.h |7 +
 mm/util.c  |   48 ++

2) use them in the proc call:
 fs/proc/base.c |   35 +++---

3) use them in audit:
 kernel/audit.h |1 +
 kernel/auditsc.c   |   82 


Does this split make sense?  Combining 1 and 2 might be acceptable to
those subsystem maintainers...

 Bill

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Follow up on command line auditing

[Richard Guy Briggs]

On Mon, Dec 02, 2013 at 10:10:27AM -0800, William Roberts wrote:
 On Mon, Dec 2, 2013 at 9:18 AM, Richard Guy Briggs r...@redhat.com wrote:
  On Mon, Dec 02, 2013 at 08:20:10AM -0800, William Roberts wrote:
  On Mon, Dec 2, 2013 at 8:07 AM, Richard Guy Briggs r...@redhat.com wrote:
   On Mon, Dec 02, 2013 at 07:42:20AM -0800, William Roberts wrote:
   Changelog since last post:
   * Rebase on latest master
  
   [PATCH] audit: Audit proc cmdline value
  
   Hi Bill,
  
   I wasn't expecting that you would squash everything down into one patch.
   I think it should be at least two.  I'm comfortable with the changes in
   the audit subsystem.  Could those be one patch?  As for the changes to
   proc (including base and util) those might be better as a seperate
   patch.
 
  Richard,
  Ok so what do you think the best way forward is? I don't want to duplicate
  code from proc/base.c. I would need to export proc_pid_cmdline()
  in the first patch or re-implement it in the audit subsystem, followed
  by a patch
  to merge the functionality. What would you prefer?
 
  I would split them into 3 patches:
 
  1) implement the length and copy funcitons:
   include/linux/mm.h |7 +
   mm/util.c  |   48 ++
 
  2) use them in the proc call:
   fs/proc/base.c |   35 +++---
 
  3) use them in audit:
   kernel/audit.h |1 +
   kernel/auditsc.c   |   82 
  
 
  Does this split make sense?  Combining 1 and 2 might be acceptable to
  those subsystem maintainers...
 
 You read my mind here after I sent this, this is exactly what I was thinking.
 
 When I am done do I publish this to kernel mainline, here, or elsewhere?

Both here and lkml would make sense.  Find the respective maintainers
using scripts/get_maintainer.pl and Cc: them.

 Bill

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Namespaces in event records

[Richard Guy Briggs]

On Tue, Dec 03, 2013 at 11:24:12AM +0100, Ondrej Moris wrote:
 Hi, I am wondering if there is a way to get namespaces related to an
 audit event? There are obviously no namespace fields and I do not
 see them in the message as well. It might be important to audit a
 namespace of the process causing the event... or not?

That does sound potentially useful.  I've been working on reducing some
of the restrictions caused by the introduciton of namespaces on audit,
in particular, dealing with net namespaces and pid namespaces.  I'm
still looking at user namespaces with caution.  I've not dealt with
mount, uts and ipc namespaces.

In particular to your concerns, I'd looked at how to identify namespaces
in debug or log output and hadn't yet come up with anything satisfying
yet.

 Ondrej Moriš, RHCSA, RHCE, RHCSS, RHCVA

- RGB

--
Richard Guy Briggs rbri...@redhat.com
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red 
Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 3/3] audit: drop audit_cmd_lock in AUDIT_USER family of cases

[Richard Guy Briggs]

We do not need to hold the audit_cmd_mutex for this family of cases.  The
possible exception to this is the call to audit_filter_user(), so drop the lock
immediately after.  To help in fixing the race we are trying to avoid, make
sure that nothing called by audit_filter_user() calls audit_log_start().  In
particular, watch out for *_audit_rule_match().

This fix will take care of systemd and anything USING audit.  It still means
that we could race with something configuring audit and auditd shutting down.

Signed-off-by: Richard Guy Briggs r...@tricolour.ca
Signed-off-by: Richard Guy Briggs r...@redhat.com
---
 kernel/audit.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 4689012..4cbc945 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -713,6 +713,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
if (err)
break;
}
+   mutex_unlock(audit_cmd_mutex);
audit_log_common_recv_msg(ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab,  msg='%.1024s',
@@ -729,6 +730,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
}
audit_set_pid(ab, NETLINK_CB(skb).portid);
audit_log_end(ab);
+   mutex_lock(audit_cmd_mutex);
}
break;
case AUDIT_ADD_RULE:
-- 
1.7.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 0/3] audit: remove audit_log_start() contention in AUDIT_USER type calls

[Richard Guy Briggs]

There is a race condition between systemd and auditd:

systemd|auditd
---+---
...|
- audit_receive   |...
   - mutex_lock(audit_cmd_mutex) |- audit_receive
  ... - audit_log_start   |   - mutex_lock(audit_cmd_mutex)
 - wait_for_auditd|  // wait for systemd
- schedule_timeout(60*HZ) |

This fix will take care of systemd and anything USING audit.  It still means
that we could race with something configuring audit and auditd shutting down.

The idea of dropping the lock at the top of audit_receive_msg() isn't as clean
as I had hoped, with AUDIT_ADD_RULE, AUDIT_TRIM, AUDIT_MAKE_EQUIV all
potentially allocating additional audit buffers indirectly through
trim_marked().  It may make sense to have trim_marked() send its queue through
a new thread.

Richard Guy Briggs (3):
  selinux: call WARN_ONCE() instead of calling audit_log_start()
  smack: call WARN_ONCE() instead of calling audit_log_start()
  audit: drop audit_cmd_lock in AUDIT_USER family of cases

 kernel/audit.c |2 ++
 security/selinux/ss/services.c |   12 
 security/smack/smack_lsm.c |5 ++---
 3 files changed, 8 insertions(+), 11 deletions(-)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit