[PATCH] audit: Add cmdline to taskinfo output

[PATCH] audit: Add cmdline to taskinfo output

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2641 bytes --]

>From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
From: William Roberts <wroberts@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.

Change-Id: I98a417c9ab3b95664c49aa1c7513cfd8296b6a2a
Signed-off-by: William Roberts <wroberts@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

[-- Attachment #1.2: Type: text/html, Size: 5120 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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 <wroberts@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 <wroberts@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 <rbriggs@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

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 3409 bytes --]

On Wed, Oct 23, 2013 at 1:40 PM, William Roberts
<bill.c.roberts@gmail.com>wrote:

> From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
> From: William Roberts <wroberts@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.
>
> Change-Id: I98a417c9ab3b95664c49aa1c7513cfd8296b6a2a
> Signed-off-by: William Roberts <wroberts@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
>
>

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.

[-- Attachment #1.2: Type: text/html, Size: 6156 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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.roberts@gmail.com>wrote:
> 
> > From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
> > From: William Roberts <wroberts@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 <rbriggs@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

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 1617 bytes --]

On Mon, Oct 28, 2013 at 8:10 AM, Richard Guy Briggs <rgb@redhat.com> wrote:

> 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.roberts@gmail.com>wrote:
> >
> > > From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
> > > From: William Roberts <wroberts@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.
>
>
Thanks!


>
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@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
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 2746 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 1888 bytes --]

On Mon, Oct 28, 2013 at 9:30 AM, William Roberts
<bill.c.roberts@gmail.com>wrote:

>
>
>
> On Mon, Oct 28, 2013 at 8:10 AM, Richard Guy Briggs <rgb@redhat.com>wrote:
>
>> 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.roberts@gmail.com>wrote:
>> >
>> > > From 0a8623b8f9fa625da81364cf3b87d2799171f83e Mon Sep 17 00:00:00 2001
>> > > From: William Roberts <wroberts@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.
>>
>>
> Thanks!
>
>

By any chance do you have a public tree I can configure you as a remote?


>
>> - RGB
>>
>> --
>> Richard Guy Briggs <rbriggs@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
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 3635 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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.roberts@gmail.com>wrote:
> > On Mon, Oct 28, 2013 at 8:10 AM, Richard Guy Briggs <rgb@redhat.com>wrote:
> >> 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.roberts@gmail.com>wrote:
> >> > 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 <rbriggs@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

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 3230 bytes --]

On Thu, Oct 31, 2013 at 8:51 AM, William Roberts
<bill.c.roberts@gmail.com>wrote:

>
>
>
> On Thu, Oct 31, 2013 at 8:46 AM, Richard Guy Briggs <rgb@redhat.com>wrote:
>
>> On Thu, Oct 31, 2013 at 08:33:34AM -0700, William Roberts wrote:
>> > On Thu, Oct 31, 2013 at 8:28 AM, Richard Guy Briggs <rgb@redhat.com>
>> wrote:
>> >
>> > > On Thu, Oct 31, 2013 at 08:24:11AM -0700, William Roberts wrote:
>> > > > On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb <sgrubb@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 <
>> sgrubb@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)
>> >
>> > How are you going to detect that cmdlne has changed, its a region of
>> > memory in userspace? We would have to cmp the values, and if we cannot
>> > detect the transition, this gets more expensive. Also, I have yet to
>> > see a case where the above statement is true, so it would be a very
>> > infrequent event.
>>
>> Is it likely that those two point to the same region of memory?  If so,
>> just compare the pointers.
>>
>
> no cmdline is mapped into the user process, cmdline is mapped into the
> kernel, so their
> virtual addresses and pa's are different (hopefully or I don't understand
> mmu based memory management)
>
>

Sorry incorrect above:
cmdline --> user process
comm --> Kernel process


>
>> > However, their is a condition in my patch where an error will cause
>> > comm=(null) not to be printed, which could be
>> > viewed as a disappearing field.
>>
>> Would it be useful if this condition were changed to print instead
>> comm=(error)?
>>
>
> It could be, but ideally we should printk the error too.... someone can
> arbitrarily set their
> proc cmdline entry or tsk comm to something "reserved"
>
>
>>
>> > > > William C Roberts
>> > >
>> > > - RGB
>> >
>> > William C Roberts
>>
>> - RGB
>>
>> --
>> Richard Guy Briggs <rbriggs@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
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 5486 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2924 bytes --]

On Thu, Oct 31, 2013 at 8:46 AM, Richard Guy Briggs <rgb@redhat.com> wrote:

> On Thu, Oct 31, 2013 at 08:33:34AM -0700, William Roberts wrote:
> > On Thu, Oct 31, 2013 at 8:28 AM, Richard Guy Briggs <rgb@redhat.com>
> wrote:
> >
> > > On Thu, Oct 31, 2013 at 08:24:11AM -0700, William Roberts wrote:
> > > > On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb <sgrubb@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 <sgrubb@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)
> >
> > How are you going to detect that cmdlne has changed, its a region of
> > memory in userspace? We would have to cmp the values, and if we cannot
> > detect the transition, this gets more expensive. Also, I have yet to
> > see a case where the above statement is true, so it would be a very
> > infrequent event.
>
> Is it likely that those two point to the same region of memory?  If so,
> just compare the pointers.
>

no cmdline is mapped into the user process, cmdline is mapped into the
kernel, so their
virtual addresses and pa's are different (hopefully or I don't understand
mmu based memory management)


>
> > However, their is a condition in my patch where an error will cause
> > comm=(null) not to be printed, which could be
> > viewed as a disappearing field.
>
> Would it be useful if this condition were changed to print instead
> comm=(error)?
>

It could be, but ideally we should printk the error too.... someone can
arbitrarily set their
proc cmdline entry or tsk comm to something "reserved"


>
> > > > William C Roberts
> > >
> > > - RGB
> >
> > William C Roberts
>
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@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
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Richard Guy Briggs]

On Thu, Oct 31, 2013 at 08:33:34AM -0700, William Roberts wrote:
> On Thu, Oct 31, 2013 at 8:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> > On Thu, Oct 31, 2013 at 08:24:11AM -0700, William Roberts wrote:
> > > On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb <sgrubb@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 <sgrubb@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)
> 
> How are you going to detect that cmdlne has changed, its a region of
> memory in userspace? We would have to cmp the values, and if we cannot
> detect the transition, this gets more expensive. Also, I have yet to
> see a case where the above statement is true, so it would be a very
> infrequent event.

Is it likely that those two point to the same region of memory?  If so,
just compare the pointers.

