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

["Miloslav Trmač" <mitr@redhat.com>]

John Dennis píše v Čt 11. 09. 2008 v 13:30 -0400:
> Special processing with regards to the presence or absence of a null
> byte is one example of prohibited interpretation.
This is UNIX, "string" means "NUL-terminated string" (in fact the
presence of a NUL byte is the only way to reasonably detect binary
data). 

You're far more likely to encounter a fixed-length field with an
optional terminating NUL (like the old-style, 16-byte directory entries)
than an ASCII-compatible string that intentionally contains a NUL byte.

TTY input auditing was the only place where it makes a difference, all
other code was passing a string that was at least as long as the
specified size to audit_log_n_untrustedstring().

> 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.
Yes, that's possible - but then audit_log_n_untrustedstring() would be
more accurately called audit_log_n_ascii_like_binary_data().

Anyway, Eric/Al - if you prefer this solution, I can prepare an
alternative patch.

> 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).
Yes, auditd adds a NUL terminator to the audit record, and then treats
it as a regular NUL-terminated string; if the audit record contains an
embedded NUL byte, the rest of the record is discarded by auditd. 
	Mirek