Dave,

I think Steve and I have agreed on a solution, I'll put together a patch
right now based on what is currently in net-2.6 (i.e. the existing
NetLabel audit patch) and submit it to the lists in a few hours.

Steve Grubb wrote:

On Friday 29 September 2006 14:09, Paul Moore wrote:

quoted

quoted

type field is already taken for another purpose, it needs to be renamed.

If we can't have duplicate field names I would propose prefixing both
these fields (and doing similar things with the other NetLabel specific
fields) with a "cipso_" making them "cipso_doi" and "cipso_type".

That would be fine. This limits future field name collisions.

quoted

quoted

quoted

+/**
+ * netlbl_unlabel_acceptflg_set - Set the unlabeled accept flag
+ * @value: desired value
+ * @audit_secid: the LSM secid to use in the audit message
+ *
+ * Description:
+ * Set the value of the unlabeled accept flag to @value.
+ *
+ */
+static void netlbl_unlabel_acceptflg_set(u8 value, u32 audit_secid)
+{
+     atomic_set(&netlabel_unlabel_accept_flg, value);
+     netlbl_audit_nomsg((value ?
+                         AUDIT_MAC_UNLBL_ACCEPT : AUDIT_MAC_UNLBL_DENY),
+                        audit_secid);

Looking at how this is being used, I think only 1 message type should be
used. There are places in the audit system where we set a flag to 1 or 0,
but only have 1 message type. We record the old and new value. So, you'd
need to pass that to the logger.

With that in mind I would probably change the message type to
AUDIT_MAC_UNLBL_ALLOW and use a "unlbl_accept" field; is that okay?  

That would be fine. Just a quick note...we have generally been "old " to 
indicate the previous value. Example, "backlog=512 old=256".

quoted

quoted

quoted

+/**
+ * netlbl_audit_start_common - Start an audit message
+ * @type: audit message type
+ * @secid: LSM context ID
+ *
+ * Description:
+ * Start an audit message using the type specified in @type and fill the
audit + * message with some fields common to all NetLabel audit messages.
Returns + * a pointer to the audit buffer on success, NULL on failure.
+ *
+ */
+struct audit_buffer *netlbl_audit_start_common(int type, u32 secid)
+{

Generally, logging functions are moved into auditsc.c where the context
and other functions are defined.

How about leaving this for a future revision?

Come to think of it, you don't need to move it. The reason to move it is to 
access the context and use helper functions related to it. But I found that 
you were using "current" which may not always be the sender. So if you cannot 
use current, most of the stuff you are logging can't be, so the event being 
logged becomes simpler and you don't need to move it.

I have not traced through all the code, but if you do any security checks 
before taking the rules, be careful not to use current.

quoted

quoted

quoted

+     audit_log_format(audit_buf,
+                      "netlabel: auid=%u uid=%u tty=%s pid=%d",
+                      audit_loginuid,
+                      current->uid,
+                      audit_tty,
+                      current->pid);

Why are you logging all this? When we add audit rules, all that we log is
the auid, and subj. If we need to log all this, we should probably have a
helper function that gets called by other config change loggers.

If I drop the uid, tty, and pid fields will this be acceptable?

 and comm & exe, yes. Anything you were basing off of current has to go. The 
audit rule logging was reduced to the credentials that are carried along in 
the netlink packet since that's all you can trust. The sending process could 
be gone by the time you get to this point in the code.

quoted

quoted

quoted

+     audit_log_format(audit_buf, " comm=");
+     audit_log_untrustedstring(audit_buf, audit_comm);
+     if (current->mm) {
+             down_read(&current->mm->mmap_sem);
+             vma = current->mm->mmap;
+             while (vma) {
+                     if ((vma->vm_flags & VM_EXECUTABLE) &&
+                         vma->vm_file) {
+                             audit_log_d_path(audit_buf,
+                                              " exe=",
+                                              vma->vm_file->f_dentry,
+                                              vma->vm_file->f_vfsmnt);
+                             break;
+                     }
+                     vma = vma->vm_next;
+             }
+             up_read(&current->mm->mmap_sem);
+     }
+

If this function was moved inside auditsc.c you could use a function
there that does this. But the question remains why all this data?

In the ideal world would you prefer this to be removed?

Yes.

-Steve

-- 
paul moore
linux security @ hp