Re: [PATCH 1/2] audit: fix NUL handling in untrusted strings

[John Dennis <jdennis@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3344 bytes --]

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>


[-- Attachment #1.2: Type: text/html, Size: 4031 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]