Re: [PATCH 1/1] NetLabel: add audit support for configuration changes

[Paul Moore <paul.moore@hp.com>]

Steve Grubb wrote:
> On Thursday 28 September 2006 14:03, paul.moore@hp.com wrote:
>>@@ -381,21 +380,35 @@ static int netlbl_cipsov4_add(struct sk_
>>
>> {
>> 	int ret_val = -EINVAL;
>>-	u32 map_type;
>>+	u32 type;
>>+	u32 doi;
>>+	const char *type_str = "(unknown)";
>>+	struct audit_buffer *audit_buf;
>>
>>-	if (!info->attrs[NLBL_CIPSOV4_A_MTYPE])
>>+	if (!info->attrs[NLBL_CIPSOV4_A_DOI] ||
>>+	    !info->attrs[NLBL_CIPSOV4_A_MTYPE])
>> 		return -EINVAL;
>>
>>-	map_type = nla_get_u32(info->attrs[NLBL_CIPSOV4_A_MTYPE]);
>>-	switch (map_type) {
>>+	type = nla_get_u32(info->attrs[NLBL_CIPSOV4_A_MTYPE]);
>>+	switch (type) {
>> 	case CIPSO_V4_MAP_STD:
>>+		type_str = "std";
>> 		ret_val = netlbl_cipsov4_add_std(info);
>> 		break;
>> 	case CIPSO_V4_MAP_PASS:
>>+		type_str = "pass";
>> 		ret_val = netlbl_cipsov4_add_pass(info);
>> 		break;
>> 	}
>>
>>+	if (ret_val == 0) {
>>+		doi = nla_get_u32(info->attrs[NLBL_CIPSOV4_A_DOI]);
>>+		audit_buf = netlbl_audit_start_common(AUDIT_MAC_CIPSOV4_ADD,
>>+						      NETLINK_CB(skb).sid);
>>+		audit_log_format(audit_buf, " doi=%u type=%s", doi, type_str);
> 
> 
> 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".

If this isn't acceptable please suggest names which you feel are
appropriate.

>>+/**
>>+ * 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?  If
not please suggest something you would find acceptable.

>>+/**
>>+ * 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?  I'd like this first
attempt to be relatively self contained.  James Morris has made other
comments along the same lines.

>>+	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?

>>+	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?

-- 
paul moore
linux security @ hp