[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

[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

[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

[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

[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