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

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

[Miloslav Trmač]

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.

Signed-off-by: Miloslav Trmac <mitr@redhat.com>
--- 
kernel/audit.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 4414e93..03b6397 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1362,6 +1362,12 @@ void audit_log_n_string(struct audit_buffer *ab, const char *string,
 	skb_put(skb, slen + 2);	/* don't include null terminator */
 }
 
+static inline int
+audit_is_control_character(unsigned char c)
+{
+	return c == '"' || c < 0x21 || c > 0x7E;
+}
+
 /**
  * audit_string_contains_control - does a string need to be logged in hex
  * @string: string to be checked
@@ -1371,7 +1377,7 @@ 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)
+		if (audit_is_control_character(*p))
 			return 1;
 	}
 	return 0;
@@ -1394,10 +1400,15 @@ int audit_string_contains_control(const char *string, size_t len)
 void audit_log_n_untrustedstring(struct audit_buffer *ab, const char *string,
 				 size_t len)
 {
-	if (audit_string_contains_control(string, len))
-		audit_log_n_hex(ab, string, len);
-	else
-		audit_log_n_string(ab, string, len);
+	const unsigned char *p;
+
+	for (p = string; p < (const unsigned char *)string + len && *p; p++) {
+		if (audit_is_control_character(*p)) {
+			audit_log_n_hex(ab, string, len);
+			return;
+		}
+	}
+	audit_log_n_string(ab, string, p - (const unsigned char *)string);
 }
 
 /**

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

[Eric Paris]

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.

It's still not right looking at the other user,
audit_log_single_execve_arg().  An execve arg with a NULL could loose
the stuff after the NULL (not break the record like audit_tty) since the
execve uses %s rather than calling trusted string.

How about we change the meaning of audit_string_contains_control()
return values?  If it returns positive that is the number of bytes in a
legitimate string up to the first null.  -1 means it is hex.  That
eliminates your code duplication and allows us to do the right thing in
both the generic untrusted_string code and the execve code.....

-Eric

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

[Miloslav Trmač]

Thanks for the review.
Eric Paris píše v Čt 11. 09. 2008 v 10:25 -0400:
> On Thu, 2008-09-11 at 00:23 +0200, Miloslav Trmač wrote:
> > 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.
> 
> It's still not right looking at the other user,
> audit_log_single_execve_arg().  An execve arg with a NULL could loose
> the stuff after the NULL (not break the record like audit_tty) since the
> execve uses %s rather than calling trusted string.
execve() arguments are NUL-terminated strings:
audit_log_single_execve_arg() starts with
	len_left = len = strnlen_user(p, MAX_ARG_STRLEN) - 1;
and the two cycles handle chunks in the first "len" bytes of "p".
audit_log_single_execve_arg() never touches any bytes after the first
NUL.

So, when audit_log_single_execve_arg() calls
audit_string_contains_control(buf, to_send), strlen(buf) == to_send and
the patch does not change anything.

> How about we change the meaning of audit_string_contains_control()
> return values?  If it returns positive that is the number of bytes in a
> legitimate string up to the first null.  -1 means it is hex.
That is possible, although the return value convention is somewhat
complex.  Anyway, no change to the semantics of
audit_string_contains_control() is necessary for execve() argument
logging.
	Mirek

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

[John Dennis]

[-- 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 --]

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

[Miloslav Trmač]

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

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

[Miloslav Trmač]

Miloslav Trmač píše v Čt 11. 09. 2008 v 20:10 +0200:
> Yes, that's possible - but then audit_log_n_untrustedstring() would be
> more accurately called audit_log_n_ascii_like_binary_data().
... my original patch (which stops at the first NUL) works the same way
the other interfaces with maximum string length (e.g. strnlen() or
printf ("%.5s", ...)) do.
	Mirek

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

[Steve Grubb]

On Thursday 11 September 2008 14:10:12 Miloslav Trmač wrote:
> > 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.

In every case where this occurs (kernel or user space), the field values are 
expected to be encoded to prevent it from being discarded.

-Steve

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

[John Dennis]

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

Miloslav Trmač wrote:
> 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). 
>
>   
A primary purpose of the audit system is to log with the greatest 
accuracy possible the actual data. If that data somehow contained a 
null, even in a context in which a null would have been prohibited, the 
audit system absolutely needs to be able to correctly record that 
aberrant event and it's actual data. If the audit system failed at that 
moment it's failing at the worst possible moment, the moment when you're 
looking for bad data.

A UNIX-like operating system does not in and of itself mandate the 
default conventions of the C programming language. A great danger and 
the source of bugs is making assumptions concerning how to interpret 
octet sequences. By far and away the worst possible place to make a 
mistake handling an octet sequence is in the kernel. It would be wrong 
for the audit system to assume the memory block it was pointed to only 
ever contained null terminated ascii strings, especially when the memory 
block is terminated by virtue of an octet count.
> 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.
>   

-- 
John Dennis <jdennis@redhat.com>


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

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

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

[Andrew Morton]

On Thu, 11 Sep 2008 00:23:38 +0200
Miloslav Trma__ <mitr@redhat.com> wrote:

> 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.

It's unclear how serious this problem is.  Do you believe that it is
sufficiently serious to warrant merging these fixes into 2.6.27? 
2.6.26.x?  2.6.25.x?

Thanks.

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

[John Dennis]

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

Steve Grubb wrote:
> On Thursday 11 September 2008 14:10:12 Miloslav Trmač wrote:
>   
>>> 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.
>>     
>
> In every case where this occurs (kernel or user space), the field values are 
> expected to be encoded to prevent it from being discarded.
>   

This is true. The proposed patch defeats the encoding of the entire data 
block and thus fails the criteria Steve correctly states is a requirement.

The concern I have in the user level audit code is not with handling the 
encoded string values which is fine, but rather with the handling the 
decoded string block.

-- 
John Dennis <jdennis@redhat.com>


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

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

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

[Miloslav Trmač]

John Dennis píše v Čt 11. 09. 2008 v 15:12 -0400:
> Miloslav Trmač wrote: 
> > 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). 
> >   
> A primary purpose of the audit system is to log with the greatest
> accuracy possible the actual data. If that data somehow contained a
> null, even in a context in which a null would have been prohibited,
> the audit system absolutely needs to be able to correctly record that
> aberrant event and it's actual data. If the audit system failed at
> that moment it's failing at the worst possible moment, the moment when
> you're looking for bad data.
If the interface says "NUL-terminated string", any bytes after that are
not "actual data".  The bytes might be useful for diagnosing an anomaly
if the kernel's behavior somehow depended on the bytes after NUL due to
a kernel bug - but the kernel's behavior might depend on anything due to
such a bug, and we don't log the complete state of the system in each
audit message.  The "actual data" of a string is only up to the NUL
byte.

> It would be wrong for the audit system to assume the memory block it
> was pointed to only ever contained null terminated ascii strings,
> especially when the memory block is terminated by virtue of an octet
> count.
Yes, that's why it was wrong to use audit_*string() for TTY input data.
And the 2/2 patch fixes it - at the source of the problem, not in an
unrelated function that was incorrectly used.
	Mirek

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

[Miloslav Trmač]

Andrew Morton píše v Čt 11. 09. 2008 v 12:14 -0700:
> On Thu, 11 Sep 2008 00:23:38 +0200
> Miloslav Trma__ <mitr@redhat.com> wrote:
> 
> > 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.
> > 

> It's unclear how serious this problem is.
* AUDIT_USER_TTY records (which are sent from user-space with a trailing
NUL byte) are missing a terminating '"' character.
* Some data is not recorded in AUDIT_TTY records, and the terminating
'"' character is missing in that case as well.

>   Do you believe that it is
> sufficiently serious to warrant merging these fixes into 2.6.27? 
> 2.6.26.x?  2.6.25.x?
This patch (1/2) only fixes creation of incorrectly formatted, but
easy-to-understand audit records; it would be nice to have it in 2.6.27
(assuming the audit maintainer acks it - or a variant of it).  The other
one (2/2), which makes sure all TTY audit data is recorded, should
probably be merged in the stable releases as well.
	Mirek

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

[John Dennis]

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

Miloslav Trmač wrote:
> If the interface says "NUL-terminated string", any bytes after that are
> not "actual data".
Yes, that's correct. However, the function in question, 
audit_log_n_untrustedstring() is not an interface accepting a null 
terminated string, it accepts a count. The helper function on which it 
is dependent, audit_string_contains_control(), disregards the length 
parameter it is passed and thus audit_log_n_untrustedstring() misbehaves 
as a consequence.
>> It would be wrong for the audit system to assume the memory block it
>> was pointed to only ever contained null terminated ascii strings,
>> especially when the memory block is terminated by virtue of an octet
>> count.
>>     
> Yes, that's why it was wrong to use audit_*string() for TTY input data.
> And the 2/2 patch fixes it - at the source of the problem, not in an
> unrelated function that was incorrectly used.
>   
This is true, but it's only part of the problem, the string functions 
still need to be robust, even used inappropriately.

-- 
John Dennis <jdennis@redhat.com>


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

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

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

[Miloslav Trmač]

John Dennis píše v Čt 11. 09. 2008 v 15:47 -0400:
> Miloslav Trmač wrote: 
> > If the interface says "NUL-terminated string", any bytes after that are
> > not "actual data".
> Yes, that's correct. However, the function in question,
> audit_log_n_untrustedstring() is not an interface accepting a null
> terminated string, it accepts a count.
These options are not exclusive - see strnlen().
audit_log_n_untrustedstring(), as patched, follows that model exactly.

> > Yes, that's why it was wrong to use audit_*string() for TTY input data.
> > And the 2/2 patch fixes it - at the source of the problem, not in an
> > unrelated function that was incorrectly used.
> >   
> This is true, but it's only part of the problem, the string functions
> still need to be robust, even used inappropriately.
They need to do what they promise in the first place, only then can they
possibly do something useful with invalid input.  AUDIT_USER_TTY
messages, like all messages sent from user-space, are sent with a
trailing NUL.  The contents of the message are (more or less) trusted,
it is perfectly reasonable to stop at first NUL.
audit_log_n_untrustedstring(), as patched, does exactly what is needed -
the length parameter is used only to make unrelated kernel data does not
appear in the audit record. 

Now, given a function called fn(), either behavior (terminating at NUL,
or treating NUL as a control character) is valid, and AUDIT_USER_TTY
handling can be written for both behaviors of fn().  But given the
function is called ...untrustedstring, and "string" in C implies
NUL-terminated, I think it is more consistent to stop on NUL.
	Mirek