> However, their is a condition in my patch where an error will cause
> comm=(null) not to be printed, which could be
> viewed as a disappearing field.

Would it be useful if this condition were changed to print instead comm=(error)?

> > > William C Roberts
> >
> > - RGB
> 
> William C Roberts

- RGB

--
Richard Guy Briggs <rbriggs@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

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2059 bytes --]

On Thu, Oct 31, 2013 at 8:28 AM, Richard Guy Briggs <rgb@redhat.com> wrote:

> On Thu, Oct 31, 2013 at 08:24:11AM -0700, William Roberts wrote:
> > On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb <sgrubb@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 <sgrubb@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)
>

How are you going to detect that cmdlne has changed, its a region of memory
in userspace? We would have to
cmp the values, and if we cannot detect the transition, this gets more
expensive. Also, I have yet to see a case
where the above statement is true, so it would be a very infrequent event.

However, their is a condition in my patch where an error will cause
comm=(null) not to be printed, which could be
viewed as a disappearing field.


>
> > William C Roberts
>
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@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
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 3148 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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 <sgrubb@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 <sgrubb@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 <rbriggs@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

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 3569 bytes --]

On Thu, Oct 31, 2013 at 7:36 AM, Steve Grubb <sgrubb@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 <sgrubb@redhat.com> wrote:
> > > > > Again... the comm field got cut off and now I have no idea again.
> > >
> > > Which is the same as all arches. What I'm trying to say is that all
> arches
> > > would benefit from fixing this problem. I don't like the idea of it
> > > getting fixed
> > > for one platform and leaving it for all others to figure out another
> day.
> >
> > By arches your don't mean arm right?
>
> Any piece of hardware support the audit code. For example, x86_64/S390/PPC,
> etc.
>
>
> > This code runs the same on other architectures. If you mean platforms,
> like
> > Android, vs some other Linux distro, then yes I want a generic approach,
> > which I think cmdline gets us... no mater how many layers of VM
> exec/forking
> > indirection hell you may find yourself in, you at least get a chance at
> > what started the chain. On Android, that happens to be the packagename.
>
> What I'm suggesting is to fix "comm" to have more characters than 16.
> Which may
> mean getting it from somewhere else, or allowing a slightly bigger
> storage, or
> allowing an alternate storage in the audit context.
>

My point is, that comm and cmdline often times differ. JVMs oftren times
set the
comm field to some generated thread name via pthreads/prctl interface.
Since they
often times differ, I need them both. I even sent an example of this where
the comm
field was not what I wanted, and the cmdline field was.


>
>
> > > Is there some reason that the length of that field must be set to 16?
> I've
> > > seen
> > > user id numbers increased by a config option. It might be that the
> naming
> > > convention of android apps is enough to get a change.
> > >
> > > > I think exe= in the audit logs is essentially arg[0]... so thats not
> > >
> > > going
> > >
> > > > to work here,
> >
> > We can't change the naming convention of andorid apps, too large of an
> > ecosystem to change and no real easy way to be backwards compatible. That
> > one is off the table.
>
> That wasn't my suggestion. I was meaning that because of the andriod naming
> convention the current program name storage is useless and might need
> fixing.
>

I have patches that set the comm field to the package name submitted to
Android, but Dalvik
during execution will adjust the comm field to account for async task
naming etc. But even if
we fixed the VM to NOT do this... processes can still have a differing comm
title
and cmdline entry.


>
>
> > 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.

So the bottom line is, I need comm and cmdline, any other approach needs to
address
the fact that things can happen via fork and not only exec as well as
differing comm and
cmdline fields.

-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

On Wednesday, October 30, 2013 01:18:13 PM William Roberts wrote:
> On Wed, Oct 30, 2013 at 12:42 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > > Again... the comm field got cut off and now I have no idea again.
> > 
> > Which is the same as all arches. What I'm trying to say is that all arches
> > would benefit from fixing this problem. I don't like the idea of it
> > getting fixed
> > for one platform and leaving it for all others to figure out another day.
> 
> By arches your don't mean arm right?

Any piece of hardware support the audit code. For example, x86_64/S390/PPC, 
etc.


> This code runs the same on other architectures. If you mean platforms, like
> Android, vs some other Linux distro, then yes I want a generic approach,
> which I think cmdline gets us... no mater how many layers of VM exec/forking
> indirection hell you may find yourself in, you at least get a chance at
> what started the chain. On Android, that happens to be the packagename.

What I'm suggesting is to fix "comm" to have more characters than 16. Which may 
mean getting it from somewhere else, or allowing a slightly bigger storage, or 
allowing an alternate storage in the audit context.


> > Is there some reason that the length of that field must be set to 16? I've
> > seen
> > user id numbers increased by a config option. It might be that the naming
> > convention of android apps is enough to get a change.
> > 
> > > I think exe= in the audit logs is essentially arg[0]... so thats not
> > 
> > going
> > 
> > > to work here,
> 
> We can't change the naming convention of andorid apps, too large of an
> ecosystem to change and no real easy way to be backwards compatible. That
> one is off the table.

That wasn't my suggestion. I was meaning that because of the andriod naming 
convention the current program name storage is useless and might need fixing.


> 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.

-Steve

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 1146 bytes --]

On Wed, Oct 30, 2013 at 2:20 PM, Eric Paris <eparis@redhat.com> wrote:

> I'm like a child wandering into the middle of a movie and having no idea
> what is going on.  But...
>

my day to day reality :-P


>
> >         The limit is PATH_MAX. You could have an absolute path that
> >         uses all available
> >         characters.
> >
> >         -Steve
> >
> >
> > So looking at PATH_MAX...
> > include/linux/limits.h:12:#define PATH_MAX        4096 /* # chars in a
> > path name including nul */
>
> I seem to recall that PATH_MAX is not really PATH_MAX  :)  It can be +10
> ish...   for the " (deleted)" that gets appended if things are
> unlinked...  just something I sorta recall rolling around the back of my
> head and I haven't really any idea if it is applicable here in any
> way...
>
>
I don't think so. The concern is PAGE_SIZE is to big for a message, so lets
pick something else
to truncate the message size on, the suggesting was PATH_MAX which makes
sense
and works in this case.

The patch I have on top of what I submitted so far is to pick PAGE_SIZE or
PATH_MAX, whichever is
smaller.

-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 1840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Eric Paris]

I'm like a child wandering into the middle of a movie and having no idea
what is going on.  But...

>         The limit is PATH_MAX. You could have an absolute path that
>         uses all available
>         characters.
>         
>         -Steve
> 
> 
> So looking at PATH_MAX...
> include/linux/limits.h:12:#define PATH_MAX        4096 /* # chars in a
> path name including nul */

