[PATCH] IPC_SET_PERM cleanup

[PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

The following patch addresses most of the issues with the IPC_SET_PERM
records as described in:
https://www.redhat.com/archives/linux-audit/2006-May/msg00010.html

To summarize, I made the following changes:

1. Changed sys_msgctl() and semctl_down() so that an IPC_SET_PERM
   record is emitted in the failure case as well as the success case.
   This matches the behavior in sys_shmctl().  I could simplify the
   code in sys_msgctl() and semctl_down() slightly but it would mean
   that in some error cases we could get an IPC_SET_PERM record
   without an IPC record and that seemed odd.

2. No change to the IPC record type, given no feedback on the backward
   compatibility question.

3. Removed the qbytes field from the IPC record.  It wasn't being
   set and when audit_ipc_obj() is called from ipcperms(), the
   information isn't available.  If we want the information in the IPC
   record, more extensive changes will be necessary.  Since it only
   applies to message queues and it isn't really permission related, it
   doesn't seem worth it.

4. Removed the obj field from the IPC_SET_PERM record.  This means that
   the kern_ipc_perm argument is no longer needed.

5. Replaced the spaces in the IPC_SET_PERM field names with underscores.

I tested this with the lspp.22 kernel on an x86_64 box.  Please let me
know if you see any issues.

-- ljk

 include/linux/audit.h |    2 +-
 ipc/msg.c             |    9 +++++----
 ipc/sem.c             |    8 +++++---
 ipc/shm.c             |    2 +-
 kernel/auditsc.c      |   22 +++++-----------------
 5 files changed, 17 insertions(+), 26 deletions(-)

--- linux-2.6.16.x86_64.orig/kernel/auditsc.c   2006-05-05 14:29:42.000000000 -0400
+++ linux-2.6.16.x86_64/kernel/auditsc.c        2006-05-05 14:48:44.000000000 -0400
@@ -665,8 +665,8 @@ static void audit_log_exit(struct audit_
                case AUDIT_IPC: {
                        struct audit_aux_data_ipcctl *axi = (void *)aux;
                        audit_log_format(ab,
-                                " qbytes=%lx iuid=%u igid=%u mode=%x",
-                                axi->qbytes, axi->uid, axi->gid, axi->mode);
+                                "iuid=%u igid=%u mode=%x",
+                                axi->uid, axi->gid, axi->mode);
                        if (axi->osid != 0) {
                                char *ctx = NULL;
                                u32 len;
@@ -684,21 +684,10 @@ static void audit_log_exit(struct audit_
                case AUDIT_IPC_SET_PERM: {
                        struct audit_aux_data_ipcctl *axi = (void *)aux;
                        audit_log_format(ab,
-                               " new qbytes=%lx new iuid=%u new igid=%u new mode=%x",
+                               "new_qbytes=%lx new_iuid=%u new_igid=%u new_mode=%x",
                                axi->qbytes, axi->uid, axi->gid, axi->mode);
-                       if (axi->osid != 0) {
-                               char *ctx = NULL;
-                               u32 len;
-                               if (selinux_ctxid_to_string(
-                                               axi->osid, &ctx, &len)) {
-                                       audit_log_format(ab, " osid=%u",
-                                                       axi->osid);
-                                       call_panic = 1;
-                               } else
-                                       audit_log_format(ab, " obj=%s", ctx);
-                               kfree(ctx);
-                       }
                        break; }
+
                case AUDIT_EXECVE: {
                        struct audit_aux_data_execve *axi = (void *)aux;
                        int i;
@@ -1232,7 +1221,7 @@ int audit_ipc_obj(struct kern_ipc_perm *
  *
  * Returns 0 for success or NULL context or < 0 on error.
  */
-int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode, struct kern_ipc_perm *ipcp)
+int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode)
 {
        struct audit_aux_data_ipcctl *ax;
        struct audit_context *context = current->audit_context;
@@ -1248,7 +1237,6 @@ int audit_ipc_set_perm(unsigned long qby
        ax->uid = uid;
        ax->gid = gid;
        ax->mode = mode;
-       selinux_get_ipc_sid(ipcp, &ax->osid);

        ax->d.type = AUDIT_IPC_SET_PERM;
        ax->d.next = context->aux;
--- linux-2.6.16.x86_64.orig/ipc/msg.c  2006-05-05 14:30:15.000000000 -0400
+++ linux-2.6.16.x86_64/ipc/msg.c       2006-05-05 14:50:32.000000000 -0400
@@ -454,6 +454,11 @@ asmlinkage long sys_msgctl (int msqid, i
        err = audit_ipc_obj(ipcp);
        if (err)
                goto out_unlock_up;
+       if (cmd==IPC_SET) {
+               err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid, setbuf.mode);
+               if (err)
+                       goto out_unlock_up;
+       }

        err = -EPERM;
        if (current->euid != ipcp->cuid &&
@@ -468,10 +473,6 @@ asmlinkage long sys_msgctl (int msqid, i
        switch (cmd) {
        case IPC_SET:
        {
-               err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid, setbuf.mode, ipcp);
-               if (err)
-                       goto out_unlock_up;
-
                err = -EPERM;
                if (setbuf.qbytes > msg_ctlmnb && !capable(CAP_SYS_RESOURCE))
                        goto out_unlock_up;
--- linux-2.6.16.x86_64.orig/ipc/shm.c  2006-05-05 15:08:23.000000000 -0400
+++ linux-2.6.16.x86_64/ipc/shm.c       2006-05-05 14:51:53.000000000 -0400
@@ -643,7 +643,7 @@ asmlinkage long sys_shmctl (int shmid, i
                err = audit_ipc_obj(&(shp->shm_perm));
                if (err)
                        goto out_unlock_up;
-               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode, &(shp->shm_perm));
+               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode);
                if (err)
                        goto out_unlock_up;
                err=-EPERM;
--- linux-2.6.16.x86_64.orig/ipc/sem.c  2006-05-05 14:30:02.000000000 -0400
+++ linux-2.6.16.x86_64/ipc/sem.c       2006-05-05 14:50:58.000000000 -0400
@@ -828,6 +828,11 @@ static int semctl_down(int semid, int se
        if (err)
                goto out_unlock;

+       if (cmd == IPC_SET) {
+               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode);
+               if (err)
+                       goto out_unlock;
+       }
        if (current->euid != ipcp->cuid &&
            current->euid != ipcp->uid && !capable(CAP_SYS_ADMIN)) {
                err=-EPERM;
@@ -844,9 +849,6 @@ static int semctl_down(int semid, int se
                err = 0;
                break;
        case IPC_SET:
-               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode, ipcp);
-               if (err)
-                       goto out_unlock;
                ipcp->uid = setbuf.uid;
                ipcp->gid = setbuf.gid;
                ipcp->mode = (ipcp->mode & ~S_IRWXUGO)
--- linux-2.6.16.x86_64.orig/include/linux/audit.h      2006-05-05 15:09:42.000000000 -0400
+++ linux-2.6.16.x86_64/include/linux/audit.h   2006-05-05 14:49:35.000000000 -0400
@@ -324,7 +324,7 @@ extern void auditsc_get_stamp(struct aud
 extern int  audit_set_loginuid(struct task_struct *task, uid_t loginuid);
 extern uid_t audit_get_loginuid(struct audit_context *ctx);
 extern int audit_ipc_obj(struct kern_ipc_perm *ipcp);
-extern int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode, struct kern_ipc_perm  *ipcp);
+extern int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode);
 extern int audit_bprm(struct linux_binprm *bprm);
 extern int audit_socketcall(int nargs, unsigned long *args);
 extern int audit_sockaddr(int len, void *addr);

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Friday 05 May 2006 16:19, Linda Knippers wrote:
> -                       if (axi->osid != 0) {
> -                               char *ctx = NULL;
> -                               u32 len;
> -                               if (selinux_ctxid_to_string(
> -                                               axi->osid, &ctx, &len)) {
> -                                       audit_log_format(ab, " osid=%u",
> -                                                       axi->osid);
> -                                       call_panic = 1;
> -                               } else
> -                                       audit_log_format(ab, " obj=%s",
> ctx); -                               kfree(ctx);
> -                       }

This patch deletes the context string out of this record. Are we losing 
anything important?

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

Steve Grubb wrote:
> On Friday 05 May 2006 16:19, Linda Knippers wrote:
> 
>>-                       if (axi->osid != 0) {
>>-                               char *ctx = NULL;
>>-                               u32 len;
>>-                               if (selinux_ctxid_to_string(
>>-                                               axi->osid, &ctx, &len)) {
>>-                                       audit_log_format(ab, " osid=%u",
>>-                                                       axi->osid);
>>-                                       call_panic = 1;
>>-                               } else
>>-                                       audit_log_format(ab, " obj=%s",
>>ctx); -                               kfree(ctx);
>>-                       }
> 
> 
> This patch deletes the context string out of this record. Are we losing 
> anything important?

I don't think so.  I don't think the IPC_SET operations change the sid
(at least I don't see it in the code) so its redundant with the obj information
that's in the IPC record.  If I'm missing it, I hope someone will point
it out to me.

If an IPC_SET can change the sid, then we'll have to move all the calls
to audit_ipc_set_perm() so that we get the new obj information in the
success case and don't lose the entire record in the failure case.

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Fri, May 05, 2006 at 04:59:49PM -0400, Linda Knippers wrote:
> Steve Grubb wrote:
> > This patch deletes the context string out of this record. Are we losing 
> > anything important?
> 
> I don't think so.  I don't think the IPC_SET operations change the sid
> (at least I don't see it in the code) so its redundant with the obj information
> that's in the IPC record.  If I'm missing it, I hope someone will point
> it out to me.

I agree that it's redundant. This is the sub-record that describes the
requested changes to object properties, and any information that can't be
changed through IPC_SET doesn't need to be in it. The information about
the current state of the object and subject is in separate sub-records.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

In case anyone is interested, this is what the audit records
for an semctl IPC_SET look like with my patch.

Success case:
type=SYSCALL msg=audit(1146863632.117:98): arch=c000003e syscall=66
success=yes exit=0 a0=10000 a1=1 a2=1 a3=7fff2811c6f0 items=0 ppid=4355
pid=4367 auid=500 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
tty=pts1 comm="syscalls"
exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls"
subj=user_u:system_r:unconfined_t:s0-s0:c0.c255
type=IPC_SET_PERM msg=audit(1146863632.117:98): new_qbytes=0 new_iuid=501
new_igid=0 new_mode=0
type=IPC msg=audit(1146863632.117:98): iuid=0 igid=0 mode=1c0
obj=user_u:system_r:unconfined_t:s0-s0:c0.c255

Failure case:
type=SYSCALL msg=audit(1146863632.369:102): arch=c000003e syscall=66
success=no exit=-1 a0=18000 a1=1 a2=1 a3=7fff11ca6280 items=0 ppid=4377
pid=4388 auid=500 uid=0 gid=0 euid=501 suid=0 fsuid=501 egid=0 sgid=0 fsgid=0
tty=pts1 comm="syscalls"
exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls"
subj=user_u:system_r:unconfined_t:s0-s0:c0.c255
type=IPC_SET_PERM msg=audit(1146863632.369:102): new_qbytes=0 new_iuid=501
new_igid=0 new_mode=0
type=IPC msg=audit(1146863632.369:102): iuid=0 igid=0 mode=1c0
obj=user_u:system_r:unconfined_t:s0-s0:c0.c255

With the .22 kernel, they look like this:

Success case:
type=SYSCALL msg=audit(1146691872.791:94): arch=c000003e syscall=66
success=yes exit=0 a0=10000 a1=1 a2=1 a3=7fff328a7e70 items=0 pid=4327
auid=500 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
tty=pts2 comm="syscalls"
exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls"
subj=user_u:system_r:unconfined_t:s0-s0:c0.c255
type=IPC_SET_PERM msg=audit(1146691872.791:94):  new qbytes=0 new
iuid=501 new igid=0 new mode=0
obj=user_u:system_r:unconfined_t:s0-s0:c0.c255
type=IPC msg=audit(1146691872.791:94):  qbytes=5a5a5a5a5a5a5a5a iuid=0
igid=0 mode=1c0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255

Failure case:
type=SYSCALL msg=audit(1146691112.828:64): arch=c000003e syscall=66
success=no exit=-1 a0=8000 a1=1 a2=1 a3=7fff96610bd0 items=0 pid=3101
auid=500 uid=0 gid=0 euid=501 suid=0 fsuid=501 egid=0 sgid=0 fsgid=0
tty=pts2 comm="syscalls"
exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls"
subj=user_u:system_r:unconfined_t:s0-s0:c0.c255
type=IPC msg=audit(1146691112.828:64):  qbytes=5a5a5a5a5a5a5a5a iuid=0
igid=0 mode=1c0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Dustin Kirkland]

On 5/5/06, Linda Knippers <linda.knippers@hp.com> wrote:
> The following patch addresses most of the issues with the IPC_SET_PERM
> records as described in:
> https://www.redhat.com/archives/linux-audit/2006-May/msg00010.html

Hi Linda-

I apologize for the delay in my response.  I'm pretty much permanently
away from Audit-related work, and just now got a chance to respond to
this.

First, let me point you to a thread wherein I explained why I made the
audit ipc changes that I did.  It starts here:
https://www.redhat.com/archives/linux-audit/2006-March/msg00088.html

That said, thanks for testing some of this out more thoroughly and
posting your findings.

> To summarize, I made the following changes:
>
> 1. Changed sys_msgctl() and semctl_down() so that an IPC_SET_PERM
>    record is emitted in the failure case as well as the success case.
>    This matches the behavior in sys_shmctl().  I could simplify the
>    code in sys_msgctl() and semctl_down() slightly but it would mean
>    that in some error cases we could get an IPC_SET_PERM record
>    without an IPC record and that seemed odd.
>
> 2. No change to the IPC record type, given no feedback on the backward
>    compatibility question.
>
> 3. Removed the qbytes field from the IPC record.  It wasn't being
>    set and when audit_ipc_obj() is called from ipcperms(), the
>    information isn't available.  If we want the information in the IPC
>    record, more extensive changes will be necessary.  Since it only
>    applies to message queues and it isn't really permission related, it
>    doesn't seem worth it.
>
> 4. Removed the obj field from the IPC_SET_PERM record.  This means that
>    the kern_ipc_perm argument is no longer needed.
>
> 5. Replaced the spaces in the IPC_SET_PERM field names with underscores.

Thanks, that was my oversight.  There should definitely be underscores
rather than spaces.

> I tested this with the lspp.22 kernel on an x86_64 box.  Please let me
> know if you see any issues.
>
> -- ljk
>
>  include/linux/audit.h |    2 +-
>  ipc/msg.c             |    9 +++++----
>  ipc/sem.c             |    8 +++++---
>  ipc/shm.c             |    2 +-
>  kernel/auditsc.c      |   22 +++++-----------------
>  5 files changed, 17 insertions(+), 26 deletions(-)
>
> --- linux-2.6.16.x86_64.orig/kernel/auditsc.c   2006-05-05 14:29:42.000000000 -0400
> +++ linux-2.6.16.x86_64/kernel/auditsc.c        2006-05-05 14:48:44.000000000 -0400
> @@ -665,8 +665,8 @@ static void audit_log_exit(struct audit_
>                 case AUDIT_IPC: {
>                         struct audit_aux_data_ipcctl *axi = (void *)aux;
>                         audit_log_format(ab,
> -                                " qbytes=%lx iuid=%u igid=%u mode=%x",
> -                                axi->qbytes, axi->uid, axi->gid, axi->mode);
> +                                "iuid=%u igid=%u mode=%x",
> +                                axi->uid, axi->gid, axi->mode);
>                         if (axi->osid != 0) {
>                                 char *ctx = NULL;
>                                 u32 len;
> @@ -684,21 +684,10 @@ static void audit_log_exit(struct audit_
>                 case AUDIT_IPC_SET_PERM: {
>                         struct audit_aux_data_ipcctl *axi = (void *)aux;
>                         audit_log_format(ab,
> -                               " new qbytes=%lx new iuid=%u new igid=%u new mode=%x",
> +                               "new_qbytes=%lx new_iuid=%u new_igid=%u new_mode=%x",
>                                 axi->qbytes, axi->uid, axi->gid, axi->mode);
> -                       if (axi->osid != 0) {
> -                               char *ctx = NULL;
> -                               u32 len;
> -                               if (selinux_ctxid_to_string(
> -                                               axi->osid, &ctx, &len)) {
> -                                       audit_log_format(ab, " osid=%u",
> -                                                       axi->osid);
> -                                       call_panic = 1;
> -                               } else
> -                                       audit_log_format(ab, " obj=%s", ctx);
> -                               kfree(ctx);
> -                       }
>                         break; }
> +
>                 case AUDIT_EXECVE: {
>                         struct audit_aux_data_execve *axi = (void *)aux;
>                         int i;
> @@ -1232,7 +1221,7 @@ int audit_ipc_obj(struct kern_ipc_perm *
>   *
>   * Returns 0 for success or NULL context or < 0 on error.
>   */
> -int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode, struct kern_ipc_perm *ipcp)
> +int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode)
>  {
>         struct audit_aux_data_ipcctl *ax;
>         struct audit_context *context = current->audit_context;
> @@ -1248,7 +1237,6 @@ int audit_ipc_set_perm(unsigned long qby
>         ax->uid = uid;
>         ax->gid = gid;
>         ax->mode = mode;
> -       selinux_get_ipc_sid(ipcp, &ax->osid);
>
>         ax->d.type = AUDIT_IPC_SET_PERM;
>         ax->d.next = context->aux;
> --- linux-2.6.16.x86_64.orig/ipc/msg.c  2006-05-05 14:30:15.000000000 -0400
> +++ linux-2.6.16.x86_64/ipc/msg.c       2006-05-05 14:50:32.000000000 -0400
> @@ -454,6 +454,11 @@ asmlinkage long sys_msgctl (int msqid, i
>         err = audit_ipc_obj(ipcp);
>         if (err)
>                 goto out_unlock_up;
> +       if (cmd==IPC_SET) {
> +               err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid, setbuf.mode);
> +               if (err)
> +                       goto out_unlock_up;
> +       }
>
>         err = -EPERM;
>         if (current->euid != ipcp->cuid &&
> @@ -468,10 +473,6 @@ asmlinkage long sys_msgctl (int msqid, i
>         switch (cmd) {
>         case IPC_SET:
>         {
> -               err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid, setbuf.mode, ipcp);
> -               if (err)
> -                       goto out_unlock_up;
> -
>                 err = -EPERM;
>                 if (setbuf.qbytes > msg_ctlmnb && !capable(CAP_SYS_RESOURCE))
>                         goto out_unlock_up;
> --- linux-2.6.16.x86_64.orig/ipc/shm.c  2006-05-05 15:08:23.000000000 -0400
> +++ linux-2.6.16.x86_64/ipc/shm.c       2006-05-05 14:51:53.000000000 -0400
> @@ -643,7 +643,7 @@ asmlinkage long sys_shmctl (int shmid, i
>                 err = audit_ipc_obj(&(shp->shm_perm));
>                 if (err)
>                         goto out_unlock_up;
> -               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode, &(shp->shm_perm));
> +               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode);
>                 if (err)
>                         goto out_unlock_up;
>                 err=-EPERM;
> --- linux-2.6.16.x86_64.orig/ipc/sem.c  2006-05-05 14:30:02.000000000 -0400
> +++ linux-2.6.16.x86_64/ipc/sem.c       2006-05-05 14:50:58.000000000 -0400
> @@ -828,6 +828,11 @@ static int semctl_down(int semid, int se
>         if (err)
>                 goto out_unlock;
>
> +       if (cmd == IPC_SET) {
> +               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode);
> +               if (err)
> +                       goto out_unlock;
> +       }
>         if (current->euid != ipcp->cuid &&
>             current->euid != ipcp->uid && !capable(CAP_SYS_ADMIN)) {
>                 err=-EPERM;
> @@ -844,9 +849,6 @@ static int semctl_down(int semid, int se
>                 err = 0;
>                 break;
>         case IPC_SET:
> -               err = audit_ipc_set_perm(0, setbuf.uid, setbuf.gid, setbuf.mode, ipcp);
> -               if (err)
> -                       goto out_unlock;
>                 ipcp->uid = setbuf.uid;
>                 ipcp->gid = setbuf.gid;
>                 ipcp->mode = (ipcp->mode & ~S_IRWXUGO)
> --- linux-2.6.16.x86_64.orig/include/linux/audit.h      2006-05-05 15:09:42.000000000 -0400
> +++ linux-2.6.16.x86_64/include/linux/audit.h   2006-05-05 14:49:35.000000000 -0400
> @@ -324,7 +324,7 @@ extern void auditsc_get_stamp(struct aud
>  extern int  audit_set_loginuid(struct task_struct *task, uid_t loginuid);
>  extern uid_t audit_get_loginuid(struct audit_context *ctx);
>  extern int audit_ipc_obj(struct kern_ipc_perm *ipcp);
> -extern int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode, struct kern_ipc_perm  *ipcp);
> +extern int audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode);
>  extern int audit_bprm(struct linux_binprm *bprm);
>  extern int audit_socketcall(int nargs, unsigned long *args);
>  extern int audit_sockaddr(int len, void *addr);
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>

Re: [PATCH] IPC_SET_PERM cleanup

[Dustin Kirkland]

On 5/5/06, Linda Knippers <linda.knippers@hp.com> wrote:
> The following patch addresses most of the issues with the IPC_SET_PERM
> records as described in:
> https://www.redhat.com/archives/linux-audit/2006-May/msg00010.html

Hi Linda-

I apologize for the delay in my response.  I'm pretty much permanently
away from Audit-related work, and just now got a chance to respond to
this.

First, let me point you to a thread wherein I explained why I made the
audit ipc changes that I did (in case you missed it the first time
around).  It starts here:
https://www.redhat.com/archives/linux-audit/2006-March/msg00088.html

That said, thanks for testing some of this out more thoroughly and
posting your findings.

> To summarize, I made the following changes:
>
> 1. Changed sys_msgctl() and semctl_down() so that an IPC_SET_PERM
>    record is emitted in the failure case as well as the success case.
>    This matches the behavior in sys_shmctl().  I could simplify the
>    code in sys_msgctl() and semctl_down() slightly but it would mean
>    that in some error cases we could get an IPC_SET_PERM record
>    without an IPC record and that seemed odd.

I think this is ok.

> 2. No change to the IPC record type, given no feedback on the backward
>    compatibility question.

I'm not one to speak authoritatively about compatibility issues... 
But I do prefer the more descriptive AUDIT_IPC_SET_PERM type, as it
more accurately explains what the record is.  Someone might complain
at some point about changing the record type, but there will be
considerably more invasive API changes elsewhere between the 2.6.5 and
the 2.6.16 kernels (and the RHEL4/RHEL5 kernels).

> 3. Removed the qbytes field from the IPC record.  It wasn't being
>    set and when audit_ipc_obj() is called from ipcperms(), the
>    information isn't available.  If we want the information in the IPC
>    record, more extensive changes will be necessary.  Since it only
>    applies to message queues and it isn't really permission related, it
>    doesn't seem worth it.

I agree with you here.  However, I resisted the urge to remove the
qbytes field when I reworked the ipc audit code as I did not know
__why__ this was being saved.  I assumed this was required by CAPP for
one reason or another.  I personally don't find it a very interesting
field to capture.  But I encourage you to review this with Klaus (or
another of the CAPP/LSPP experts).

> 4. Removed the obj field from the IPC_SET_PERM record.  This means that
>    the kern_ipc_perm argument is no longer needed.

Hmm...  This is probably ok, as long as you can __guarantee__ that an
IPC record will always closely follow an IPC_SET_PERM (and can be
associated together).  The object label is needed for LSPP.  If that
can be found in an associated record, so be it.  Looking at your
example results, it seems ok.

> 5. Replaced the spaces in the IPC_SET_PERM field names with underscores.

Thanks, that was my oversight.  There should definitely be underscores
rather than spaces.

:-Dustin

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

Hi Dustin,

Thanks for the comments.

Dustin Kirkland wrote:
> On 5/5/06, Linda Knippers <linda.knippers@hp.com> wrote:
> 
>> The following patch addresses most of the issues with the IPC_SET_PERM
>> records as described in:
>> https://www.redhat.com/archives/linux-audit/2006-May/msg00010.html
> 
> 
> Hi Linda-
> 
> I apologize for the delay in my response.  I'm pretty much permanently
> away from Audit-related work, and just now got a chance to respond to
> this.
> 
> First, let me point you to a thread wherein I explained why I made the
> audit ipc changes that I did (in case you missed it the first time
> around).  It starts here:
> https://www.redhat.com/archives/linux-audit/2006-March/msg00088.html
> 
> That said, thanks for testing some of this out more thoroughly and
> posting your findings.

Thanks, I did find that mail as I was looking through the changes and it was
helpful.
> 
>> To summarize, I made the following changes:
>>
>> 1. Changed sys_msgctl() and semctl_down() so that an IPC_SET_PERM
>>    record is emitted in the failure case as well as the success case.
>>    This matches the behavior in sys_shmctl().  I could simplify the
>>    code in sys_msgctl() and semctl_down() slightly but it would mean
>>    that in some error cases we could get an IPC_SET_PERM record
>>    without an IPC record and that seemed odd.
> 
> 
> I think this is ok.
> 
>> 2. No change to the IPC record type, given no feedback on the backward
>>    compatibility question.
> 
> 
> I'm not one to speak authoritatively about compatibility issues... But I
> do prefer the more descriptive AUDIT_IPC_SET_PERM type, as it
> more accurately explains what the record is.  Someone might complain
> at some point about changing the record type, but there will be
> considerably more invasive API changes elsewhere between the 2.6.5 and
> the 2.6.16 kernels (and the RHEL4/RHEL5 kernels).
> 
>> 3. Removed the qbytes field from the IPC record.  It wasn't being
>>    set and when audit_ipc_obj() is called from ipcperms(), the
>>    information isn't available.  If we want the information in the IPC
>>    record, more extensive changes will be necessary.  Since it only
>>    applies to message queues and it isn't really permission related, it
>>    doesn't seem worth it.
> 
> 
> I agree with you here.  However, I resisted the urge to remove the
> qbytes field when I reworked the ipc audit code as I did not know
> __why__ this was being saved.  I assumed this was required by CAPP for
> one reason or another.  I personally don't find it a very interesting
> field to capture.  But I encourage you to review this with Klaus (or
> another of the CAPP/LSPP experts).

I left it in the AUDIT_IPC_SET_PERM record but removed it from the IPC
record.  The reason I can see for having it in the AUDIT_IPC_SET_PERM
record is that depending on the user and what the user is trying to set it to,
it can cause an EPERM.  In that case it might be helpful to see the value
the users was attempting to set it to in the AUDIT_IPC_SET_PERM record, but
the current value seems less interesting.  For CAPP, we only recorded the
new values.  They were captured in the IPC record. In any case, I have asked
for our evaluator's opinion as well.

> 
>> 4. Removed the obj field from the IPC_SET_PERM record.  This means that
>>    the kern_ipc_perm argument is no longer needed.
> 
> 
> Hmm...  This is probably ok, as long as you can __guarantee__ that an
> IPC record will always closely follow an IPC_SET_PERM (and can be
> associated together).  The object label is needed for LSPP.  If that
> can be found in an associated record, so be it.  Looking at your
> example results, it seems ok.

I believe that's true.  I could have cleaned up the code slightly but that
would have introduced cases where we could get an IPC_SET_PERM without
the IPC record and I wanted to avoid that.
> 
>> 5. Replaced the spaces in the IPC_SET_PERM field names with underscores.
> 
> 
> Thanks, that was my oversight.  There should definitely be underscores
> rather than spaces.

I believe an early iteration of your patch did have underscores but somehow
they got turned into spaces during some rework.

Thanks again for the comments.

-- ljk
> 
> :-Dustin

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Mon, May 08, 2006 at 01:29:57PM -0500, Dustin Kirkland wrote:
> On 5/5/06, Linda Knippers <linda.knippers@hp.com> wrote:
> >2. No change to the IPC record type, given no feedback on the backward
> >   compatibility question.
> 
> I'm not one to speak authoritatively about compatibility issues... 
> But I do prefer the more descriptive AUDIT_IPC_SET_PERM type, as it
> more accurately explains what the record is.  Someone might complain
> at some point about changing the record type, but there will be
> considerably more invasive API changes elsewhere between the 2.6.5 and
> the 2.6.16 kernels (and the RHEL4/RHEL5 kernels).

I think having a separate record type would be an alternative to the
"new_" (or "new ") prefix. If the record types are distinct, it would be
possible to use the same field names for current and requested object
properties. 

The most important thing would be that there are sane rules how the
records should be parsed. The dangling "new " is only okay if there's a
well defined mechanism for field modifiers or attributes, but currently
that seems to be completely ad hoc.

> >3. Removed the qbytes field from the IPC record.  It wasn't being
> >   set and when audit_ipc_obj() is called from ipcperms(), the
> >   information isn't available.  If we want the information in the IPC
> >   record, more extensive changes will be necessary.  Since it only
> >   applies to message queues and it isn't really permission related, it
> >   doesn't seem worth it.
> 
> I agree with you here.  However, I resisted the urge to remove the
> qbytes field when I reworked the ipc audit code as I did not know
> __why__ this was being saved.  I assumed this was required by CAPP for
> one reason or another.  I personally don't find it a very interesting
> field to capture.  But I encourage you to review this with Klaus (or
> another of the CAPP/LSPP experts).

I personally don't care about the qbytes field since it's not security
relevant. I agree with Linda's argument from the followup that it makes
sense to audit it where a bad value may cause the call to fail.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Tuesday 09 May 2006 10:59, Klaus Weidner wrote:
> I think having a separate record type would be an alternative to the
> "new_" (or "new ") prefix. If the record types are distinct, it would be
> possible to use the same field names for current and requested object
> properties.

I'd rather see the word "new" separated by a space so that searching is 
better/easier.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

> I'd rather see the word "new" separated by a space so that searching is 
> better/easier.

How it is easier/better?  It was very easy for me to change our test
case to look for new_* and would have been alot harder with the space.

I thought we had a discussion in the LSPP call and on the list about
spaces vs. underscores?

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Tuesday 09 May 2006 11:12, Linda Knippers wrote:
> How it is easier/better?

Much easier for the user. For example, the user might be searching for uid. 
Should the user have to specify new_uid ? Would they also need to search for 
old_uid? Would they have to know all the possible variations on uid?

> I thought we had a discussion in the LSPP call and on the list about
> spaces vs. underscores?

We had this discussion on the audit list back in March while discussing the 
Audit Parsing Library.

https://www.redhat.com/archives/linux-audit/2006-March/msg00186.html

I thought it was settled at that time. If this was brought up on the LSPP 
telecon I missed it.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

>>How it is easier/better?
> 
> Much easier for the user. For example, the user might be searching for uid. 
> Should the user have to specify new_uid ? Would they also need to search for 
> old_uid? Would they have to know all the possible variations on uid?

If someone is looking for the records for a particular uid, wouldn't
they expect to get the records generated by someone with that uid?
Picking up something with the new_uid wouldn't be correct in my opinion
since its data relating, really an argument for the syscall, not related
to who caused the audit record to be emitted.

> We had this discussion on the audit list back in March while discussing the 
> Audit Parsing Library.
> 
> https://www.redhat.com/archives/linux-audit/2006-March/msg00186.html
> 
> I thought it was settled at that time. If this was brought up on the LSPP 
> telecon I missed it.

It didn't seem setttled, although you were the last to reply.  I think
the discussion on the LSPP list is what initiated the mail exchange.

> Might make it tougher too. For example, suppose someone wanted to find auid = 
> 520. The user space object manager, dbus, has a auid of -1, and is not what 
> we want. The sender auid is what we want. So, by having a space, we still 
> match the sender auid = 520. The word to the left is to give context to a 
> human reading the raw record, but the search utility should still match the 
> auid value even if it has sender before it. If we combine field names, I 
> think I'll have to maintain a table of field names to types so that ausearch 
> can make sense of the field's contents no matter what the name is.

The auid is an interesting case where the auid in the record isn't very
interesting and we're relying on something in the user message.  I think
it still could be interesting to distinguish between the two types of
auid.

At this point there are already a bunch of uid fields (auid, uid, euid,
suid, fsuid, iuid, ouid) in various audit records, and a similar set
of guid files, so would you be happier with nuid, ngid, etc?

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Tuesday 09 May 2006 11:34, Linda Knippers wrote:
> If someone is looking for the records for a particular uid, wouldn't
> they expect to get the records generated by someone with that uid?

Not necessarily. I would like to present all matches of uid and let them 
decide what is relavent.

> > I thought it was settled at that time. If this was brought up on the LSPP
> > telecon I missed it.
>
> It didn't seem setttled, although you were the last to reply.  I think
> the discussion on the LSPP list is what initiated the mail exchange.

I even updated the audit parsing specs to include all keywords:
http://people.redhat.com/sgrubb/audit/audit-parse.txt

> At this point there are already a bunch of uid fields (auid, uid, euid,
> suid, fsuid, iuid, ouid) in various audit records, and a similar set
> of guid files, so would you be happier with nuid, ngid, etc?

Does ouid and ogid not fit? I'd like us to define what we need in the parser 
API and then use it in the audit messages. Ancilliary words like new, old, 
last, first should not be tied with an underscore. If you find any, let me 
know.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Tue, May 09, 2006 at 11:55:34AM -0400, Steve Grubb wrote:
> I even updated the audit parsing specs to include all keywords:
> http://people.redhat.com/sgrubb/audit/audit-parse.txt
[...]
> Does ouid and ogid not fit? I'd like us to define what we need in the parser 
> API and then use it in the audit messages. Ancilliary words like new, old, 
> last, first should not be tied with an underscore. If you find any, let me 
> know.

The spec doesn't define what ancillary words are, the syntax it describes
is that the audit record consists of key=value pairs.

I think the options are the following:

- adapt the spec to define ancillary words such as "new".

- add the new_THING field names to the spec (and/or rename them to
  nTHING).

- use unmodified THING field names, and use the record type name to
  disambiguate them.

I dislike the ancillary words since it violates the key=value format (and
the principle of least surprise), and it makes parsing more complex.
Either of the other two options would be ok with me, but I agree with
Steve that any new field names should be documented in the spec and not
just added gratuitously.

(Back in November I had proposed hierarchically structured audit records,
which would have supported structs with named fields directly, but that
discussion died in favor of ad-hoc printfs...)

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

>>If someone is looking for the records for a particular uid, wouldn't
>>> they expect to get the records generated by someone with that uid?
> 
> Not necessarily. I would like to present all matches of uid and let them 
> decide what is relavent.

It seems to me like you could be generating alot of noise.  What will
an ausearch -ui <uid> give you?  The manpage says:

>  -ui <user id>
>               Search for an event with the given user ID.

I wouldn't expect that to include events generated by other users
that used the user id as an argument.

>>At this point there are already a bunch of uid fields (auid, uid, euid,
>>> suid, fsuid, iuid, ouid) in various audit records, and a similar set
>>> of guid files, so would you be happier with nuid, ngid, etc?
> 
> 
> Does ouid and ogid not fit? I'd like us to define what we need in the parser 
> API and then use it in the audit messages. Ancilliary words like new, old, 
> last, first should not be tied with an underscore. If you find any, let me 
> know.

According to your spec, ouid means file owner uid, so that doesn't seem
to fit.

I'm starting to wonder whether we actually need the IPC_NEW_PERMS
record.  We don't spell out similar information for chown, for
example.  In that case, the new owner is a1 field.  Do we treat IPC's
differently because their argument is a structure pointer?

In any case, if someone truly wanted to get all audit records that
had the uid either as part of the subject/object identity and also
included all records that had the uid as an argument, they'd need
to look at the a* fields for other system calls as well.  Since we
don't look at the a* arguments for other syscalls, it doesn't seem
like we should include the arguments for the IPC syscalls if someone
is searching for the records generated by a uid.

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Tue, May 09, 2006 at 01:47:26PM -0400, Linda Knippers wrote:
> I'm starting to wonder whether we actually need the IPC_NEW_PERMS
> record.  We don't spell out similar information for chown, for
> example.  In that case, the new owner is a1 field.  Do we treat IPC's
> differently because their argument is a structure pointer?

Yes, that's the difference. chown takes an integer argument that's
already contained in the syscall record, even though "a1" isn't a very
descriptive name for it.  For ipc_set, the argument is the address of the
structure which would be useless, and the extra record makes the content
of the structure available.

> In any case, if someone truly wanted to get all audit records that
> had the uid either as part of the subject/object identity and also
> included all records that had the uid as an argument, they'd need
> to look at the a* fields for other system calls as well.  Since we
> don't look at the a* arguments for other syscalls, it doesn't seem
> like we should include the arguments for the IPC syscalls if someone
> is searching for the records generated by a uid.

CAPP requires audit records for "All modifications of the values of
security attributes" for 5.4.1 Management of Object Security Attributes
(FMT_MSA.1). It doesn't specifically require the new values as detail
information for the audit record, so if you just want to meet the letter
of CAPP you could leave it out, but the record is of course far more
useful if you include the information.

How the search on UID works in detail isn't all that important. I think
the main issue is that the syscall record contains correct and useful
information. For chown, someone who cares about the argument UID can look
at the "a1" field, but I don't think it would be necessary to have an
easy search method available for that. 

If you want something equivalent for ipc_set, having fields with
unsearchable names would be fine, which happens to be what you
get with the "new_iuid" style field names.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

Klaus Weidner wrote:
> On Tue, May 09, 2006 at 01:47:26PM -0400, Linda Knippers wrote:
> 
>>I'm starting to wonder whether we actually need the IPC_NEW_PERMS
>>record.  We don't spell out similar information for chown, for
>>example.  In that case, the new owner is a1 field.  Do we treat IPC's
>>differently because their argument is a structure pointer?
> 
> 
> Yes, that's the difference. chown takes an integer argument that's
> already contained in the syscall record, even though "a1" isn't a very
> descriptive name for it.  For ipc_set, the argument is the address of the
> structure which would be useless, and the extra record makes the content
> of the structure available.
> 
> 
>>In any case, if someone truly wanted to get all audit records that
>>had the uid either as part of the subject/object identity and also
>>included all records that had the uid as an argument, they'd need
>>to look at the a* fields for other system calls as well.  Since we
>>don't look at the a* arguments for other syscalls, it doesn't seem
>>like we should include the arguments for the IPC syscalls if someone
>>is searching for the records generated by a uid.
> 
> 
> CAPP requires audit records for "All modifications of the values of
> security attributes" for 5.4.1 Management of Object Security Attributes
> (FMT_MSA.1). It doesn't specifically require the new values as detail
> information for the audit record, so if you just want to meet the letter
> of CAPP you could leave it out, but the record is of course far more
> useful if you include the information.
> 
> How the search on UID works in detail isn't all that important. I think
> the main issue is that the syscall record contains correct and useful
> information. For chown, someone who cares about the argument UID can look
> at the "a1" field, but I don't think it would be necessary to have an
> easy search method available for that. 

I wasn't actually proposing that we do that.  I was really trying to
make the point that including the iuid and new_iuid fields in with
the results of a uid search, which is what Steve was proposing as a
good thing, doesn't seem right to me.  There are lots of other syscalls
that could have the uid as an argument that would be skipped so pulling
in the ipc ones just because the arguments are labeled seems
inconsistent and not useful.

> If you want something equivalent for ipc_set, having fields with
> unsearchable names would be fine, which happens to be what you
> get with the "new_iuid" style field names.

Maybe I should use a5, a6, ..., since they're really just more
arguments, although we'd probably have to document which values
we're including since there are more arguments that aren't
being recorded.

> 
> -Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Tuesday 09 May 2006 14:27, Linda Knippers wrote:
> I wasn't actually proposing that we do that.  I was really trying to
> make the point that including the iuid and new_iuid fields in with
> the results of a uid search, which is what Steve was proposing as a
> good thing, doesn't seem right to me.  

OK, big picture time...the fields are used by the search API. The application 
that calls the library can then consult other information to decide if this 
is a record of interest. Ausearch is one such application, but I'm not saying 
that ausearch at the command line will return all those - I'm talking about 
the search API. In retrospect, the user I was referring to is the person 
doing the programming. Sorry for misleading you on this.

Bottom line, for the search API, I want all similar types to have a common 
field name. They can have a modifier adjacent to them.

>Maybe I should use a5, a6, ...,

Please no. Let's keep it as iuid or ouid. I'd personally prefer to drop iuid 
so we can consolidate field types. ouid means "owner's uid".

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Tue, May 09, 2006 at 03:11:25PM -0400, Steve Grubb wrote:
> Bottom line, for the search API, I want all similar types to have a common 
> field name. They can have a modifier adjacent to them.

If that's the way you want to do it, there needs to be a way to get the
modifier to disambiguate them.

Is adding "new " modifiers the best way to do that? You could also
keep the field names the same and look at the syscall record type to find
out which context they get used in.

> On Tuesday 09 May 2006 14:27, Linda Knippers wrote:
> >Maybe I should use a5, a6, ...,

> Please no. Let's keep it as iuid or ouid.

Naming them "a5" etc. would be terrible. The chown way of doing it wasn't
intended as a role model, the point was just that since the information
was present (even though obfuscated) there was no requirement to add
special case logic to audit that call. A userspace reporting tool could
fix this up if it wanted to. If you need new code to get the information,
you may as well make it less obfuscated.

> I'd personally prefer to drop iuid so we can consolidate field types.
> ouid means "owner's uid".

A consolidated field type "ouid" for the object owner makes sense
(assuming that since the IPC records are changing anyway, we might as
well make this additional change).

This still leaves the independent problem that you have a single syscall
which wants to report both the current ouid and the proposed new ouid
it's trying to set it to.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Tue, May 09, 2006 at 03:10:14PM -0500, Klaus Weidner wrote:
> On Tue, May 09, 2006 at 03:11:25PM -0400, Steve Grubb wrote:
> > Bottom line, for the search API, I want all similar types to have a common 
> > field name. They can have a modifier adjacent to them.
> 
> If that's the way you want to do it, there needs to be a way to get the
> modifier to disambiguate them.
> 
> Is adding "new " modifiers the best way to do that? You could also
> keep the field names the same and look at the syscall record type to find
> out which context they get used in.

A bit more detail... 

Here are the current audit records:

type=SYSCALL msg=audit(1146691872.791:94): arch=c000003e syscall=66 success=yes exit=0 a0=10000 a1=1 a2=1 a3=7fff328a7e70 items=0 pid=4327 auid=500 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts2 comm="syscalls" exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls" subj=user_u:system_r:unconfined_t:s0-s0:c0.c255

type=IPC_SET_PERM msg=audit(1146691872.791:94):  new qbytes=0 new iuid=501 new igid=0 new mode=0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255

type=IPC msg=audit(1146691872.791:94):  qbytes=5a5a5a5a5a5a5a5a iuid=0 igid=0 mode=1c0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255


The original patches by Dustin and Linda had used "new_iuid=501" to
differentiate the values, which I personally think was fine since it's
unlikely that people want to be searching for those.

If you absolutely want to avoid adding new tag names, an alternative
would be to get rid of the "new " modifiers, and use the "type=" name to
differentiate them. The audit parsing library could then provide an
auparse_get_field_type() function so that the clients that care can treat
"IPC_SET_PERM" differently from type "IPC".

Something like:

	while (ausearch_next_event(au)) {
		if (auparse_find_field(au, "ouid")
		    && !strcmp(auparse_get_field_type(au), "IPC"))
		{
			printf("ouid=%s\n", auparse_interpret_field(au));
		}
	}

I still think that the new_* field names are fine and don't need fixing,
but the "new" modifiers just look wrong.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

Klaus Weidner wrote:
> On Tue, May 09, 2006 at 03:10:14PM -0500, Klaus Weidner wrote:
> 
>>On Tue, May 09, 2006 at 03:11:25PM -0400, Steve Grubb wrote:
>>
>>>Bottom line, for the search API, I want all similar types to have a common 
>>>field name. They can have a modifier adjacent to them.
>>
>>If that's the way you want to do it, there needs to be a way to get the
>>modifier to disambiguate them.
>>
>>Is adding "new " modifiers the best way to do that? You could also
>>keep the field names the same and look at the syscall record type to find
>>out which context they get used in.
> 
> 
> A bit more detail... 
> 
> Here are the current audit records:
> 
> type=SYSCALL msg=audit(1146691872.791:94): arch=c000003e syscall=66 success=yes exit=0 a0=10000 a1=1 a2=1 a3=7fff328a7e70 items=0 pid=4327 auid=500 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts2 comm="syscalls" exe="/usr/local/eal3_testing/audit-test/syscalls/syscalls" subj=user_u:system_r:unconfined_t:s0-s0:c0.c255
> 
> type=IPC_SET_PERM msg=audit(1146691872.791:94):  new qbytes=0 new iuid=501 new igid=0 new mode=0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255
> 
> type=IPC msg=audit(1146691872.791:94):  qbytes=5a5a5a5a5a5a5a5a iuid=0 igid=0 mode=1c0 obj=user_u:system_r:unconfined_t:s0-s0:c0.c255
> 
> 
> The original patches by Dustin and Linda had used "new_iuid=501" to
> differentiate the values, which I personally think was fine since it's
> unlikely that people want to be searching for those.

And if they do, they're easy to find with an ausearch | grep.

I think Dustin and I were both trying to be consistent with the 99% of
the audit records emitted by the kernel that use single word keywords.
I also think Loulwa had a good suggestion when we suggested modifying
some of the user space audit records to also use single word keywords.

I can understand not wanting to create a bunch of new, random keywords
and I think for the kernel, we do that be reviewing new audit records
to make sure that they only contain the necessary information and re-use
existing keywords where it makes sense.  That's why I removed obj from
the IPC_SET_PERMS record and qbytes from the IPC record.  I guess for
qbytes that means that I can use qbytes instead of new_qbytes since
the IPC_SET_PERMS record is the only place where it shows up.
That's a change I'm happy to make. :-)

> If you absolutely want to avoid adding new tag names, an alternative
> would be to get rid of the "new " modifiers, and use the "type=" name to
> differentiate them. The audit parsing library could then provide an
> auparse_get_field_type() function so that the clients that care can treat
> "IPC_SET_PERM" differently from type "IPC".
> 
> Something like:
> 
> 	while (ausearch_next_event(au)) {
> 		if (auparse_find_field(au, "ouid")
> 		    && !strcmp(auparse_get_field_type(au), "IPC"))
> 		{
> 			printf("ouid=%s\n", auparse_interpret_field(au));
> 		}
> 	}
> 
> I still think that the new_* field names are fine and don't need fixing,
> but the "new" modifiers just look wrong.

I agree.  I would also like something that's easy to parse for people
not using the search API.  Our test suite doesn't use the search API
and probably never will until there's a binary record format.

-- ljk

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

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Tuesday 09 May 2006 16:46, Linda Knippers wrote:
> > The original patches by Dustin and Linda had used "new_iuid=501" to
> > differentiate the values, which I personally think was fine since it's
> > unlikely that people want to be searching for those.
>
> And if they do, they're easy to find with an ausearch | grep.

This is at the wrong level. There may be people that are writing programs that 
want any ouid. I want to stop the proliferation of field names and follow a 
convention. Forget whether or not you think people will ever want the 
information. We need a convention and then to follow it.

> > If you absolutely want to avoid adding new tag names, an alternative
> > would be to get rid of the "new " modifiers, and use the "type=" name to
> > differentiate them. 

I don't want a proliferation of type names either. I think we have a lot of 
them and should try to use existing ones where possible.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Wed, May 10, 2006 at 10:02:31AM -0400, Steve Grubb wrote:
> On Tuesday 09 May 2006 16:46, Linda Knippers wrote:
> > > The original patches by Dustin and Linda had used "new_iuid=501" to
> > > differentiate the values, which I personally think was fine since it's
> > > unlikely that people want to be searching for those.
> >
> > And if they do, they're easy to find with an ausearch | grep.
> 
> This is at the wrong level. There may be people that are writing programs that 
> want any ouid. I want to stop the proliferation of field names and follow a 
> convention. Forget whether or not you think people will ever want the 
> information. We need a convention and then to follow it.

Yes - but "new ouid" is also a different field name from "ouid", and
unnecessarily hard to parse, especially since there's currently no well
defined concept of name modifiers like "new".

> > > If you absolutely want to avoid adding new tag names, an alternative
> > > would be to get rid of the "new " modifiers, and use the "type=" name to
> > > differentiate them. 
> 
> I don't want a proliferation of type names either. I think we have a lot of 
> them and should try to use existing ones where possible.

A list of existing record types would be useful. In this case, it's a
legitimate difference between "current object attributes" and "requested
new object attributes" sub-records that need to be distinct for the
syscall event, so using different types sounds appropriate.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Dustin Kirkland]

On 5/10/06, Klaus Weidner <klaus@atsec.com> wrote:
> On Wed, May 10, 2006 at 10:02:31AM -0400, Steve Grubb wrote:
> > On Tuesday 09 May 2006 16:46, Linda Knippers wrote:
> > > > The original patches by Dustin and Linda had used "new_iuid=501" to
> > > > differentiate the values, which I personally think was fine since it's
> > > > unlikely that people want to be searching for those.
> > >
> > > And if they do, they're easy to find with an ausearch | grep.
> >
> > This is at the wrong level. There may be people that are writing programs that
> > want any ouid. I want to stop the proliferation of field names and follow a
> > convention. Forget whether or not you think people will ever want the
> > information. We need a convention and then to follow it.
>
> Yes - but "new ouid" is also a different field name from "ouid", and
> unnecessarily hard to parse, especially since there's currently no well
> defined concept of name modifiers like "new".

What about a new field in the audit records that is a bitmask, wherein
bits are flipped on for each field being a "new" or "updated" field?

Example:

type=IPC_SET_PERM msg=audit(1146863632.117:98): new_qbytes=0 new_iuid=501
new_igid=0 new_mode=0

becomes:

type=IPC_SET_PERM msg=audit(1146863632.117:98) new=1111: qbytes=0 iuid=501
igid=0 mode=0

or in hex

type=IPC_SET_PERM msg=audit(1146863632.117:98) new=0xF: qbytes=0 iuid=501
igid=0 mode=0

And it's assumed to be 0 in most cases (where it's not explicitly
mentioned.  This way, we don't lose information where a record should
indicate that some or all fields have been updated.  And the searching
mechanisms can continue to operate as expected.

Just a thought.

:-Dustin

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Wed, May 10, 2006 at 12:02:12PM -0500, Dustin Kirkland wrote:
> What about a new field in the audit records that is a bitmask, wherein
> bits are flipped on for each field being a "new" or "updated" field?
> 
> Example:
> 
> type=IPC_SET_PERM msg=audit(1146863632.117:98): new_qbytes=0 new_iuid=501
> new_igid=0 new_mode=0
> 
> becomes:
> 
> type=IPC_SET_PERM msg=audit(1146863632.117:98) new=1111: qbytes=0 iuid=501
> igid=0 mode=0

I'm not sure that's really necessary, the type=IPC_SET_PERM already tells
you that these are new values. How about simply the following:

type=IPC_SET_PERM msg=audit(1146863632.117:98): qbytes=0 iuid=501 igid=0 mode=0

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

> I'm not sure that's really necessary, the type=IPC_SET_PERM already tells
> you that these are new values. How about simply the following:
> 
> type=IPC_SET_PERM msg=audit(1146863632.117:98): qbytes=0 iuid=501 igid=0 mode=0

Would you use ouid and ogid in the IPC record?  Do we need mode
in the IPC record?  We have it in the PATH records.

-- ljk

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Wednesday 10 May 2006 13:11, Klaus Weidner wrote:
> the type=IPC_SET_PERM already tells you that these are new values. How about
> simply the following: 
>
> type=IPC_SET_PERM msg=audit(1146863632.117:98): qbytes=0 iuid=501 igid=0
> mode=0

How about we change that to ouid & ogid ?  Object user ID & Object group ID?

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Klaus Weidner]

On Wed, May 10, 2006 at 01:29:52PM -0400, Steve Grubb wrote:
> On Wednesday 10 May 2006 13:11, Klaus Weidner wrote:
> > the type=IPC_SET_PERM already tells you that these are new values. How about
> > simply the following: 
> >
> > type=IPC_SET_PERM msg=audit(1146863632.117:98): qbytes=0 iuid=501 igid=0
> > mode=0
> 
> How about we change that to ouid & ogid ?  Object user ID & Object group ID?

Good idea, less variants to worry about.

-Klaus

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Wednesday 10 May 2006 12:29, Klaus Weidner wrote:
> > This is at the wrong level. There may be people that are writing programs
> > that want any ouid. I want to stop the proliferation of field names and
> > follow a convention. Forget whether or not you think people will ever
> > want the information. We need a convention and then to follow it.
>
> Yes - but "new ouid" is also a different field name from "ouid", and
> unnecessarily hard to parse,

I am writing the parser. No one else should have to worry about it. Besides, 
we already do this *everywhere* except in this patch. I am just trying to 
keep the whole thing consistent. If you see anywhere that has new_something 
or old_something, please let me know. 

In all the places I looked, the value given is considered the new value. The 
old value is given as old=

Some examples:
"audit_rate_limit=%d old=%d by auid=%u"
"audit_backlog_limit=%d old=%d by auid=%u"

But then there is this:
audit_log_format(ab, "login pid=%d uid=%u " "old auid=%u new auid=%u",

Arguably, that could be re-written as:
audit_log_format(ab, "login pid=%d uid=%u " "auid=%u old auid=%u"

> especially since there's currently no well defined concept of name modifiers
> like "new"

Its used in many places, but you are more likely to run across old. The 
function in the specs that was intended to do this was:

const char *auparse_get_field_name_aux(auparse_state_t *au) - return  
supplemental information about the field's name.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Linda Knippers]

Steve Grubb wrote:
> On Wednesday 10 May 2006 12:29, Klaus Weidner wrote:
> 
>>>This is at the wrong level. There may be people that are writing programs
>>>that want any ouid. I want to stop the proliferation of field names and
>>>follow a convention. Forget whether or not you think people will ever
>>>want the information. We need a convention and then to follow it.
>>
>>Yes - but "new ouid" is also a different field name from "ouid", and
>>unnecessarily hard to parse,
> 
> 
> I am writing the parser. No one else should have to worry about it. 

That assumes everyone uses your parser.  We have existing code we're
supporting that doesn't use your parser and we're not planning to
re-write our code.  I don't know how many other people are in the
same position.  I also think its helpful if the output of ausearch
is easily grepable.

> Besides, 
> we already do this *everywhere* except in this patch. I am just trying to 
> keep the whole thing consistent. If you see anywhere that has new_something 
> or old_something, please let me know. 
> 
> In all the places I looked, the value given is considered the new value. The 
> old value is given as old=

I think what these examples show is that there is no consistency.

> Some examples:
> "audit_rate_limit=%d old=%d by auid=%u"
> "audit_backlog_limit=%d old=%d by auid=%u"

What does "by" signify as a modifier?

> But then there is this:
> audit_log_format(ab, "login pid=%d uid=%u " "old auid=%u new auid=%u",
> 
> Arguably, that could be re-written as:
> audit_log_format(ab, "login pid=%d uid=%u " "auid=%u old auid=%u"

If the middle example used an underscore instead of a space, that would
make lots of people happier.
> 
>>especially since there's currently no well defined concept of name modifiers
>>like "new"
> 
> 
> Its used in many places, but you are more likely to run across old. The 
> function in the specs that was intended to do this was:
> 
> const char *auparse_get_field_name_aux(auparse_state_t *au) - return  
> supplemental information about the field's name.

If I used the APIs then I have to look at the aux information for a
bunch of records I don't want because I can't directly search for the
ones I do?

> 
> -Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Wednesday 10 May 2006 14:05, Linda Knippers wrote:
> We have existing code we're supporting that doesn't use your parser and
> we're not planning to re-write our code.

You'll have to make some mods to it, things have changed in various places.

> I don't know how many other people are in the same position.  I also think
> its helpful if the output of ausearch is easily grepable.

It will be. Nothing has changed here.

> I think what these examples show is that there is no consistency.

It shows that modifiers are not being added to every keyword.

> > "audit_rate_limit=%d old=%d by auid=%u"
> > "audit_backlog_limit=%d old=%d by auid=%u"
>
> What does "by" signify as a modifier?

Its not a modifier, its there for human readability.

> >>especially since there's currently no well defined concept of name
> >> modifiers like "new"
> >
> > Its used in many places, but you are more likely to run across old. The
> > function in the specs that was intended to do this was:
> >
> > const char *auparse_get_field_name_aux(auparse_state_t *au) - return  
> > supplemental information about the field's name.
>
> If I used the APIs then I have to look at the aux information for a
> bunch of records I don't want because I can't directly search for the
> ones I do?

Or use reg expr matching.

-Steve

Re: [PATCH] IPC_SET_PERM cleanup

[Amy Griffis]

On Tue, May 09, 2006 at 11:21:21AM -0400, Steve Grubb wrote:
> On Tuesday 09 May 2006 11:12, Linda Knippers wrote:
> > How it is easier/better?
> 
> Much easier for the user. For example, the user might be searching for uid. 
> Should the user have to specify new_uid ? Would they also need to search for 
> old_uid? Would they have to know all the possible variations on uid?

It seems like the underscore would actually make things easier for the
user.  If they want just the 'uid' field, they search on 'uid'.  If
they want all the possible variations on uid, they could use a regular
expression.

Re: [PATCH] IPC_SET_PERM cleanup

[Steve Grubb]

On Friday 05 May 2006 16:19, Linda Knippers wrote:
> @@ -684,21 +684,10 @@ static void audit_log_exit(struct audit_
>                 case AUDIT_IPC_SET_PERM: {
>                         struct audit_aux_data_ipcctl *axi = (void *)aux;
>                         audit_log_format(ab,
> -                               " new qbytes=%lx new iuid=%u new igid=%u new mode=%x", 
> +                               "new_qbytes=%lx new_iuid=%u new_igid=%u new_mode=%x",

Please revert this chunk.

-Steve