introduce get_cmdline() for retreiving the value of a processes
proc/self/cmdline value.
Acked-by: David Rientjes <[email protected]>
Acked-by: Stephen Smalley <[email protected]>
Signed-off-by: William Roberts <[email protected]>
---
include/linux/mm.h | 1 +
mm/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 3552717..01e7970 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1134,6 +1134,7 @@ void account_page_writeback(struct page *page);
int set_page_dirty(struct page *page);
int set_page_dirty_lock(struct page *page);
int clear_page_dirty_for_io(struct page *page);
+int get_cmdline(struct task_struct *task, char *buffer, int buflen);
/* Is the vma a continuation of the stack vma above it? */
static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long addr)
diff --git a/mm/util.c b/mm/util.c
index 808f375..43b4419 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -413,6 +413,54 @@ unsigned long vm_commit_limit(void)
* sysctl_overcommit_ratio / 100) + total_swap_pages;
}
+/**
+ * get_cmdline() - copy the cmdline value to a buffer.
+ * @task: the task whose cmdline value to copy.
+ * @buffer: the buffer to copy to.
+ * @buflen: the length of the buffer. Larger cmdline values are truncated
+ * to this length.
+ * Returns the size of the cmdline field copied. Note that the copy does
+ * not guarantee an ending NULL byte.
+ */
+int get_cmdline(struct task_struct *task, char *buffer, int buflen)
+{
+ int res = 0;
+ unsigned int len;
+ struct mm_struct *mm = get_task_mm(task);
+ if (!mm)
+ goto out;
+ if (!mm->arg_end)
+ goto out_mm; /* Shh! No looking before we're done */
+
+ len = mm->arg_end - mm->arg_start;
+
+ if (len > buflen)
+ len = buflen;
+
+ res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+
+ /*
+ * If the nul at the end of args has been overwritten, then
+ * assume application is using setproctitle(3).
+ */
+ if (res > 0 && buffer[res-1] != '\0' && len < buflen) {
+ len = strnlen(buffer, res);
+ if (len < res) {
+ res = len;
+ } else {
+ len = mm->env_end - mm->env_start;
+ if (len > buflen - res)
+ len = buflen - res;
+ res += access_process_vm(task, mm->env_start,
+ buffer+res, len, 0);
+ res = strnlen(buffer, res);
+ }
+ }
+out_mm:
+ mmput(mm);
+out:
+ return res;
+}
/* Tracepoints definitions. */
EXPORT_TRACEPOINT_SYMBOL(kmalloc);
--
1.7.9.5
Re-factor proc_pid_cmdline() to use get_cmdline() helper
from mm.h.
Acked-by: David Rientjes <[email protected]>
Acked-by: Stephen Smalley <[email protected]>
Signed-off-by: William Roberts <[email protected]>
---
fs/proc/base.c | 36 ++----------------------------------
1 file changed, 2 insertions(+), 34 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 03c8d74..cfd178d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -200,41 +200,9 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
return result;
}
-static int proc_pid_cmdline(struct task_struct *task, char * buffer)
+static int proc_pid_cmdline(struct task_struct *task, char *buffer)
{
- int res = 0;
- unsigned int len;
- struct mm_struct *mm = get_task_mm(task);
- if (!mm)
- goto out;
- if (!mm->arg_end)
- goto out_mm; /* Shh! No looking before we're done */
-
- len = mm->arg_end - mm->arg_start;
-
- if (len > PAGE_SIZE)
- len = PAGE_SIZE;
-
- res = access_process_vm(task, mm->arg_start, buffer, len, 0);
-
- // If the nul at the end of args has been overwritten, then
- // assume application is using setproctitle(3).
- if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
- len = strnlen(buffer, res);
- if (len < res) {
- res = len;
- } else {
- len = mm->env_end - mm->env_start;
- if (len > PAGE_SIZE - res)
- len = PAGE_SIZE - res;
- res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
- res = strnlen(buffer, res);
- }
- }
-out_mm:
- mmput(mm);
-out:
- return res;
+ return get_cmdline(task, buffer, PAGE_SIZE);
}
static int proc_pid_auxv(struct task_struct *task, char *buffer)
--
1.7.9.5
During an audit event, cache and print the value of the process's
cmdline value (proc/<pid>/cmdline). This is useful in situations
where processes are started via fork'd virtual machines where the
comm field is incorrect. Often times, setting the comm field still
is insufficient as the comm width is not very wide and most
virtual machine "package names" do not fit. Also, during execution,
many threads have their comm field set as well. By tying it back to
the global cmdline value for the process, audit records will be more
complete in systems with these properties. An example of where this
is useful and applicable is in the realm of Android. With Android,
their is no fork/exec for VM instances. The bare, preloaded Dalvik
VM listens for a fork and specialize request. When this request comes
in, the VM forks, and the loads the specific application (specializing).
This was done to take advantage of COW and to not require a load of
basic packages by the VM on very app spawn. When this spawn occurs,
the package name is set via setproctitle() and shows up in procfs.
Many of these package names are longer then 16 bytes, the historical
width of task->comm. Having the cmdline in the audit records will
couple the application back to the record directly. Also, on my
Debian development box, some audit records were more useful then
what was printed under comm.
The cached cmdline is tied to the life-cycle of the audit_context
structure and is built on demand.
Example denial prior to patch (Ubuntu):
CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 ses=4294967295 tty=(none) comm="console-kit-dae" exe="/usr/sbin/console-kit-daemon" subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
After Patches (Ubuntu):
type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 ses=4294967295 tty=(none) comm="console-kit-dae" exe="/usr/sbin/console-kit-daemon" subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
Example denial prior to patch (Android):
type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000 success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858 auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002 sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker" exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
After Patches (Android):
type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000 success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858 auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002 sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker" exe="/system/bin/app_process" cmdline="com.android.bluetooth" subj=u:r:bluetooth:s0 key=(null)
Signed-off-by: William Roberts <[email protected]>
---
kernel/audit.h | 1 +
kernel/auditsc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/kernel/audit.h b/kernel/audit.h
index b779642..bd6211f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -202,6 +202,7 @@ struct audit_context {
} execve;
};
int fds[2];
+ char *cmdline;
#if AUDIT_DEBUG
int put_count;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 90594c9..cadee2b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -79,6 +79,9 @@
/* no execve audit message should be longer than this (userspace limits) */
#define MAX_EXECVE_AUDIT_LEN 7500
+/* max length to print of cmdline value during audit */
+#define MAX_CMDLINE_AUDIT_LEN 128
+
/* number of audit rules */
int audit_n_rules;
@@ -842,6 +845,12 @@ static inline struct audit_context *audit_get_context(struct task_struct *tsk,
return context;
}
+static inline void audit_cmdline_free(struct audit_context *context)
+{
+ kfree(context->cmdline);
+ context->cmdline = NULL;
+}
+
static inline void audit_free_names(struct audit_context *context)
{
struct audit_names *n, *next;
@@ -955,6 +964,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);
}
@@ -1271,6 +1281,41 @@ static void show_special(struct audit_context *context, int *call_panic)
audit_log_end(ab);
}
+static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
+ struct audit_context *context)
+{
+ int res;
+ char *buf;
+ char *msg = "(null)";
+ audit_log_format(ab, " cmdline=");
+
+ /* Not cached */
+ if (!context->cmdline) {
+ buf = kmalloc(MAX_CMDLINE_AUDIT_LEN, GFP_KERNEL);
+ if (!buf)
+ goto out;
+ res = get_cmdline(tsk, buf, MAX_CMDLINE_AUDIT_LEN);
+ if (res == 0) {
+ kfree(buf);
+ goto out;
+ }
+ /*
+ * Ensure NULL terminated but don't clobber the end
+ * unless the buffer is full. Worst case you end up
+ * with 2 null bytes ending it. By doing it this way
+ * one avoids additional branching. One checking if the
+ * end is null and another to check if their should be
+ * an increment before setting the null byte.
+ */
+ res -= res == PATH_MAX;
+ buf[res] = '\0';
+ context->cmdline = buf;
+ }
+ msg = context->cmdline;
+out:
+ audit_log_untrustedstring(ab, msg);
+}
+
static void audit_log_exit(struct audit_context *context, struct task_struct *tsk)
{
int i, call_panic = 0;
@@ -1303,6 +1348,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
audit_log_task_info(ab, tsk);
audit_log_key(ab, context->filterkey);
+ audit_log_cmdline(ab, tsk, context);
audit_log_end(ab);
for (aux = context->aux; aux; aux = aux->next) {
--
1.7.9.5
On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
> During an audit event, cache and print the value of the process's
> cmdline value (proc/<pid>/cmdline). This is useful in situations
> where processes are started via fork'd virtual machines where the
> comm field is incorrect. Often times, setting the comm field still
> is insufficient as the comm width is not very wide and most
> virtual machine "package names" do not fit. Also, during execution,
> many threads have their comm field set as well. By tying it back to
> the global cmdline value for the process, audit records will be more
> complete in systems with these properties. An example of where this
> is useful and applicable is in the realm of Android. With Android,
> their is no fork/exec for VM instances. The bare, preloaded Dalvik
> VM listens for a fork and specialize request. When this request comes
> in, the VM forks, and the loads the specific application (specializing).
> This was done to take advantage of COW and to not require a load of
> basic packages by the VM on very app spawn. When this spawn occurs,
> the package name is set via setproctitle() and shows up in procfs.
> Many of these package names are longer then 16 bytes, the historical
> width of task->comm. Having the cmdline in the audit records will
> couple the application back to the record directly. Also, on my
> Debian development box, some audit records were more useful then
> what was printed under comm.
>
> The cached cmdline is tied to the life-cycle of the audit_context
> structure and is built on demand.
I don't think its a good idea to do this for a number of reasons.
1) don't we have a record type for command line and its arguments? Shouldn't
we use that auxiliary record if we do this?
2) we don't want each and every syscall record to grow huge(r). Remember the
command line can be virtually unlimited in length. Adding this will consume
disk space and we will be able to keep less records than we currently do.
3) User space will now have to parse this field, too. If everything is in 1
field, how can you tell the command from its arguments considering the command
name could have spaces in it. What if the arguments have spaces in them?
Its far better to fix cmd to be bigger than 16 characters than add all this
extra information that is not needed in the audit logs.
-Steve
> Example denial prior to patch (Ubuntu):
> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes
> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> ses=4294967295 tty=(none) comm="console-kit-dae"
> exe="/usr/sbin/console-kit-daemon"
> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
>
> After Patches (Ubuntu):
> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> ses=4294967295 tty=(none) comm="console-kit-dae"
> exe="/usr/sbin/console-kit-daemon"
> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
>
> Example denial prior to patch (Android):
> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
>
> After Patches (Android):
> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
> subj=u:r:bluetooth:s0 key=(null)
>
> Signed-off-by: William Roberts <[email protected]>
> ---
> kernel/audit.h | 1 +
> kernel/auditsc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index b779642..bd6211f 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -202,6 +202,7 @@ struct audit_context {
> } execve;
> };
> int fds[2];
> + char *cmdline;
>
> #if AUDIT_DEBUG
> int put_count;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 90594c9..cadee2b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -79,6 +79,9 @@
> /* no execve audit message should be longer than this (userspace limits) */
> #define MAX_EXECVE_AUDIT_LEN 7500
>
> +/* max length to print of cmdline value during audit */
> +#define MAX_CMDLINE_AUDIT_LEN 128
> +
> /* number of audit rules */
> int audit_n_rules;
>
> @@ -842,6 +845,12 @@ static inline struct audit_context
> *audit_get_context(struct task_struct *tsk, return context;
> }
>
> +static inline void audit_cmdline_free(struct audit_context *context)
> +{
> + kfree(context->cmdline);
> + context->cmdline = NULL;
> +}
> +
> static inline void audit_free_names(struct audit_context *context)
> {
> struct audit_names *n, *next;
> @@ -955,6 +964,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);
> }
>
> @@ -1271,6 +1281,41 @@ static void show_special(struct audit_context
> *context, int *call_panic) audit_log_end(ab);
> }
>
> +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct
> *tsk, + struct audit_context *context)
> +{
> + int res;
> + char *buf;
> + char *msg = "(null)";
> + audit_log_format(ab, " cmdline=");
> +
> + /* Not cached */
> + if (!context->cmdline) {
> + buf = kmalloc(MAX_CMDLINE_AUDIT_LEN, GFP_KERNEL);
> + if (!buf)
> + goto out;
> + res = get_cmdline(tsk, buf, MAX_CMDLINE_AUDIT_LEN);
> + if (res == 0) {
> + kfree(buf);
> + goto out;
> + }
> + /*
> + * Ensure NULL terminated but don't clobber the end
> + * unless the buffer is full. Worst case you end up
> + * with 2 null bytes ending it. By doing it this way
> + * one avoids additional branching. One checking if the
> + * end is null and another to check if their should be
> + * an increment before setting the null byte.
> + */
> + res -= res == PATH_MAX;
> + buf[res] = '\0';
> + context->cmdline = buf;
> + }
> + msg = context->cmdline;
> +out:
> + audit_log_untrustedstring(ab, msg);
> +}
> +
> static void audit_log_exit(struct audit_context *context, struct
> task_struct *tsk) {
> int i, call_panic = 0;
> @@ -1303,6 +1348,7 @@ static void audit_log_exit(struct audit_context
> *context, struct task_struct *ts
>
> audit_log_task_info(ab, tsk);
> audit_log_key(ab, context->filterkey);
> + audit_log_cmdline(ab, tsk, context);
> audit_log_end(ab);
>
> for (aux = context->aux; aux; aux = aux->next) {
On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb <[email protected]> wrote:
> On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
>> During an audit event, cache and print the value of the process's
>> cmdline value (proc/<pid>/cmdline). This is useful in situations
>> where processes are started via fork'd virtual machines where the
>> comm field is incorrect. Often times, setting the comm field still
>> is insufficient as the comm width is not very wide and most
>> virtual machine "package names" do not fit. Also, during execution,
>> many threads have their comm field set as well. By tying it back to
>> the global cmdline value for the process, audit records will be more
>> complete in systems with these properties. An example of where this
>> is useful and applicable is in the realm of Android. With Android,
>> their is no fork/exec for VM instances. The bare, preloaded Dalvik
>> VM listens for a fork and specialize request. When this request comes
>> in, the VM forks, and the loads the specific application (specializing).
>> This was done to take advantage of COW and to not require a load of
>> basic packages by the VM on very app spawn. When this spawn occurs,
>> the package name is set via setproctitle() and shows up in procfs.
>> Many of these package names are longer then 16 bytes, the historical
>> width of task->comm. Having the cmdline in the audit records will
>> couple the application back to the record directly. Also, on my
>> Debian development box, some audit records were more useful then
>> what was printed under comm.
>>
>> The cached cmdline is tied to the life-cycle of the audit_context
>> structure and is built on demand.
>
> I don't think its a good idea to do this for a number of reasons.
> 1) don't we have a record type for command line and its arguments? Shouldn't
> we use that auxiliary record if we do this?
Doing this in userspace means each and every user-space would have to be patched
to support this. Other people from various systems have jumped in
adding how this
would be beneficial to their cause. The data is right here in the kernel.
> 2) we don't want each and every syscall record to grow huge(r). Remember the
> command line can be virtually unlimited in length. Adding this will consume
> disk space and we will be able to keep less records than we currently do.
We cap it at 128 chars in v3 patch, and then this value can be altered out of
tree and tuned for various systems.
> 3) User space will now have to parse this field, too. If everything is in 1
> field, how can you tell the command from its arguments considering the command
> name could have spaces in it. What if the arguments have spaces in them?
How did bash figure this out to run the command? All the fields in audit
are KVP based, the parsing is pretty straight forward.
On the event of weird chars, it gets hex escaped.
>
> Its far better to fix cmd to be bigger than 16 characters than add all this
> extra information that is not needed in the audit logs.
Rather then use some transient noon-pageable kernel memory for this,
you are suggesting using static,
non-page-able kernel memory for the whole life of every process? What about
cases where audit is disabled. This will greatly bloat the kernel. I
have brought up cranking up the
width but the general reaction is, this is not a good idea.
>
> -Steve
>
>
>> Example denial prior to patch (Ubuntu):
>> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes
>> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
>> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
>> ses=4294967295 tty=(none) comm="console-kit-dae"
>> exe="/usr/sbin/console-kit-daemon"
>> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
>>
>> After Patches (Ubuntu):
>> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
>> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
>> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
>> ses=4294967295 tty=(none) comm="console-kit-dae"
>> exe="/usr/sbin/console-kit-daemon"
>> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
>> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
>>
>> Example denial prior to patch (Android):
>> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
>> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
>> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
>> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
>>
>> After Patches (Android):
>> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
>> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
>> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
>> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
>> subj=u:r:bluetooth:s0 key=(null)
>>
>> Signed-off-by: William Roberts <[email protected]>
>> ---
>> kernel/audit.h | 1 +
>> kernel/auditsc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>> diff --git a/kernel/audit.h b/kernel/audit.h
>> index b779642..bd6211f 100644
>> --- a/kernel/audit.h
>> +++ b/kernel/audit.h
>> @@ -202,6 +202,7 @@ struct audit_context {
>> } execve;
>> };
>> int fds[2];
>> + char *cmdline;
>>
>> #if AUDIT_DEBUG
>> int put_count;
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 90594c9..cadee2b 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -79,6 +79,9 @@
>> /* no execve audit message should be longer than this (userspace limits) */
>> #define MAX_EXECVE_AUDIT_LEN 7500
>>
>> +/* max length to print of cmdline value during audit */
>> +#define MAX_CMDLINE_AUDIT_LEN 128
>> +
>> /* number of audit rules */
>> int audit_n_rules;
>>
>> @@ -842,6 +845,12 @@ static inline struct audit_context
>> *audit_get_context(struct task_struct *tsk, return context;
>> }
>>
>> +static inline void audit_cmdline_free(struct audit_context *context)
>> +{
>> + kfree(context->cmdline);
>> + context->cmdline = NULL;
>> +}
>> +
>> static inline void audit_free_names(struct audit_context *context)
>> {
>> struct audit_names *n, *next;
>> @@ -955,6 +964,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);
>> }
>>
>> @@ -1271,6 +1281,41 @@ static void show_special(struct audit_context
>> *context, int *call_panic) audit_log_end(ab);
>> }
>>
>> +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct
>> *tsk, + struct audit_context *context)
>> +{
>> + int res;
>> + char *buf;
>> + char *msg = "(null)";
>> + audit_log_format(ab, " cmdline=");
>> +
>> + /* Not cached */
>> + if (!context->cmdline) {
>> + buf = kmalloc(MAX_CMDLINE_AUDIT_LEN, GFP_KERNEL);
>> + if (!buf)
>> + goto out;
>> + res = get_cmdline(tsk, buf, MAX_CMDLINE_AUDIT_LEN);
>> + if (res == 0) {
>> + kfree(buf);
>> + goto out;
>> + }
>> + /*
>> + * Ensure NULL terminated but don't clobber the end
>> + * unless the buffer is full. Worst case you end up
>> + * with 2 null bytes ending it. By doing it this way
>> + * one avoids additional branching. One checking if the
>> + * end is null and another to check if their should be
>> + * an increment before setting the null byte.
>> + */
>> + res -= res == PATH_MAX;
>> + buf[res] = '\0';
>> + context->cmdline = buf;
>> + }
>> + msg = context->cmdline;
>> +out:
>> + audit_log_untrustedstring(ab, msg);
>> +}
>> +
>> static void audit_log_exit(struct audit_context *context, struct
>> task_struct *tsk) {
>> int i, call_panic = 0;
>> @@ -1303,6 +1348,7 @@ static void audit_log_exit(struct audit_context
>> *context, struct task_struct *ts
>>
>> audit_log_task_info(ab, tsk);
>> audit_log_key(ab, context->filterkey);
>> + audit_log_cmdline(ab, tsk, context);
>> audit_log_end(ab);
>>
>> for (aux = context->aux; aux; aux = aux->next) {
--
Respectfully,
William C Roberts
On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote:
> On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb <[email protected]> wrote:
> > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
> >> During an audit event, cache and print the value of the process's
> >> cmdline value (proc/<pid>/cmdline). This is useful in situations
> >> where processes are started via fork'd virtual machines where the
> >> comm field is incorrect. Often times, setting the comm field still
> >> is insufficient as the comm width is not very wide and most
> >> virtual machine "package names" do not fit. Also, during execution,
> >> many threads have their comm field set as well. By tying it back to
> >> the global cmdline value for the process, audit records will be more
> >> complete in systems with these properties. An example of where this
> >> is useful and applicable is in the realm of Android. With Android,
> >> their is no fork/exec for VM instances. The bare, preloaded Dalvik
> >> VM listens for a fork and specialize request. When this request comes
> >> in, the VM forks, and the loads the specific application (specializing).
> >> This was done to take advantage of COW and to not require a load of
> >> basic packages by the VM on very app spawn. When this spawn occurs,
> >> the package name is set via setproctitle() and shows up in procfs.
> >> Many of these package names are longer then 16 bytes, the historical
> >> width of task->comm. Having the cmdline in the audit records will
> >> couple the application back to the record directly. Also, on my
> >> Debian development box, some audit records were more useful then
> >> what was printed under comm.
> >>
> >> The cached cmdline is tied to the life-cycle of the audit_context
> >> structure and is built on demand.
> >
> > I don't think its a good idea to do this for a number of reasons.
> > 1) don't we have a record type for command line and its arguments?
> > Shouldn't we use that auxiliary record if we do this?
>
> Doing this in userspace means each and every user-space would have to be
> patched to support this. Other people from various systems have jumped in
> adding how this would be beneficial to their cause. The data is right here in
> the kernel.
I don't mean doing it in user space, I was thinking of perhaps using the
EXECVE auxiliary record type. It looks something like this:
type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping"
a1="koji.fedoraproject.org"
Its a record type we already have with a format that can be parsed.
> > 2) we don't want each and every syscall record to grow huge(r). Remember
> > the command line can be virtually unlimited in length. Adding this will
> > consume disk space and we will be able to keep less records than we
> > currently do.
> We cap it at 128 chars in v3 patch, and then this value can be altered out
> of tree and tuned for various systems.
That still adds up on systems where people really do use audit.
> > 3) User space will now have to parse this field, too. If everything is in
> > 1
> > field, how can you tell the command from its arguments considering the
> > command name could have spaces in it. What if the arguments have spaces
> > in them?
>
> How did bash figure this out to run the command?
It was shell escaped and quoted when bash saw it. Its not when the kernel sees
it.
> All the fields in audit are KVP based, the parsing is pretty straight
> forward.
Try this,
cp /bin/ls 'test test test'
auditctll -a always,exit -F arch=b64 -S stat -k test
./test\ test\ test './test\ test\ test'
auditctl -D
ausearch --start recent --key test
> On the event of weird chars, it gets hex escaped.
and its all in 1 lump with no escaping to figure out what is what.
> > Its far better to fix cmd to be bigger than 16 characters than add all
> > this
> > extra information that is not needed in the audit logs.
>
> Rather then use some transient noon-pageable kernel memory for this,
> you are suggesting using static,
> non-page-able kernel memory for the whole life of every process? What about
> cases where audit is disabled. This will greatly bloat the kernel. I
> have brought up cranking up the
> width but the general reaction is, this is not a good idea.
That is what the problem is. 16 bytes is useless for anything.
-Steve
> >> Example denial prior to patch (Ubuntu):
> >> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes
> >> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> >> ses=4294967295 tty=(none) comm="console-kit-dae"
> >> exe="/usr/sbin/console-kit-daemon"
> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
> >>
> >> After Patches (Ubuntu):
> >> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
> >> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> >> ses=4294967295 tty=(none) comm="console-kit-dae"
> >> exe="/usr/sbin/console-kit-daemon"
> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
> >> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
> >>
> >> Example denial prior to patch (Android):
> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
> >> pid=1858
> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
> >> egid=1002
> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> >> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
> >>
> >> After Patches (Android):
> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
> >> pid=1858
> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
> >> egid=1002
> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> >> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
> >> subj=u:r:bluetooth:s0 key=(null)
> >>
> >> Signed-off-by: William Roberts <[email protected]>
> >> ---
> >>
> >> kernel/audit.h | 1 +
> >> kernel/auditsc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 47 insertions(+)
> >>
> >> diff --git a/kernel/audit.h b/kernel/audit.h
> >> index b779642..bd6211f 100644
> >> --- a/kernel/audit.h
> >> +++ b/kernel/audit.h
> >> @@ -202,6 +202,7 @@ struct audit_context {
> >>
> >> } execve;
> >>
> >> };
> >> int fds[2];
> >>
> >> + char *cmdline;
> >>
> >> #if AUDIT_DEBUG
> >>
> >> int put_count;
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 90594c9..cadee2b 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -79,6 +79,9 @@
> >>
> >> /* no execve audit message should be longer than this (userspace limits)
> >> */
> >>
> >> #define MAX_EXECVE_AUDIT_LEN 7500
> >>
> >> +/* max length to print of cmdline value during audit */
> >> +#define MAX_CMDLINE_AUDIT_LEN 128
> >> +
> >>
> >> /* number of audit rules */
> >> int audit_n_rules;
> >>
> >> @@ -842,6 +845,12 @@ static inline struct audit_context
> >> *audit_get_context(struct task_struct *tsk, return context;
> >>
> >> }
> >>
> >> +static inline void audit_cmdline_free(struct audit_context *context)
> >> +{
> >> + kfree(context->cmdline);
> >> + context->cmdline = NULL;
> >> +}
> >> +
> >>
> >> static inline void audit_free_names(struct audit_context *context)
> >> {
> >>
> >> struct audit_names *n, *next;
> >>
> >> @@ -955,6 +964,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);
> >>
> >> }
> >>
> >> @@ -1271,6 +1281,41 @@ static void show_special(struct audit_context
> >> *context, int *call_panic) audit_log_end(ab);
> >>
> >> }
> >>
> >> +static void audit_log_cmdline(struct audit_buffer *ab, struct
> >> task_struct
> >> *tsk, + struct audit_context *context)
> >> +{
> >> + int res;
> >> + char *buf;
> >> + char *msg = "(null)";
> >> + audit_log_format(ab, " cmdline=");
> >> +
> >> + /* Not cached */
> >> + if (!context->cmdline) {
> >> + buf = kmalloc(MAX_CMDLINE_AUDIT_LEN, GFP_KERNEL);
> >> + if (!buf)
> >> + goto out;
> >> + res = get_cmdline(tsk, buf, MAX_CMDLINE_AUDIT_LEN);
> >> + if (res == 0) {
> >> + kfree(buf);
> >> + goto out;
> >> + }
> >> + /*
> >> + * Ensure NULL terminated but don't clobber the end
> >> + * unless the buffer is full. Worst case you end up
> >> + * with 2 null bytes ending it. By doing it this way
> >> + * one avoids additional branching. One checking if the
> >> + * end is null and another to check if their should be
> >> + * an increment before setting the null byte.
> >> + */
> >> + res -= res == PATH_MAX;
> >> + buf[res] = '\0';
> >> + context->cmdline = buf;
> >> + }
> >> + msg = context->cmdline;
> >> +out:
> >> + audit_log_untrustedstring(ab, msg);
> >> +}
> >> +
> >>
> >> static void audit_log_exit(struct audit_context *context, struct
> >>
> >> task_struct *tsk) {
> >>
> >> int i, call_panic = 0;
> >>
> >> @@ -1303,6 +1348,7 @@ static void audit_log_exit(struct audit_context
> >> *context, struct task_struct *ts
> >>
> >> audit_log_task_info(ab, tsk);
> >> audit_log_key(ab, context->filterkey);
> >>
> >> + audit_log_cmdline(ab, tsk, context);
> >>
> >> audit_log_end(ab);
> >>
> >> for (aux = context->aux; aux; aux = aux->next) {
On Wed, Jan 15, 2014 at 5:33 PM, Steve Grubb <[email protected]> wrote:
> On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote:
>> On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb <[email protected]> wrote:
>> > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
>> >> During an audit event, cache and print the value of the process's
>> >> cmdline value (proc/<pid>/cmdline). This is useful in situations
>> >> where processes are started via fork'd virtual machines where the
>> >> comm field is incorrect. Often times, setting the comm field still
>> >> is insufficient as the comm width is not very wide and most
>> >> virtual machine "package names" do not fit. Also, during execution,
>> >> many threads have their comm field set as well. By tying it back to
>> >> the global cmdline value for the process, audit records will be more
>> >> complete in systems with these properties. An example of where this
>> >> is useful and applicable is in the realm of Android. With Android,
>> >> their is no fork/exec for VM instances. The bare, preloaded Dalvik
>> >> VM listens for a fork and specialize request. When this request comes
>> >> in, the VM forks, and the loads the specific application (specializing).
>> >> This was done to take advantage of COW and to not require a load of
>> >> basic packages by the VM on very app spawn. When this spawn occurs,
>> >> the package name is set via setproctitle() and shows up in procfs.
>> >> Many of these package names are longer then 16 bytes, the historical
>> >> width of task->comm. Having the cmdline in the audit records will
>> >> couple the application back to the record directly. Also, on my
>> >> Debian development box, some audit records were more useful then
>> >> what was printed under comm.
>> >>
>> >> The cached cmdline is tied to the life-cycle of the audit_context
>> >> structure and is built on demand.
>> >
>> > I don't think its a good idea to do this for a number of reasons.
>> > 1) don't we have a record type for command line and its arguments?
>> > Shouldn't we use that auxiliary record if we do this?
>>
>> Doing this in userspace means each and every user-space would have to be
>> patched to support this. Other people from various systems have jumped in
>> adding how this would be beneficial to their cause. The data is right here in
>> the kernel.
>
> I don't mean doing it in user space, I was thinking of perhaps using the
> EXECVE auxiliary record type. It looks something like this:
>
> type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping"
> a1="koji.fedoraproject.org"
>
> Its a record type we already have with a format that can be parsed.
>
Their is no execve in my use case, so this record would never, ever
be emitted.
>
>> > 2) we don't want each and every syscall record to grow huge(r). Remember
>> > the command line can be virtually unlimited in length. Adding this will
>> > consume disk space and we will be able to keep less records than we
>> > currently do.
>> We cap it at 128 chars in v3 patch, and then this value can be altered out
>> of tree and tuned for various systems.
>
> That still adds up on systems where people really do use audit.
Yes, but is less then setting comm width to 128, and its only
transient use, not static and un yielding in its use.
>
>
>> > 3) User space will now have to parse this field, too. If everything is in
>> > 1
>> > field, how can you tell the command from its arguments considering the
>> > command name could have spaces in it. What if the arguments have spaces
>> > in them?
>>
>> How did bash figure this out to run the command?
>
> It was shell escaped and quoted when bash saw it. Its not when the kernel sees
> it.
>
>> All the fields in audit are KVP based, the parsing is pretty straight
>> forward.
>
> Try this,
>
> cp /bin/ls 'test test test'
> auditctll -a always,exit -F arch=b64 -S stat -k test
> ./test\ test\ test './test\ test\ test'
> auditctl -D
> ausearch --start recent --key test
>
>
>> On the event of weird chars, it gets hex escaped.
>
> and its all in 1 lump with no escaping to figure out what is what.
Un-escape it. ausearch does this with paths. Then if you need to parse
it, do it.
>
>
>> > Its far better to fix cmd to be bigger than 16 characters than add all
>> > this
>> > extra information that is not needed in the audit logs.
>>
>> Rather then use some transient noon-pageable kernel memory for this,
>> you are suggesting using static,
>> non-page-able kernel memory for the whole life of every process? What about
>> cases where audit is disabled. This will greatly bloat the kernel. I
>> have brought up cranking up the
>> width but the general reaction is, this is not a good idea.
>
> That is what the problem is. 16 bytes is useless for anything.
>
Well I agree with that statement, its not going to change anytime
soon. And also has the
drawbacks ive mentioned. Also, their is another limitation. What if a
thread sets its title. Its
possible for each thread to have its own comm field. This cmdlien
value ties it back to a global entity. Any
thread can change it, but the value will be the same across all threads.
>
>
>> >> Example denial prior to patch (Ubuntu):
>> >> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82 success=yes
>> >> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
>> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
>> >> ses=4294967295 tty=(none) comm="console-kit-dae"
>> >> exe="/usr/sbin/console-kit-daemon"
>> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
>> >>
>> >> After Patches (Ubuntu):
>> >> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
>> >> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
>> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
>> >> ses=4294967295 tty=(none) comm="console-kit-dae"
>> >> exe="/usr/sbin/console-kit-daemon"
>> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
>> >> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
>> >>
>> >> Example denial prior to patch (Android):
>> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
>> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
>> >> pid=1858
>> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
>> >> egid=1002
>> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> >> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
>> >>
>> >> After Patches (Android):
>> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54 per=840000
>> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
>> >> pid=1858
>> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
>> >> egid=1002
>> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> >> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
>> >> subj=u:r:bluetooth:s0 key=(null)
>> >>
>> >> Signed-off-by: William Roberts <[email protected]>
>> >> ---
>> >>
>> >> kernel/audit.h | 1 +
>> >> kernel/auditsc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> >> 2 files changed, 47 insertions(+)
>> >>
>> >> diff --git a/kernel/audit.h b/kernel/audit.h
>> >> index b779642..bd6211f 100644
>> >> --- a/kernel/audit.h
>> >> +++ b/kernel/audit.h
>> >> @@ -202,6 +202,7 @@ struct audit_context {
>> >>
>> >> } execve;
>> >>
>> >> };
>> >> int fds[2];
>> >>
>> >> + char *cmdline;
>> >>
>> >> #if AUDIT_DEBUG
>> >>
>> >> int put_count;
>> >>
>> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> index 90594c9..cadee2b 100644
>> >> --- a/kernel/auditsc.c
>> >> +++ b/kernel/auditsc.c
>> >> @@ -79,6 +79,9 @@
>> >>
>> >> /* no execve audit message should be longer than this (userspace limits)
>> >> */
>> >>
>> >> #define MAX_EXECVE_AUDIT_LEN 7500
>> >>
>> >> +/* max length to print of cmdline value during audit */
>> >> +#define MAX_CMDLINE_AUDIT_LEN 128
>> >> +
>> >>
>> >> /* number of audit rules */
>> >> int audit_n_rules;
>> >>
>> >> @@ -842,6 +845,12 @@ static inline struct audit_context
>> >> *audit_get_context(struct task_struct *tsk, return context;
>> >>
>> >> }
>> >>
>> >> +static inline void audit_cmdline_free(struct audit_context *context)
>> >> +{
>> >> + kfree(context->cmdline);
>> >> + context->cmdline = NULL;
>> >> +}
>> >> +
>> >>
>> >> static inline void audit_free_names(struct audit_context *context)
>> >> {
>> >>
>> >> struct audit_names *n, *next;
>> >>
>> >> @@ -955,6 +964,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);
>> >>
>> >> }
>> >>
>> >> @@ -1271,6 +1281,41 @@ static void show_special(struct audit_context
>> >> *context, int *call_panic) audit_log_end(ab);
>> >>
>> >> }
>> >>
>> >> +static void audit_log_cmdline(struct audit_buffer *ab, struct
>> >> task_struct
>> >> *tsk, + struct audit_context *context)
>> >> +{
>> >> + int res;
>> >> + char *buf;
>> >> + char *msg = "(null)";
>> >> + audit_log_format(ab, " cmdline=");
>> >> +
>> >> + /* Not cached */
>> >> + if (!context->cmdline) {
>> >> + buf = kmalloc(MAX_CMDLINE_AUDIT_LEN, GFP_KERNEL);
>> >> + if (!buf)
>> >> + goto out;
>> >> + res = get_cmdline(tsk, buf, MAX_CMDLINE_AUDIT_LEN);
>> >> + if (res == 0) {
>> >> + kfree(buf);
>> >> + goto out;
>> >> + }
>> >> + /*
>> >> + * Ensure NULL terminated but don't clobber the end
>> >> + * unless the buffer is full. Worst case you end up
>> >> + * with 2 null bytes ending it. By doing it this way
>> >> + * one avoids additional branching. One checking if the
>> >> + * end is null and another to check if their should be
>> >> + * an increment before setting the null byte.
>> >> + */
>> >> + res -= res == PATH_MAX;
>> >> + buf[res] = '\0';
>> >> + context->cmdline = buf;
>> >> + }
>> >> + msg = context->cmdline;
>> >> +out:
>> >> + audit_log_untrustedstring(ab, msg);
>> >> +}
>> >> +
>> >>
>> >> static void audit_log_exit(struct audit_context *context, struct
>> >>
>> >> task_struct *tsk) {
>> >>
>> >> int i, call_panic = 0;
>> >>
>> >> @@ -1303,6 +1348,7 @@ static void audit_log_exit(struct audit_context
>> >> *context, struct task_struct *ts
>> >>
>> >> audit_log_task_info(ab, tsk);
>> >> audit_log_key(ab, context->filterkey);
>> >>
>> >> + audit_log_cmdline(ab, tsk, context);
>> >>
>> >> audit_log_end(ab);
>> >>
>> >> for (aux = context->aux; aux; aux = aux->next) {
>
--
Respectfully,
William C Roberts
On Wednesday, January 15, 2014 05:44:29 PM William Roberts wrote:
> On Wed, Jan 15, 2014 at 5:33 PM, Steve Grubb <[email protected]> wrote:
> > On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote:
> >> On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb <[email protected]> wrote:
> >> > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
> >> >> During an audit event, cache and print the value of the process's
> >> >> cmdline value (proc/<pid>/cmdline). This is useful in situations
> >> >> where processes are started via fork'd virtual machines where the
> >> >> comm field is incorrect. Often times, setting the comm field still
> >> >> is insufficient as the comm width is not very wide and most
> >> >> virtual machine "package names" do not fit. Also, during execution,
> >> >> many threads have their comm field set as well. By tying it back to
> >> >> the global cmdline value for the process, audit records will be more
> >> >> complete in systems with these properties. An example of where this
> >> >> is useful and applicable is in the realm of Android. With Android,
> >> >> their is no fork/exec for VM instances. The bare, preloaded Dalvik
> >> >> VM listens for a fork and specialize request. When this request comes
> >> >> in, the VM forks, and the loads the specific application
> >> >> (specializing).
> >> >> This was done to take advantage of COW and to not require a load of
> >> >> basic packages by the VM on very app spawn. When this spawn occurs,
> >> >> the package name is set via setproctitle() and shows up in procfs.
> >> >> Many of these package names are longer then 16 bytes, the historical
> >> >> width of task->comm. Having the cmdline in the audit records will
> >> >> couple the application back to the record directly. Also, on my
> >> >> Debian development box, some audit records were more useful then
> >> >> what was printed under comm.
> >> >>
> >> >> The cached cmdline is tied to the life-cycle of the audit_context
> >> >> structure and is built on demand.
> >> >
> >> > I don't think its a good idea to do this for a number of reasons.
> >> > 1) don't we have a record type for command line and its arguments?
> >> > Shouldn't we use that auxiliary record if we do this?
> >>
> >> Doing this in userspace means each and every user-space would have to be
> >> patched to support this. Other people from various systems have jumped in
> >> adding how this would be beneficial to their cause. The data is right
> >> here in the kernel.
> >
> > I don't mean doing it in user space, I was thinking of perhaps using the
> > EXECVE auxiliary record type. It looks something like this:
> >
> > type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping"
> > a1="koji.fedoraproject.org"
> >
> > Its a record type we already have with a format that can be parsed.
>
> Their is no execve in my use case, so this record would never, ever
> be emitted.
Well, that record has mostly the same fields that you are publishing except
they are formatted in a way that is usable. What I was suggesting is populate
the fields and add the auxiliary record.
> >> > 2) we don't want each and every syscall record to grow huge(r).
> >> > Remember
> >> > the command line can be virtually unlimited in length. Adding this will
> >> > consume disk space and we will be able to keep less records than we
> >> > currently do.
> >>
> >> We cap it at 128 chars in v3 patch, and then this value can be altered
> >> out
> >> of tree and tuned for various systems.
> >
> > That still adds up on systems where people really do use audit.
>
> Yes, but is less then setting comm width to 128, and its only
> transient use, not static and un yielding in its use.
I think that would be more efficient in terms of disk space usage. We don't need
all the arguments for every syscall audit event. We just need the program name
in full.
> >> > 3) User space will now have to parse this field, too. If everything is
> >> > in
> >> > 1
> >> > field, how can you tell the command from its arguments considering the
> >> > command name could have spaces in it. What if the arguments have spaces
> >> > in them?
> >>
> >> How did bash figure this out to run the command?
> >
> > It was shell escaped and quoted when bash saw it. Its not when the kernel
> > sees it.
> >
> >> All the fields in audit are KVP based, the parsing is pretty straight
> >> forward.
> >
> > Try this,
> >
> > cp /bin/ls 'test test test'
> > auditctll -a always,exit -F arch=b64 -S stat -k test
> > ./test\ test\ test './test\ test\ test'
> > auditctl -D
> > ausearch --start recent --key test
> >
> >> On the event of weird chars, it gets hex escaped.
> >
> > and its all in 1 lump with no escaping to figure out what is what.
>
> Un-escape it. ausearch does this with paths. Then if you need to parse
> it, do it.
How can you? When you unescape cmdline for the example I gave, you will have
"./test test test ./test test test". Which program ran and how many arguments
were passed? If we are trying to improve on what comm= provides by having the
full information, I have to be able to find out exactly what the program name
was so it can be used for searching. If that can't be done, then we don't need
this addition in its current form.
-Steve
> >> > Its far better to fix cmd to be bigger than 16 characters than add all
> >> > this
> >> > extra information that is not needed in the audit logs.
> >>
> >> Rather then use some transient noon-pageable kernel memory for this,
> >> you are suggesting using static,
> >> non-page-able kernel memory for the whole life of every process? What
> >> about
> >> cases where audit is disabled. This will greatly bloat the kernel. I
> >> have brought up cranking up the
> >> width but the general reaction is, this is not a good idea.
> >
> > That is what the problem is. 16 bytes is useless for anything.
>
> Well I agree with that statement, its not going to change anytime
> soon. And also has the
> drawbacks ive mentioned. Also, their is another limitation. What if a
> thread sets its title. Its
> possible for each thread to have its own comm field. This cmdlien
> value ties it back to a global entity. Any
> thread can change it, but the value will be the same across all threads.
>
> >> >> Example denial prior to patch (Ubuntu):
> >> >> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
> >> >> success=yes
> >> >> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> >> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> >> >> fsgid=0
> >> >> ses=4294967295 tty=(none) comm="console-kit-dae"
> >> >> exe="/usr/sbin/console-kit-daemon"
> >> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
> >> >>
> >> >> After Patches (Ubuntu):
> >> >> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
> >> >> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1
> >> >> pid=1329
> >> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> >> >> fsgid=0
> >> >> ses=4294967295 tty=(none) comm="console-kit-dae"
> >> >> exe="/usr/sbin/console-kit-daemon"
> >> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
> >> >> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
> >> >>
> >> >> Example denial prior to patch (Android):
> >> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54
> >> >> per=840000
> >> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
> >> >> pid=1858
> >> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
> >> >> egid=1002
> >> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> >> >> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
> >> >>
> >> >> After Patches (Android):
> >> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54
> >> >> per=840000
> >> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
> >> >> pid=1858
> >> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
> >> >> egid=1002
> >> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> >> >> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
> >> >> subj=u:r:bluetooth:s0 key=(null)
> >> >>
> >> >> Signed-off-by: William Roberts <[email protected]>
On Wed, Jan 15, 2014 at 8:51 PM, Steve Grubb <[email protected]> wrote:
> On Wednesday, January 15, 2014 05:44:29 PM William Roberts wrote:
>> On Wed, Jan 15, 2014 at 5:33 PM, Steve Grubb <[email protected]> wrote:
>> > On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote:
>> >> On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb <[email protected]> wrote:
>> >> > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
>> >> >> During an audit event, cache and print the value of the process's
>> >> >> cmdline value (proc/<pid>/cmdline). This is useful in situations
>> >> >> where processes are started via fork'd virtual machines where the
>> >> >> comm field is incorrect. Often times, setting the comm field still
>> >> >> is insufficient as the comm width is not very wide and most
>> >> >> virtual machine "package names" do not fit. Also, during execution,
>> >> >> many threads have their comm field set as well. By tying it back to
>> >> >> the global cmdline value for the process, audit records will be more
>> >> >> complete in systems with these properties. An example of where this
>> >> >> is useful and applicable is in the realm of Android. With Android,
>> >> >> their is no fork/exec for VM instances. The bare, preloaded Dalvik
>> >> >> VM listens for a fork and specialize request. When this request comes
>> >> >> in, the VM forks, and the loads the specific application
>> >> >> (specializing).
>> >> >> This was done to take advantage of COW and to not require a load of
>> >> >> basic packages by the VM on very app spawn. When this spawn occurs,
>> >> >> the package name is set via setproctitle() and shows up in procfs.
>> >> >> Many of these package names are longer then 16 bytes, the historical
>> >> >> width of task->comm. Having the cmdline in the audit records will
>> >> >> couple the application back to the record directly. Also, on my
>> >> >> Debian development box, some audit records were more useful then
>> >> >> what was printed under comm.
>> >> >>
>> >> >> The cached cmdline is tied to the life-cycle of the audit_context
>> >> >> structure and is built on demand.
>> >> >
>> >> > I don't think its a good idea to do this for a number of reasons.
>> >> > 1) don't we have a record type for command line and its arguments?
>> >> > Shouldn't we use that auxiliary record if we do this?
>> >>
>> >> Doing this in userspace means each and every user-space would have to be
>> >> patched to support this. Other people from various systems have jumped in
>> >> adding how this would be beneficial to their cause. The data is right
>> >> here in the kernel.
>> >
>> > I don't mean doing it in user space, I was thinking of perhaps using the
>> > EXECVE auxiliary record type. It looks something like this:
>> >
>> > type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping"
>> > a1="koji.fedoraproject.org"
>> >
>> > Its a record type we already have with a format that can be parsed.
>>
>> Their is no execve in my use case, so this record would never, ever
>> be emitted.
>
> Well, that record has mostly the same fields that you are publishing except
> they are formatted in a way that is usable. What I was suggesting is populate
> the fields and add the auxiliary record.
>
>
>> >> > 2) we don't want each and every syscall record to grow huge(r).
>> >> > Remember
>> >> > the command line can be virtually unlimited in length. Adding this will
>> >> > consume disk space and we will be able to keep less records than we
>> >> > currently do.
>> >>
>> >> We cap it at 128 chars in v3 patch, and then this value can be altered
>> >> out
>> >> of tree and tuned for various systems.
>> >
>> > That still adds up on systems where people really do use audit.
>>
>> Yes, but is less then setting comm width to 128, and its only
>> transient use, not static and un yielding in its use.
>
> I think that would be more efficient in terms of disk space usage. We don't need
> all the arguments for every syscall audit event. We just need the program name
> in full.
>
>
>> >> > 3) User space will now have to parse this field, too. If everything is
>> >> > in
>> >> > 1
>> >> > field, how can you tell the command from its arguments considering the
>> >> > command name could have spaces in it. What if the arguments have spaces
>> >> > in them?
>> >>
>> >> How did bash figure this out to run the command?
>> >
>> > It was shell escaped and quoted when bash saw it. Its not when the kernel
>> > sees it.
>> >
>> >> All the fields in audit are KVP based, the parsing is pretty straight
>> >> forward.
>> >
>> > Try this,
>> >
>> > cp /bin/ls 'test test test'
>> > auditctll -a always,exit -F arch=b64 -S stat -k test
>> > ./test\ test\ test './test\ test\ test'
>> > auditctl -D
>> > ausearch --start recent --key test
>> >
>> >> On the event of weird chars, it gets hex escaped.
>> >
>> > and its all in 1 lump with no escaping to figure out what is what.
>>
>> Un-escape it. ausearch does this with paths. Then if you need to parse
>> it, do it.
>
> How can you? When you unescape cmdline for the example I gave, you will have
> "./test test test ./test test test". Which program ran and how many arguments
> were passed? If we are trying to improve on what comm= provides by having the
> full information, I have to be able to find out exactly what the program name
> was so it can be used for searching. If that can't be done, then we don't need
> this addition in its current form.
In your example, you will have an execve record, with it parsed, will you not?
cmdline does not necessarily represent the arguments or process name.
Sometimes it does, sometimes it doesn't. Just treat the thing as one string,
perhaps do some form of substring matching in a tool. I could set my
proctitle to
mydumbname and then use some tool to search for mydumbname. Even though
the exec record might be totally different. You really don't need to
add any more
searching logic then that.
To make this clear, I am not trying to improve on what comm provides.
comm provides
16 chars for per thread name. The key is, its per thread, and can be
anything. The
"cmdline" value, is an arbitrary spot that is a global entity for the process.
So in my change, all things coming into these events will have a similar
cmdline audit. Which may help in narrowing down on whats going on in the system.
>
> -Steve
>
>
>> >> > Its far better to fix cmd to be bigger than 16 characters than add all
>> >> > this
>> >> > extra information that is not needed in the audit logs.
>> >>
>> >> Rather then use some transient noon-pageable kernel memory for this,
>> >> you are suggesting using static,
>> >> non-page-able kernel memory for the whole life of every process? What
>> >> about
>> >> cases where audit is disabled. This will greatly bloat the kernel. I
>> >> have brought up cranking up the
>> >> width but the general reaction is, this is not a good idea.
>> >
>> > That is what the problem is. 16 bytes is useless for anything.
>>
>> Well I agree with that statement, its not going to change anytime
>> soon. And also has the
>> drawbacks ive mentioned. Also, their is another limitation. What if a
>> thread sets its title. Its
>> possible for each thread to have its own comm field. This cmdlien
>> value ties it back to a global entity. Any
>> thread can change it, but the value will be the same across all threads.
>>
>> >> >> Example denial prior to patch (Ubuntu):
>> >> >> CALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
>> >> >> success=yes
>> >> >> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
>> >> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
>> >> >> fsgid=0
>> >> >> ses=4294967295 tty=(none) comm="console-kit-dae"
>> >> >> exe="/usr/sbin/console-kit-daemon"
>> >> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
>> >> >>
>> >> >> After Patches (Ubuntu):
>> >> >> type=SYSCALL msg=audit(1387828084.070:361): arch=c000003e syscall=82
>> >> >> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1
>> >> >> pid=1329
>> >> >> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
>> >> >> fsgid=0
>> >> >> ses=4294967295 tty=(none) comm="console-kit-dae"
>> >> >> exe="/usr/sbin/console-kit-daemon"
>> >> >> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
>> >> >> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
>> >> >>
>> >> >> Example denial prior to patch (Android):
>> >> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54
>> >> >> per=840000
>> >> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
>> >> >> pid=1858
>> >> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
>> >> >> egid=1002
>> >> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> >> >> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
>> >> >>
>> >> >> After Patches (Android):
>> >> >> type=1300 msg=audit(248323.940:247): arch=40000028 syscall=54
>> >> >> per=840000
>> >> >> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224
>> >> >> pid=1858
>> >> >> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002
>> >> >> egid=1002
>> >> >> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
>> >> >> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
>> >> >> subj=u:r:bluetooth:s0 key=(null)
>> >> >>
>> >> >> Signed-off-by: William Roberts <[email protected]>
>
--
Respectfully,
William C Roberts
On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote:
> >> > Try this,
> >> >
> >> > cp /bin/ls 'test test test'
> >> > auditctll -a always,exit -F arch=b64 -S stat -k test
> >> > ./test\ test\ test './test\ test\ test'
> >> > auditctl -D
> >> > ausearch --start recent --key test
> >> >
> >> >> On the event of weird chars, it gets hex escaped.
> >> >
> >> > and its all in 1 lump with no escaping to figure out what is what.
> >>
> >> Un-escape it. ausearch does this with paths. Then if you need to parse
> >> it, do it.
> >
> > How can you? When you unescape cmdline for the example I gave, you will
> > have "./test test test ./test test test". Which program ran and how many
> > arguments were passed? If we are trying to improve on what comm= provides
> > by having the full information, I have to be able to find out exactly
> > what the program name was so it can be used for searching. If that can't
> > be done, then we don't need this addition in its current form.
>
> In your example, you will have an execve record, with it parsed, will you
> not?
Only if you change your patch.
> cmdline does not necessarily represent the arguments or process name.
> Sometimes it does, sometimes it doesn't. Just treat the thing as one
> string, perhaps do some form of substring matching in a tool.
You are missing the point. The point is that you are trying to place trust in
something that can be gamed. The audit system is designed such that it cannot
be fooled very easily. Each piece of the subject and object are separated so
that programs can be written to analyze events. What I am trying to say is now
you are making something that concatenates fields with no way to regroup them
later to reconstruct what really happened,
> To make this clear, I am not trying to improve on what comm provides.
> comm provides
> 16 chars for per thread name. The key is, its per thread, and can be
> anything. The
> "cmdline" value, is an arbitrary spot that is a global entity for the
> process. So in my change, all things coming into these events will have a
> similar cmdline audit. Which may help in narrowing down on whats going on
> in the system
It needs to be more trustworthy than this.
-Steve
On Thu, Jan 16, 2014 at 6:02 AM, Steve Grubb <[email protected]> wrote:
> On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote:
>> >> > Try this,
>> >> >
>> >> > cp /bin/ls 'test test test'
>> >> > auditctll -a always,exit -F arch=b64 -S stat -k test
>> >> > ./test\ test\ test './test\ test\ test'
>> >> > auditctl -D
>> >> > ausearch --start recent --key test
>> >> >
>> >> >> On the event of weird chars, it gets hex escaped.
>> >> >
>> >> > and its all in 1 lump with no escaping to figure out what is what.
>> >>
>> >> Un-escape it. ausearch does this with paths. Then if you need to parse
>> >> it, do it.
>> >
>> > How can you? When you unescape cmdline for the example I gave, you will
>> > have "./test test test ./test test test". Which program ran and how many
>> > arguments were passed? If we are trying to improve on what comm= provides
>> > by having the full information, I have to be able to find out exactly
>> > what the program name was so it can be used for searching. If that can't
>> > be done, then we don't need this addition in its current form.
>>
>> In your example, you will have an execve record, with it parsed, will you
>> not?
>
> Only if you change your patch.
My patch has nothing to do with the emitting of an execve record. You
will get an
execve record with the arguments parsed out. Its not even really
"parsing" as each
element is in a NULL terminated char * array.
>
>
>> cmdline does not necessarily represent the arguments or process name.
>> Sometimes it does, sometimes it doesn't. Just treat the thing as one
>> string, perhaps do some form of substring matching in a tool.
>
> You are missing the point. The point is that you are trying to place trust in
> something that can be gamed. The audit system is designed such that it cannot
> be fooled very easily. Each piece of the subject and object are separated so
> that programs can be written to analyze events. What I am trying to say is now
> you are making something that concatenates fields with no way to regroup them
> later to reconstruct what really happened,
>
>
>> To make this clear, I am not trying to improve on what comm provides.
>> comm provides
>> 16 chars for per thread name. The key is, its per thread, and can be
>> anything. The
>> "cmdline" value, is an arbitrary spot that is a global entity for the
>> process. So in my change, all things coming into these events will have a
>> similar cmdline audit. Which may help in narrowing down on whats going on
>> in the system
>
> It needs to be more trustworthy than this.
Its as trustworthy as comm, its as trustworthy as path, etc. The audit
subsystem already prints many
untrusted values to aid in narrowing down the process, or to observe a
running processes behavior; this
is no different.
On Thursday, January 16, 2014 07:03:34 AM William Roberts wrote:
> On Thu, Jan 16, 2014 at 6:02 AM, Steve Grubb <[email protected]> wrote:
> > On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote:
> >> >> > Try this,
> >> >> >
> >> >> > cp /bin/ls 'test test test'
> >> >> > auditctll -a always,exit -F arch=b64 -S stat -k test
> >> >> > ./test\ test\ test './test\ test\ test'
> >> >> > auditctl -D
> >> >> > ausearch --start recent --key test
> >> >> >
> >> >> >> On the event of weird chars, it gets hex escaped.
> >> >> >
> >> >> > and its all in 1 lump with no escaping to figure out what is what.
> >> >>
> >> >> Un-escape it. ausearch does this with paths. Then if you need to parse
> >> >> it, do it.
> >> >
> >> > How can you? When you unescape cmdline for the example I gave, you will
> >> > have "./test test test ./test test test". Which program ran and how
> >> > many
> >> > arguments were passed? If we are trying to improve on what comm=
> >> > provides
> >> > by having the full information, I have to be able to find out exactly
> >> > what the program name was so it can be used for searching. If that
> >> > can't
> >> > be done, then we don't need this addition in its current form.
> >>
> >> In your example, you will have an execve record, with it parsed, will you
> >> not?
> >
> > Only if you change your patch.
>
> My patch has nothing to do with the emitting of an execve record. You
> will get an
> execve record with the arguments parsed out. Its not even really
> "parsing" as each
> element is in a NULL terminated char * array.
That is what I am telling you is wrong. We can't have a string that can't be
parsed later. If you reformat the output as an execve record, then we have
something that is trustworthy.
> >> cmdline does not necessarily represent the arguments or process name.
> >> Sometimes it does, sometimes it doesn't. Just treat the thing as one
> >> string, perhaps do some form of substring matching in a tool.
> >
> > You are missing the point. The point is that you are trying to place trust
> > in something that can be gamed. The audit system is designed such that it
> > cannot be fooled very easily. Each piece of the subject and object are
> > separated so that programs can be written to analyze events. What I am
> > trying to say is now you are making something that concatenates fields
> > with no way to regroup them later to reconstruct what really happened,
> >
> >> To make this clear, I am not trying to improve on what comm provides.
> >> comm provides
> >> 16 chars for per thread name. The key is, its per thread, and can be
> >> anything. The
> >> "cmdline" value, is an arbitrary spot that is a global entity for the
> >> process. So in my change, all things coming into these events will have a
> >> similar cmdline audit. Which may help in narrowing down on whats going on
> >> in the system
> >
> > It needs to be more trustworthy than this.
>
> Its as trustworthy as comm, its as trustworthy as path, etc. The audit
> subsystem already prints many
> untrusted values to aid in narrowing down the process, or to observe a
> running processes behavior; this
> is no different.
Sure it is. comm is 1 entity on the value side of the name value pair. If it
is detected as being special, its encoded so that it can be correctly
dissected later. What you are creating cannot be correctly dissected. Please
try the example I gave you and think about it a bit.
Thanks,
-Steve
On Thu, Jan 16, 2014 at 8:40 AM, William Roberts
<[email protected]> wrote:
> On Thu, Jan 16, 2014 at 7:11 AM, Steve Grubb <[email protected]> wrote:
>> On Thursday, January 16, 2014 07:03:34 AM William Roberts wrote:
>>> On Thu, Jan 16, 2014 at 6:02 AM, Steve Grubb <[email protected]> wrote:
>>> > On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote:
>>> >> >> > Try this,
>>> >> >> >
>>> >> >> > cp /bin/ls 'test test test'
>>> >> >> > auditctll -a always,exit -F arch=b64 -S stat -k test
>>> >> >> > ./test\ test\ test './test\ test\ test'
>>> >> >> > auditctl -D
>>> >> >> > ausearch --start recent --key test
>>> >> >> >
>>> >> >> >> On the event of weird chars, it gets hex escaped.
>>> >> >> >
>>> >> >> > and its all in 1 lump with no escaping to figure out what is what.
>>> >> >>
>>> >> >> Un-escape it. ausearch does this with paths. Then if you need to parse
>>> >> >> it, do it.
>>> >> >
>>> >> > How can you? When you unescape cmdline for the example I gave, you will
>>> >> > have "./test test test ./test test test". Which program ran and how
>>> >> > many
>>> >> > arguments were passed? If we are trying to improve on what comm=
>>> >> > provides
>>> >> > by having the full information, I have to be able to find out exactly
>>> >> > what the program name was so it can be used for searching. If that
>>> >> > can't
>>> >> > be done, then we don't need this addition in its current form.
>>> >>
>>> >> In your example, you will have an execve record, with it parsed, will you
>>> >> not?
>>> >
>>> > Only if you change your patch.
>>>
>>> My patch has nothing to do with the emitting of an execve record. You
>>> will get an
>>> execve record with the arguments parsed out. Its not even really
>>> "parsing" as each
>>> element is in a NULL terminated char * array.
>>
>> That is what I am telling you is wrong. We can't have a string that can't be
>> parsed later. If you reformat the output as an execve record, then we have
>> something that is trustworthy.
>
Formatting of the string does not change the trust worthiness of its value. If
I told you your name was JoEy, I could re-type it like this Joey and
its still not
true. I think the part of this your confusing is that this is really
proctitle. On desktops
the execution environment simply takes all the cmdline args and dumps
it there. But,
this is not always the case. The reason your arguments and command are
well formatted
on exec is becuase of the interface to the kernel. Its in a char *
array. Their is NO parsing
that is done on the kernels behalf. Accessing each value is just a
matter of indexing the array.
Parsing "cmdline", which again is arbitrary and controlled via
setproctitle() is not needed,
because its arbitrary. However, the fact that its arbitrary does not
affect its usefulness. Many
values used in the audit records, like comm (prctl()), can be
manipulated by executing programs. But it
may help a user to deduce what is going on.
>>
>>
>>> >> cmdline does not necessarily represent the arguments or process name.
>>> >> Sometimes it does, sometimes it doesn't. Just treat the thing as one
>>> >> string, perhaps do some form of substring matching in a tool.
>>> >
>>> > You are missing the point. The point is that you are trying to place trust
>>> > in something that can be gamed. The audit system is designed such that it
>>> > cannot be fooled very easily. Each piece of the subject and object are
>>> > separated so that programs can be written to analyze events. What I am
>>> > trying to say is now you are making something that concatenates fields
>>> > with no way to regroup them later to reconstruct what really happened,
>>> >
>>> >> To make this clear, I am not trying to improve on what comm provides.
>>> >> comm provides
>>> >> 16 chars for per thread name. The key is, its per thread, and can be
>>> >> anything. The
>>> >> "cmdline" value, is an arbitrary spot that is a global entity for the
>>> >> process. So in my change, all things coming into these events will have a
>>> >> similar cmdline audit. Which may help in narrowing down on whats going on
>>> >> in the system
>>> >
>>> > It needs to be more trustworthy than this.
>>>
>>> Its as trustworthy as comm, its as trustworthy as path, etc. The audit
>>> subsystem already prints many
>>> untrusted values to aid in narrowing down the process, or to observe a
>>> running processes behavior; this
>>> is no different.
>>
>> Sure it is. comm is 1 entity on the value side of the name value pair. If it
>> is detected as being special, its encoded so that it can be correctly
>> dissected later. What you are creating cannot be correctly dissected. Please
>> try the example I gave you and think about it a bit.
>
Your example BTW, can be parsed. I have an FSM on my
whiteboard that will do it. But you don't need to parse it. Again,
as I stated above it might not be set to your example. If you have
a process that does this:
prctl(PR_SET_NAME, "./test \test \test");
You will see that comm now has white space and other chars.
I think to help avoid this confusion, I will change the keyword from
cmdline= to proctitle=
re-adding mailing list, accidentally dropped.