I seem to recall that PATH_MAX is not really PATH_MAX  :)  It can be +10
ish...   for the " (deleted)" that gets appended if things are
unlinked...  just something I sorta recall rolling around the back of my
head and I haven't really any idea if it is applicable here in any
way...

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 5332 bytes --]

On Wed, Oct 30, 2013 at 12:42 PM, Steve Grubb <sgrubb@redhat.com> wrote:

> On Tuesday, October 29, 2013 05:43:36 PM William Roberts wrote:
> > >> I guess I could just set the comm field explicitly via the packagename
> > >> when the classloader loads the value, but I was hoping for something
> more
> > >> generic that would
> > >> let me get larger package names then 16.
> > >
> > > I made the change of setting the comm field from within the VM, but its
> > > less then ideal... that 16char limitation is a pain. In Android Java
> Land,
> > > some of the packages that get run can be quite large. Also, current
> APIs
> > > in Javaland already change this...
> > >
> > > Also, a more generic solution would be desired.
> > >
> > > Lets look at what happens:
> > > type=SYSCALL msg=audit(10/29/2013 15:16:08.185:177) : arch=unknown elf
> > > type(40000028) syscall=fstat per=840000 success=yes exit=38 a0=7432ed34
> > > a1=20241 a2=180 a3=7432ed0c items=1 ppid=322 pid=1432 auid=unset
> > > uid=unknown(1027) gid=unknown(1027) euid=unknown(1027)
> suid=unknown(1027)
> > > fsuid=unknown(1027) egid=unknown(1027) sgid=unknown(1027)
> > > fsgid=unknown(1027) tty=(none) ses=4294967295 comm=AsyncTask #1
> > > exe=/system/bin/app_process cmdline="com.android.nfc" subj=u:r:nfc:s0
> > > key=(null)
> > >
> > > Here the nfc task has an async task, that async task api sets the cmd
> > > field when it attaches a thread to the VM....
> > >
> > > type=1300 msg=audit(1383088554.170:322): arch=40000028 syscall=54
> > > per=840000 success=yes exit=0 a0=a a1=c0186201 a2=be985430 a3=be98542c
> > > items=0 ppid=321 pid=1181 auid=4294967295 uid=10036 gid=10036
> euid=10036
> > > suid=10036 fsuid=10036 egid=10036 sgid=10036 fsgid=10036 tty=(none)
> > > ses=4294967295 comm="putmethod.latin" exe="/system/bin/app_process"
> > > cmdline="com.android.inputmethod.latin" subj=u:r:shared_app:s0
> key=(null)
> > >
> > > Again... the comm field got cut off and now I have no idea again.
>
> Which is the same as all arches. What I'm trying to say is that all arches
> would benefit from fixing this problem. I don't like the idea of it
> getting fixed
> for one platform and leaving it for all others to figure out another day.
>

By arches your don't mean arm right? This code runs the same on other
architectures. If
you mean platforms, like Android, vs some other Linux distro, then yes I
want a generic
approach, which I think cmdline gets us... no mater how many layers of VM
exec/forking
indirection hell you may find yourself in, you at least get a chance at
what started the chain.
On Android, that happens to be the packagename.


>
> Is there some reason that the length of that field must be set to 16? I've
> seen
> user id numbers increased by a config option. It might be that the naming
> convention of android apps is enough to get a change.
>
> > I think exe= in the audit logs is essentially arg[0]... so thats not
> going
> > to work here,
>

We can't change the naming convention of andorid apps, too large of an
ecosystem to
change and no real easy way to be backwards compatible. That one is off the
table.

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.


> The original problem was shell scripts. We got /bin/bash and had no idea
> what
> was being executed. So, cmd was offered as a quick fix.
>
> > and I don't think I can change that value from userspace as its not
> > > logged with untrusted string, which is a good indication its mutable
> from
> > > userspace.
> > >
> > > Why dont I just limit the size of what is displayed on cmdline to
> > > something like 128 or 256?
> > >
> > > Eventually some limit has to be set, whether its PAGE_SIZE or
> not..their
> > > will always be an argument of "too much memory". But its also
> important to
> > > note its off by default, you have to turn it on, so most desktop
> instances
> > > will leave it off, whilst I will dynamically enable it as needed.
> > >
> > > Thanks again for your review and help, I appreciate it.
> >
> > Looking further into your size concerns, EXECVE is truncated at 7500
> >
> > kernel/auditsc.c:
> > #define MAX_EXECVE_AUDIT_LEN 7500
>
> I think that is for the purposes of splitting records. At the time,
> compiling
> a kernel or maybe linking it allowed passing thousands of file names on the
> command line and Eric wrote a patch to split the execve record into
> multiple
> ones. Userspace had to be adapted to glue them back together.
>

Ahh ok, thanks.


>
> > the proc cmdline info is truncated at PAGE_SIZE, which most of the time
> in
> > 4096.. so its even smaller then that.
>
> And then if there is a space in the path, it gets encoded so double that
> size.
>

Ahh yeah.. forgot about those.


>
> > So based on our discussion, whats the next step at moving forward on
> this?
> >
> > Do you want a separate limit other then PAGE_SIZE on this?
>
> The limit is PATH_MAX. You could have an absolute path that uses all
> available
> characters.
>
> -Steve
>

So looking at PATH_MAX...
include/linux/limits.h:12:#define PATH_MAX        4096 /* # chars in a path
name including nul */

I can set it to that...NP

- Bill

