Eric Paris wrote: 
On Thu, 2008-09-11 at 00:23 +0200, Miloslav Trmač wrote:
  
From: Miloslav Trmac <mitr@redhat.com>

audit_string_contains_control() stops checking at the first NUL byte.
If audit_string_contains_control() returns FALSE,
audit_log_n_untrustedstring() submits the complete string - including
the NUL byte and all following bytes, up to the specified maximum length
- to audit_log_n_string(), which copies the data unchanged into the
audit record.

The audit record can thus contain a NUL byte (and some unchecked data
after that).  Because the user-space audit daemon treats audit records
as NUL-terminated strings, an untrusted string that is shorter than the
specified maximum length effectively terminates the audit record.

This patch modifies audit_log_n_untrustedstring() to only log the data
before the first NUL byte, if any.
    

I'm going to have to say NAK on this patch.
I agree with Eric, this is the wrong solution, but for different (additional) reasons.

It's incumbent upon the kernel audit system to correctly log all string data and not try to interpret the contents of that string data. Special processing with regards to the presence or absence of a null byte is one example of prohibited interpretation. An attacker could hide information after a null byte if it knew everything after the null byte was being discarded. It is critical for post-mortem analysis to be able to reconstruct the fact string data was passed somewhere which contained a null byte which may have been a trigger for subsequent behaviour. In addition depending on the string data character encoding it is possible to have legitimate null bytes which do not represent string termination. Making assumptions about the character encoding of an octet sequence is another example of prohibited interpretation. In the particular example cited the string data is actually part of terminal protocol which in fact permits nulls because it's not string data but rather an octet sequence.

The important point is:
A string value should always be a counted octet sequence for the purpose of auditing.

It seems to me the problem is with audit_string_contains_control():

int audit_string_contains_control(const char *string, size_t len)
{
    const unsigned char *p;
    for (p = string; p < (const unsigned char *)string + len && *p; p++) {
        if (*p == '"' || *p < 0x21 || *p > 0x7e)
            return 1;
    }
    return 0;
}

The problem is that it is passed a counted octet sequence but in some circumstances ignores the count. This occurs when *p == 0, the test for NULL should be removed. If that test is removed it will return the flag which indicates the string must be encoded differently to be conformant with the protocol.

With this change auditd will not terminate the record prematurely because the string value will have been properly encoded according to the protocol.

As a side note I'm concerned there may be places in the user audit code which treat string data as null terminated (at least that is my recollection). If so that is an area which needs to be fixed to treat string values as counted octet sequences.

-- 
John Dennis <jdennis@redhat.com>