Steve Grubb wrote:
> On Thursday 28 September 2006 14:03, [EMAIL PROTECTED] 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(¤t->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(¤t->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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