[-- Attachment #1.2: Type: text/html, Size: 7657 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

On Tuesday, October 29, 2013 05:43:36 PM William Roberts wrote:
> >> I guess I could just set the comm field explicitly via the packagename
> >> when the classloader loads the value, but I was hoping for something more
> >> generic that would
> >> let me get larger package names then 16.
> > 
> > I made the change of setting the comm field from within the VM, but its
> > less then ideal... that 16char limitation is a pain. In Android Java Land,
> > some of the packages that get run can be quite large. Also, current APIs
> > in Javaland already change this...
> > 
> > Also, a more generic solution would be desired.
> > 
> > Lets look at what happens:
> > type=SYSCALL msg=audit(10/29/2013 15:16:08.185:177) : arch=unknown elf
> > type(40000028) syscall=fstat per=840000 success=yes exit=38 a0=7432ed34
> > a1=20241 a2=180 a3=7432ed0c items=1 ppid=322 pid=1432 auid=unset
> > uid=unknown(1027) gid=unknown(1027) euid=unknown(1027) suid=unknown(1027)
> > fsuid=unknown(1027) egid=unknown(1027) sgid=unknown(1027)
> > fsgid=unknown(1027) tty=(none) ses=4294967295 comm=AsyncTask #1
> > exe=/system/bin/app_process cmdline="com.android.nfc" subj=u:r:nfc:s0
> > key=(null)
> > 
> > Here the nfc task has an async task, that async task api sets the cmd
> > field when it attaches a thread to the VM....
> > 
> > type=1300 msg=audit(1383088554.170:322): arch=40000028 syscall=54
> > per=840000 success=yes exit=0 a0=a a1=c0186201 a2=be985430 a3=be98542c
> > items=0 ppid=321 pid=1181 auid=4294967295 uid=10036 gid=10036 euid=10036
> > suid=10036 fsuid=10036 egid=10036 sgid=10036 fsgid=10036 tty=(none)
> > ses=4294967295 comm="putmethod.latin" exe="/system/bin/app_process"
> > cmdline="com.android.inputmethod.latin" subj=u:r:shared_app:s0 key=(null)
> > 
> > Again... the comm field got cut off and now I have no idea again.

Which is the same as all arches. What I'm trying to say is that all arches 
would benefit from fixing this problem. I don't like the idea of it getting fixed 
for one platform and leaving it for all others to figure out another day.

Is there some reason that the length of that field must be set to 16? I've seen 
user id numbers increased by a config option. It might be that the naming 
convention of android apps is enough to get a change.

> I think exe= in the audit logs is essentially arg[0]... so thats not going
> to work here, 

The original problem was shell scripts. We got /bin/bash and had no idea what 
was being executed. So, cmd was offered as a quick fix.

> and I don't think I can change that value from userspace as its not
> > logged with untrusted string, which is a good indication its mutable from
> > userspace.
> > 
> > Why dont I just limit the size of what is displayed on cmdline to
> > something like 128 or 256?
> > 
> > Eventually some limit has to be set, whether its PAGE_SIZE or not..their
> > will always be an argument of "too much memory". But its also important to
> > note its off by default, you have to turn it on, so most desktop instances
> > will leave it off, whilst I will dynamically enable it as needed.
> > 
> > Thanks again for your review and help, I appreciate it.
> 
> Looking further into your size concerns, EXECVE is truncated at 7500
> 
> kernel/auditsc.c:
> #define MAX_EXECVE_AUDIT_LEN 7500

I think that is for the purposes of splitting records. At the time, compiling 
a kernel or maybe linking it allowed passing thousands of file names on the 
command line and Eric wrote a patch to split the execve record into multiple 
ones. Userspace had to be adapted to glue them back together.
 
> the proc cmdline info is truncated at PAGE_SIZE, which most of the time in
> 4096.. so its even smaller then that.

And then if there is a space in the path, it gets encoded so double that size.

> So based on our discussion, whats the next step at moving forward on this?
> 
> Do you want a separate limit other then PAGE_SIZE on this?

The limit is PATH_MAX. You could have an absolute path that uses all available 
characters.

-Steve

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 5713 bytes --]

On Tue, Oct 29, 2013 at 4:24 PM, William Roberts
<bill.c.roberts@gmail.com>wrote:

>
>
>
> On Tue, Oct 29, 2013 at 1:25 PM, William Roberts <bill.c.roberts@gmail.com
> > wrote:
>
>>
>>
>>
>> On Tue, Oct 29, 2013 at 12:55 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>>
>>> On Tuesday, October 29, 2013 12:12:29 PM William Roberts wrote:
>>> > > > to small for most package names, and
>>> > > > already contains the VM command. I really have no information of
>>> what
>>> > > > Android App has created the issue.
>>> > >
>>> > > This is true for all arches. Usually you can have it pretty narrowly
>>> > > defined to
>>> > > where you have a pretty good guess between 2 or 3 apps with the same
>>> root
>>> > > name. But in your case its totally named wrong.
>>> >
>>> > I could set the title via prctl and PR_SET_NAME, but again I would be
>>> > limited at 16 bytes, at least with cmdline I am limited at a page.
>>>
>>> A page would be a problem for audit records. What I see is a NULL
>>> terminated
>>> list of arguments which the program name is argv[0]. So, you'd want to
>>> grab
>>> that one. Butyou could have something in there with PATH_MAX and
>>> whitespaces
>>> which would be excessively long.
>>>
>>> > As a simple example, a basic example from samsung gets truncated.
>>> >
>>> > com.samsung.myapp
>>> >
>>> > > > Solution:
>>> > > > Get the proc cmdline info (not trust worthy, but can help debugging
>>> > >
>>> > > Android)
>>> > >
>>> > > > type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
>>> > >
>>> > > per=840000
>>> > >
>>> > > > success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
>>> > > > ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027
>>> suid=1027
>>> > > > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
>>> > > > comm=4173796E635461736B202331 exe="/system/bin/app_process"
>>> > > > cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
>>> > > >
>>> > > > Now I know it was the NFC app
>>> > >
>>> > > What do you get on x86_64 auditing a shell or python script with
>>> your same
>>> > > patch? Also, does cmdline potentially include arguments?
>>> >
>>> > I would have to get back to you on this, but whatever is set in
>>> > /proc/<pid>/cmdline shows up here, which means
>>> > it could have arguments etc.
>>>
>>> The reason I'm asking is that it might be better for all arches to
>>> switch. All
>>> have the 16 character limit. But we would only want argv[0] and not the
>>> arguments.
>>>
>>> -Steve
>>>
>>
>> I guess i'm thinking about how can I access the smallest set of data that
>> I need to get the information I want.... however, wouldn't argv[0]
>> typically be the vm name...
>> <vm> <program>
>> And on Android, to make it even more of a pain.... A VM is already
>> running, that then forks itself and then invokes the classloader, so their
>> is no
>> explicit exec.
>>
>> I guess I could just set the comm field explicitly via the packagename
>> when the classloader loads the value, but I was hoping for something more
>> generic that would
>> let me get larger package names then 16.
>>
>>
> I made the change of setting the comm field from within the VM, but its
> less then ideal... that 16char limitation is a pain. In Android Java Land,
> some of the packages that get run can be quite large. Also, current APIs in
> Javaland
> already change this...
>
> Also, a more generic solution would be desired.
>
> Lets look at what happens:
> type=SYSCALL msg=audit(10/29/2013 15:16:08.185:177) : arch=unknown elf
> type(40000028) syscall=fstat per=840000 success=yes exit=38 a0=7432ed34
> a1=20241 a2=180 a3=7432ed0c items=1 ppid=322 pid=1432 auid=unset
> uid=unknown(1027) gid=unknown(1027) euid=unknown(1027) suid=unknown(1027)
> fsuid=unknown(1027) egid=unknown(1027) sgid=unknown(1027)
> fsgid=unknown(1027) tty=(none) ses=4294967295 comm=AsyncTask #1
> exe=/system/bin/app_process cmdline="com.android.nfc" subj=u:r:nfc:s0
> key=(null)
>
> Here the nfc task has an async task, that async task api sets the cmd
> field when it attaches a thread to the VM....
>
> type=1300 msg=audit(1383088554.170:322): arch=40000028 syscall=54
> per=840000 success=yes exit=0 a0=a a1=c0186201 a2=be985430 a3=be98542c
> items=0 ppid=321 pid=1181 auid=4294967295 uid=10036 gid=10036 euid=10036
> suid=10036 fsuid=10036 egid=10036 sgid=10036 fsgid=10036 tty=(none)
> ses=4294967295 comm="putmethod.latin" exe="/system/bin/app_process"
> cmdline="com.android.inputmethod.latin" subj=u:r:shared_app:s0 key=(null)
>
> Again... the comm field got cut off and now I have no idea again. I think
> exe= in the audit logs is essentially arg[0]... so thats not going to work
> here, and I don't think I can change that value from userspace as its not
> logged with untrusted string, which is a good indication its mutable from
> userspace.
>
> Why dont I just limit the size of what is displayed on cmdline to
> something like 128 or 256?
>
> Eventually some limit has to be set, whether its PAGE_SIZE or not..their
> will always be an argument of "too much memory". But its also important to
> note its off by default, you have to turn it on, so most desktop instances
> will leave it off, whilst I will dynamically enable it as needed.
>
> Thanks again for your review and help, I appreciate it.
>
>
>
>

