5:23 p.m.
From: Miloslav Trmac <mitr(a)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(a)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);
}
/**
9:25 a.m.
On Thu, 2008-09-11 at 00:23 +0200, Miloslav Trmač wrote:
From: Miloslav Trmac <mitr(a)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
9:43 a.m.
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
12:30 p.m.
Eric Paris wrote:
On Thu, 2008-09-11 at 00:23 +0200, Miloslav Trmač wrote:
> From: Miloslav Trmac <mitr(a)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(a)redhat.com>
1:10 p.m.
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
1:15 p.m.
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
2:08 p.m.
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
2:19 p.m.
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(a)redhat.com>
2:12 p.m.
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(a)redhat.com>
2:27 p.m.
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
2:47 p.m.
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(a)redhat.com>
3:03 p.m.
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
2:14 p.m.
On Thu, 11 Sep 2008 00:23:38 +0200
Miloslav Trma__ <mitr(a)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.
2:37 p.m.
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(a)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
5946
days inactive
5947
days old
linux-audit@lists.linux-audit.osci.io
13 comments
5 participants
participants (5)
-
Andrew Morton -
Eric Paris -
John Dennis -
Miloslav Trmač -
Steve Grubb