Looking further into your size concerns, EXECVE is truncated at 7500

kernel/auditsc.c:
#define MAX_EXECVE_AUDIT_LEN 7500

the proc cmdline info is truncated at PAGE_SIZE, which most of the time in
4096.. so its even smaller then that.


So based on our discussion, whats the next step at moving forward on this?

Do you want a separate limit other then PAGE_SIZE on this?

-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 8351 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 5038 bytes --]

On Tue, Oct 29, 2013 at 1:25 PM, William Roberts
<bill.c.roberts@gmail.com>wrote:

>
>
>
> On Tue, Oct 29, 2013 at 12:55 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>
>> On Tuesday, October 29, 2013 12:12:29 PM William Roberts wrote:
>> > > > to small for most package names, and
>> > > > already contains the VM command. I really have no information of
>> what
>> > > > Android App has created the issue.
>> > >
>> > > This is true for all arches. Usually you can have it pretty narrowly
>> > > defined to
>> > > where you have a pretty good guess between 2 or 3 apps with the same
>> root
>> > > name. But in your case its totally named wrong.
>> >
>> > I could set the title via prctl and PR_SET_NAME, but again I would be
>> > limited at 16 bytes, at least with cmdline I am limited at a page.
>>
>> A page would be a problem for audit records. What I see is a NULL
>> terminated
>> list of arguments which the program name is argv[0]. So, you'd want to
>> grab
>> that one. Butyou could have something in there with PATH_MAX and
>> whitespaces
>> which would be excessively long.
>>
>> > As a simple example, a basic example from samsung gets truncated.
>> >
>> > com.samsung.myapp
>> >
>> > > > Solution:
>> > > > Get the proc cmdline info (not trust worthy, but can help debugging
>> > >
>> > > Android)
>> > >
>> > > > type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
>> > >
>> > > per=840000
>> > >
>> > > > success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
>> > > > ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027
>> suid=1027
>> > > > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
>> > > > comm=4173796E635461736B202331 exe="/system/bin/app_process"
>> > > > cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
>> > > >
>> > > > Now I know it was the NFC app
>> > >
>> > > What do you get on x86_64 auditing a shell or python script with your
>> same
>> > > patch? Also, does cmdline potentially include arguments?
>> >
>> > I would have to get back to you on this, but whatever is set in
>> > /proc/<pid>/cmdline shows up here, which means
>> > it could have arguments etc.
>>
>> The reason I'm asking is that it might be better for all arches to
>> switch. All
>> have the 16 character limit. But we would only want argv[0] and not the
>> arguments.
>>
>> -Steve
>>
>
> I guess i'm thinking about how can I access the smallest set of data that
> I need to get the information I want.... however, wouldn't argv[0]
> typically be the vm name...
> <vm> <program>
> And on Android, to make it even more of a pain.... A VM is already
> running, that then forks itself and then invokes the classloader, so their
> is no
> explicit exec.
>
> I guess I could just set the comm field explicitly via the packagename
> when the classloader loads the value, but I was hoping for something more
> generic that would
> let me get larger package names then 16.
>
>
I made the change of setting the comm field from within the VM, but its
less then ideal... that 16char limitation is a pain. In Android Java Land,
some of the packages that get run can be quite large. Also, current APIs in
Javaland
already change this...

Also, a more generic solution would be desired.

Lets look at what happens:
type=SYSCALL msg=audit(10/29/2013 15:16:08.185:177) : arch=unknown elf
type(40000028) syscall=fstat per=840000 success=yes exit=38 a0=7432ed34
a1=20241 a2=180 a3=7432ed0c items=1 ppid=322 pid=1432 auid=unset
uid=unknown(1027) gid=unknown(1027) euid=unknown(1027) suid=unknown(1027)
fsuid=unknown(1027) egid=unknown(1027) sgid=unknown(1027)
fsgid=unknown(1027) tty=(none) ses=4294967295 comm=AsyncTask #1
exe=/system/bin/app_process cmdline="com.android.nfc" subj=u:r:nfc:s0
key=(null)

Here the nfc task has an async task, that async task api sets the cmd field
when it attaches a thread to the VM....

type=1300 msg=audit(1383088554.170:322): arch=40000028 syscall=54
per=840000 success=yes exit=0 a0=a a1=c0186201 a2=be985430 a3=be98542c
items=0 ppid=321 pid=1181 auid=4294967295 uid=10036 gid=10036 euid=10036
suid=10036 fsuid=10036 egid=10036 sgid=10036 fsgid=10036 tty=(none)
ses=4294967295 comm="putmethod.latin" exe="/system/bin/app_process"
cmdline="com.android.inputmethod.latin" subj=u:r:shared_app:s0 key=(null)

Again... the comm field got cut off and now I have no idea again. I think
exe= in the audit logs is essentially arg[0]... so thats not going to work
here, and I don't think I can change that value from userspace as its not
logged with untrusted string, which is a good indication its mutable from
userspace.

Why dont I just limit the size of what is displayed on cmdline to something
like 128 or 256?

Eventually some limit has to be set, whether its PAGE_SIZE or not..their
will always be an argument of "too much memory". But its also important to
note its off by default, you have to turn it on, so most desktop instances
will leave it off, whilst I will dynamically enable it as needed.

Thanks again for your review and help, I appreciate it.

[-- Attachment #1.2: Type: text/html, Size: 7242 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2747 bytes --]

On Tue, Oct 29, 2013 at 12:55 PM, Steve Grubb <sgrubb@redhat.com> wrote:

> On Tuesday, October 29, 2013 12:12:29 PM William Roberts wrote:
> > > > to small for most package names, and
> > > > already contains the VM command. I really have no information of what
> > > > Android App has created the issue.
> > >
> > > This is true for all arches. Usually you can have it pretty narrowly
> > > defined to
> > > where you have a pretty good guess between 2 or 3 apps with the same
> root
> > > name. But in your case its totally named wrong.
> >
> > I could set the title via prctl and PR_SET_NAME, but again I would be
> > limited at 16 bytes, at least with cmdline I am limited at a page.
>
> A page would be a problem for audit records. What I see is a NULL
> terminated
> list of arguments which the program name is argv[0]. So, you'd want to grab
> that one. Butyou could have something in there with PATH_MAX and
> whitespaces
> which would be excessively long.
>
> > As a simple example, a basic example from samsung gets truncated.
> >
> > com.samsung.myapp
> >
> > > > Solution:
> > > > Get the proc cmdline info (not trust worthy, but can help debugging
> > >
> > > Android)
> > >
> > > > type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
> > >
> > > per=840000
> > >
> > > > success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
> > > > ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027
> suid=1027
> > > > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> > > > comm=4173796E635461736B202331 exe="/system/bin/app_process"
> > > > cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
> > > >
> > > > Now I know it was the NFC app
> > >
> > > What do you get on x86_64 auditing a shell or python script with your
> same
> > > patch? Also, does cmdline potentially include arguments?
> >
> > I would have to get back to you on this, but whatever is set in
> > /proc/<pid>/cmdline shows up here, which means
> > it could have arguments etc.
>
> The reason I'm asking is that it might be better for all arches to switch.
> All
> have the 16 character limit. But we would only want argv[0] and not the
> arguments.
>
> -Steve
>

I guess i'm thinking about how can I access the smallest set of data that I
need to get the information I want.... however, wouldn't argv[0] typically
be the vm name...
<vm> <program>
And on Android, to make it even more of a pain.... A VM is already running,
that then forks itself and then invokes the classloader, so their is no
explicit exec.

I guess I could just set the comm field explicitly via the packagename when
the classloader loads the value, but I was hoping for something more
generic that would
let me get larger package names then 16.

[-- Attachment #1.2: Type: text/html, Size: 3873 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

On Tuesday, October 29, 2013 12:12:29 PM William Roberts wrote:
> > > to small for most package names, and
> > > already contains the VM command. I really have no information of what
> > > Android App has created the issue.
> > 
> > This is true for all arches. Usually you can have it pretty narrowly
> > defined to
> > where you have a pretty good guess between 2 or 3 apps with the same root
> > name. But in your case its totally named wrong.
> 
> I could set the title via prctl and PR_SET_NAME, but again I would be
> limited at 16 bytes, at least with cmdline I am limited at a page. 

A page would be a problem for audit records. What I see is a NULL terminated 
list of arguments which the program name is argv[0]. So, you'd want to grab 
that one. Butyou could have something in there with PATH_MAX and whitespaces 
which would be excessively long.

> As a simple example, a basic example from samsung gets truncated.
> 
> com.samsung.myapp
> 
> > > Solution:
> > > Get the proc cmdline info (not trust worthy, but can help debugging
> > 
> > Android)
> > 
> > > type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
> > 
> > per=840000
> > 
> > > success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
> > > ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> > > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> > > comm=4173796E635461736B202331 exe="/system/bin/app_process"
> > > cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
> > > 
> > > Now I know it was the NFC app
> > 
> > What do you get on x86_64 auditing a shell or python script with your same
> > patch? Also, does cmdline potentially include arguments?
> 
> I would have to get back to you on this, but whatever is set in
> /proc/<pid>/cmdline shows up here, which means
> it could have arguments etc.

The reason I'm asking is that it might be better for all arches to switch. All 
have the 16 character limit. But we would only want argv[0] and not the 
arguments.

-Steve

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2782 bytes --]

On Tue, Oct 29, 2013 at 12:01 PM, Steve Grubb <sgrubb@redhat.com> wrote:

> Hello,
>
> On Tuesday, October 29, 2013 10:44:48 AM William Roberts wrote:
> > On Tue, Oct 29, 2013 at 8:14 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Monday, October 28, 2013 04:50:38 PM William Roberts wrote:
> > I'm 100% ok with the dynamic option changing it from NULL to a real value
> > IMO a like that better then what I currently have.
> >
> > Old:
> > type=1300 msg=audit(1383022671.232:230): arch=40000028
>
> This arch is not defined:
> arch=unknown elf type(40000028)
>
> Which one is it?
>

FYI this is on Android with my patch backported to a 3.4 Kernel, so pretty
much all of my testing is
around this setup. Also were running a custom stripped down auditd over
here, so it doesn't fix anything up.

The architecture is ARM


>
> > syscall=54
> > per=840000 success=yes exit=0 a0=23 a1=fa05 a2=0 a3=74e1ee34 items=0
> > ppid=298 pid=1431 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> > comm=4173796E635461736B202331
>
> comm=AsyncTask #1
>
> > exe="/system/bin/app_process" subj=u:r:nfc:s0
> > key=(null)
> >
> > Issue:
> > comm field in task is only 16  chars,
>
> Yes, its a limitation on ALL arches.
>
> > to small for most package names, and
> > already contains the VM command. I really have no information of what
> > Android App has created the issue.
>
> This is true for all arches. Usually you can have it pretty narrowly
> defined to
> where you have a pretty good guess between 2 or 3 apps with the same root
> name. But in your case its totally named wrong.
>

I could set the title via prctl and PR_SET_NAME, but again I would be
limited
at 16 bytes, at least with cmdline I am limited at a page. As a simple
example,
a basic example from samsung gets truncated.

com.samsung.myapp


>
>
> > Solution:
> > Get the proc cmdline info (not trust worthy, but can help debugging
> Android)
> >
> > type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
> per=840000
> > success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
> > ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> > fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> > comm=4173796E635461736B202331 exe="/system/bin/app_process"
> > cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
> >
> > Now I know it was the NFC app
>
> What do you get on x86_64 auditing a shell or python script with your same
> patch? Also, does cmdline potentially include arguments?
>

I would have to get back to you on this, but whatever is set in
/proc/<pid>/cmdline shows up here, which means
it could have arguments etc.


>
> -Steve
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4463 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

Hello,

On Tuesday, October 29, 2013 10:44:48 AM William Roberts wrote:
> On Tue, Oct 29, 2013 at 8:14 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Monday, October 28, 2013 04:50:38 PM William Roberts wrote:
> I'm 100% ok with the dynamic option changing it from NULL to a real value
> IMO a like that better then what I currently have.
> 
> Old:
> type=1300 msg=audit(1383022671.232:230): arch=40000028 

This arch is not defined:
arch=unknown elf type(40000028)

Which one is it?

> syscall=54
> per=840000 success=yes exit=0 a0=23 a1=fa05 a2=0 a3=74e1ee34 items=0
> ppid=298 pid=1431 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> comm=4173796E635461736B202331

comm=AsyncTask #1 

> exe="/system/bin/app_process" subj=u:r:nfc:s0
> key=(null)
> 
> Issue:
> comm field in task is only 16  chars,

Yes, its a limitation on ALL arches.

> to small for most package names, and
> already contains the VM command. I really have no information of what
> Android App has created the issue.

This is true for all arches. Usually you can have it pretty narrowly defined to 
where you have a pretty good guess between 2 or 3 apps with the same root 
name. But in your case its totally named wrong.


> Solution:
> Get the proc cmdline info (not trust worthy, but can help debugging Android)
> 
> type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5 per=840000
> success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
> ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> comm=4173796E635461736B202331 exe="/system/bin/app_process"
> cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
> 
> Now I know it was the NFC app

What do you get on x86_64 auditing a shell or python script with your same 
patch? Also, does cmdline potentially include arguments?

-Steve

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2683 bytes --]

On Tue, Oct 29, 2013 at 10:44 AM, William Roberts
<bill.c.roberts@gmail.com>wrote:

>
>
>
> On Tue, Oct 29, 2013 at 8:14 AM, Steve Grubb <sgrubb@redhat.com> wrote:
>
>> On Monday, October 28, 2013 04:50:38 PM William Roberts wrote:
>> > 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.
>> >
>> > The additional cmdline output only runs if the audit feature
>> > AUDIT_FEATURE_CMDLINE_OUTPUT is set high at runtime.
>>
>> I don't exactly like this. The audit event records are very normalized.
>> When
>> you have a specific kind of record, you can count on always having the
>> certain
>> fields even if its value is (NULL). So, having fields swinging in and out
>> by
>> configuration is not something I'd like to see start.
>>
>> Can you show me an event that has the problem and what it looks like when
>> its
>> fixed by this patch?
>>
>> Thanks,
>> -Steve
>>
>
>
> I'm 100% ok with the dynamic option changing it from NULL to a real value
> IMO a like
> that better then what I currently have.
>
> Old:
> type=1300 msg=audit(1383022671.232:230): arch=40000028 syscall=54
> per=840000 success=yes exit=0 a0=23 a1=fa05 a2=0 a3=74e1ee34 items=0
> ppid=298 pid=1431 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
> fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
> comm=4173796E635461736B202331 exe="/system/bin/app_process" subj=u:r:nfc:s0
> key=(null)
>
> Issue:
> comm field in task is only 16  chars, to small for most package names, and
> already contains the VM command. I really have no information of what
> Android App has created the issue.
>
> Solution:
> Get the proc cmdline info (not trust worthy, but can help debugging
> Android)
>
> type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5
> per=840000 success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c
> items=1 ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027
> suid=1027 fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none)
> ses=4294967295 comm=4173796E635461736B202331 exe="/system/bin/app_process"
> cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)
>
> Now I know it was the NFC app
>
>
FYI, from this point I would get the application from a trustworthy source
and audit it, obviously this could be an app using setproctitle() and
spoofing something legitimate. Like malicious app A pretending to be angry
birds. I cant read this
cmdline value form userspace, as a denial from selinux may cause the app to
crash. So sometimes the apps proc entry is gone before I can record what
happened in userspace.

[-- Attachment #1.2: Type: text/html, Size: 3737 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2092 bytes --]

On Tue, Oct 29, 2013 at 8:14 AM, Steve Grubb <sgrubb@redhat.com> wrote:

> On Monday, October 28, 2013 04:50:38 PM William Roberts wrote:
> > 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.
> >
> > The additional cmdline output only runs if the audit feature
> > AUDIT_FEATURE_CMDLINE_OUTPUT is set high at runtime.
>
> I don't exactly like this. The audit event records are very normalized.
> When
> you have a specific kind of record, you can count on always having the
> certain
> fields even if its value is (NULL). So, having fields swinging in and out
> by
> configuration is not something I'd like to see start.
>
> Can you show me an event that has the problem and what it looks like when
> its
> fixed by this patch?
>
> Thanks,
> -Steve
>


I'm 100% ok with the dynamic option changing it from NULL to a real value
IMO a like
that better then what I currently have.

Old:
type=1300 msg=audit(1383022671.232:230): arch=40000028 syscall=54
per=840000 success=yes exit=0 a0=23 a1=fa05 a2=0 a3=74e1ee34 items=0
ppid=298 pid=1431 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
comm=4173796E635461736B202331 exe="/system/bin/app_process" subj=u:r:nfc:s0
key=(null)

Issue:
comm field in task is only 16  chars, to small for most package names, and
already contains the VM command. I really have no information of what
Android App has created the issue.

Solution:
Get the proc cmdline info (not trust worthy, but can help debugging Android)

type=1300 msg=audit(1383068585.326:205): arch=40000028 syscall=5 per=840000
success=yes exit=38 a0=74d86d34 a1=20241 a2=180 a3=74d86d0c items=1
ppid=296 pid=1378 auid=4294967295 uid=1027 gid=1027 euid=1027 suid=1027
fsuid=1027 egid=1027 sgid=1027 fsgid=1027 tty=(none) ses=4294967295
comm=4173796E635461736B202331 exe="/system/bin/app_process"
cmdline="com.android.nfc" subj=u:r:nfc:s0 key=(null)

Now I know it was the NFC app

[-- Attachment #1.2: Type: text/html, Size: 2879 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

On Monday, October 28, 2013 04:50:38 PM William Roberts wrote:
> 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.
> 
> The additional cmdline output only runs if the audit feature
> AUDIT_FEATURE_CMDLINE_OUTPUT is set high at runtime.

I don't exactly like this. The audit event records are very normalized. When 
you have a specific kind of record, you can count on always having the certain 
fields even if its value is (NULL). So, having fields swinging in and out by 
configuration is not something I'd like to see start.

Can you show me an event that has the problem and what it looks like when its 
fixed by this patch?

Thanks,
-Steve

[PATCH] audit: Add cmdline to taskinfo output

[William Roberts]

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.

The additional cmdline output only runs if the audit feature
AUDIT_FEATURE_CMDLINE_OUTPUT is set high at runtime.

Signed-off-by: William Roberts <wroberts@tresys.com>
---

Sorry for the noise, the commit message got truncated last time.
This will apply to Richard's tree on branch audit-for-next.
This requires eparis's generic get/set patches.

 fs/proc/base.c             |    2 +-
 include/linux/proc_fs.h    |    1 +
 include/uapi/linux/audit.h |    4 +++-
 kernel/audit.c             |   38 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 03c8d74..cb1ba2f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -200,7 +200,7 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-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 608e60a..2f386b3 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -41,6 +41,7 @@ extern void *proc_get_parent_data(const struct inode *);
 extern void proc_remove(struct proc_dir_entry *);
 extern void remove_proc_entry(const char *, struct proc_dir_entry *);
 extern int remove_proc_subtree(const char *, struct proc_dir_entry *);
+extern int proc_pid_cmdline(struct task_struct *, char *);
 
 #else /* CONFIG_PROC_FS */
 
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index e2f0d99..5d77124 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -388,7 +388,9 @@ struct audit_features {
 
 #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
 #define AUDIT_FEATURE_LOGINUID_IMMUTABLE	1
-#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOGINUID_IMMUTABLE
+#define AUDIT_FEATURE_CMDLINE_OUTPUT		2
+#define AUDIT_LAST_FEATURE	AUDIT_FEATURE_CMDLINE_OUTPUT
+
 
 #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
 #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
diff --git a/kernel/audit.c b/kernel/audit.c
index 8378c5e..bf4b1af 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -63,6 +63,7 @@
 #include <linux/freezer.h>
 #include <linux/tty.h>
 #include <linux/pid_namespace.h>
+#include <linux/proc_fs.h>
 
 #include "audit.h"
 
@@ -144,9 +145,10 @@ static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
 				   .features = 0,
 				   .lock = 0,};
 
-static char *audit_feature_names[2] = {
+static char *audit_feature_names[3] = {
 	"only_unset_loginuid",
 	"loginuid_immutable",
+	"audit_output_cmdline",
 };
 
 
@@ -1691,6 +1693,39 @@ 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;
+
+	/* Ensure that the feature is set */
+	if (!is_audit_feature_set(AUDIT_FEATURE_CMDLINE_OUTPUT))
+		return;
+
+	/* Get the process cmdline */
+	page = __get_free_page(GFP_TEMPORARY);
+	if (!page)
+		return;
+
+	len = proc_pid_cmdline(tsk, (char *)page);
+	if (len <= 0) {
+		free_page(page);
+		return;
+	}
+
+	/*
+	* 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_page(page);
+}
+
 void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 {
 	const struct cred *cred;
@@ -1739,6 +1774,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 		up_read(&mm->mmap_sem);
 	}
 	audit_log_task_context(ab);
+	audit_log_add_cmdline(ab, tsk);
 }
 EXPORT_SYMBOL(audit_log_task_info);
 
-- 
1.7.9.5

[PATCH] audit: Add cmdline to taskinfo output

[William Roberts]

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.

The additional cmdline output only runs if the audit feature
Signed-off-by: William Roberts <wroberts@tresys.com>
---

This will apply to Richard's tree on branch audit-for-next. This
requires eparis's generic get/set patches.

 fs/proc/base.c             |    2 +-
 include/linux/proc_fs.h    |    1 +
 include/uapi/linux/audit.h |    4 +++-
 kernel/audit.c             |   38 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 03c8d74..cb1ba2f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -200,7 +200,7 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-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 608e60a..2f386b3 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -41,6 +41,7 @@ extern void *proc_get_parent_data(const struct inode *);
 extern void proc_remove(struct proc_dir_entry *);
 extern void remove_proc_entry(const char *, struct proc_dir_entry *);
 extern int remove_proc_subtree(const char *, struct proc_dir_entry *);
+extern int proc_pid_cmdline(struct task_struct *, char *);
 
 #else /* CONFIG_PROC_FS */
 
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index e2f0d99..5d77124 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -388,7 +388,9 @@ struct audit_features {
 
 #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
 #define AUDIT_FEATURE_LOGINUID_IMMUTABLE	1
-#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOGINUID_IMMUTABLE
+#define AUDIT_FEATURE_CMDLINE_OUTPUT		2
+#define AUDIT_LAST_FEATURE	AUDIT_FEATURE_CMDLINE_OUTPUT
+
 
 #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
 #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
diff --git a/kernel/audit.c b/kernel/audit.c
index 8378c5e..bf4b1af 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -63,6 +63,7 @@
 #include <linux/freezer.h>
 #include <linux/tty.h>
 #include <linux/pid_namespace.h>
+#include <linux/proc_fs.h>
 
 #include "audit.h"
 
@@ -144,9 +145,10 @@ static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
 				   .features = 0,
 				   .lock = 0,};
 
-static char *audit_feature_names[2] = {
+static char *audit_feature_names[3] = {
 	"only_unset_loginuid",
 	"loginuid_immutable",
+	"audit_output_cmdline",
 };
 
 
@@ -1691,6 +1693,39 @@ 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;
+
+	/* Ensure that the feature is set */
+	if (!is_audit_feature_set(AUDIT_FEATURE_CMDLINE_OUTPUT))
+		return;
+
+	/* Get the process cmdline */
+	page = __get_free_page(GFP_TEMPORARY);
+	if (!page)
+		return;
+
+	len = proc_pid_cmdline(tsk, (char *)page);
+	if (len <= 0) {
+		free_page(page);
+		return;
+	}
+
+	/*
+	* 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_page(page);
+}
+
 void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 {
 	const struct cred *cred;
@@ -1739,6 +1774,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 		up_read(&mm->mmap_sem);
 	}
 	audit_log_task_context(ab);
+	audit_log_add_cmdline(ab, tsk);
 }
 EXPORT_SYMBOL(audit_log_task_info);
 
-- 
1.7.9.5