12:56 p.m.
--- David Woodhouse <dwmw2(a)infradead.org> wrote:
A string in quotes isn't sufficient -- a file name
can contain quotes as
well as carriage returns. The only safe option is
hex -- it's just a
question of whether it's "string or fall back to hex
if dangerous" or
whether it's "just hex".
Another alternative is the old "\ddd" notation.
It's not nice, but it is much better than dumping
path names in hex. I know that an entire generation
would have to learn octal, but if the Unix crowd
could deal with hex and octal I expect the Linux
community could too.
Casey Schaufler
casey(a)schaufler-ca.com
__________________________________
Do you Yahoo!?
Yahoo! Small Business - Try our new resources site!
http://smallbusiness.yahoo.com/resources/
1:07 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thu, 2005-03-17 at 10:56 -0800, Casey Schaufler wrote:
Another alternative is the old "\ddd" notation.
It's not nice, but it is much better than dumping
path names in hex.
When we're speaking of making the kernel render it that way, I disagree.
We really don't want to get into that kind of text processing. In the
common case where it's safe, let's dump it as text. In the rare case
that it contains dubious characters, dump it as hex.
--
dwmw2
1:17 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thu, 17 Mar 2005 19:07:19 GMT, David Woodhouse said:
When we're speaking of making the kernel render it that way, I
disagree.
We really don't want to get into that kind of text processing. In the
common case where it's safe, let's dump it as text. In the rare case
that it contains dubious characters, dump it as hex.
If you include "whitespace" as a "dubious" character, that would kill
several birds with one stone. Then you just need 'name=A:ascii_string'
or 'name=H:hexstring' and most parsing issues go away. Works for me...
1:30 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thu, 2005-03-17 at 14:17 -0500, Valdis.Kletnieks(a)vt.edu wrote:
If you include "whitespace" as a "dubious"
character, that would kill
several birds with one stone. Then you just need 'name=A:ascii_string'
or 'name=H:hexstring' and most parsing issues go away. Works for me...
Untested:
--- linux-2.6.9/kernel/audit.c.auditstr 2005-03-17 19:08:42.000000000 +0000
+++ linux-2.6.9/kernel/audit.c 2005-03-17 19:24:20.000000000 +0000
@@ -731,6 +731,29 @@ void audit_log_format(struct audit_buffe
va_end(args);
}
+void audit_log_hex(struct audit_buffer *ab, const unsigned char *buf, size_t len)
+{
+ int i;
+
+ for (i=0; i<len; i++)
+ audit_log_format(ab, "%02x", buf[i]);
+}
+
+void audit_log_untrustedstring(struct audit_buffer *ab, const char *string)
+{
+ const char *p = string;
+
+ while (*p) {
+ if (*p == '"' || *p == ' ' || *p < 0x20 || *p > 0x7f) {
+ audit_log_hex(ab, string, strlen(string));
+ return;
+ }
+ p++;
+ }
+ audit_log_format(ab, "\"%s\"", string);
+}
+
+
/* This is a helper-function to print the d_path without using a static
* buffer or allocating another buffer in addition to the one in
* audit_buffer. */
--- linux-2.6.9/kernel/auditsc.c.auditstr 2005-03-17 19:08:42.000000000 +0000
+++ linux-2.6.9/kernel/auditsc.c 2005-03-17 19:19:41.000000000 +0000
@@ -726,9 +726,10 @@ static void audit_log_exit(struct audit_
if (!ab)
continue; /* audit_panic has been called */
audit_log_format(ab, "item=%d", i);
- if (context->names[i].name)
- audit_log_format(ab, " name=%s",
- context->names[i].name);
+ if (context->names[i].name) {
+ audit_log_format(ab, "name=");
+ audit_log_untrustedstring(ab, context->names[i].name);
+ }
if (context->names[i].ino != (unsigned long)-1)
audit_log_format(ab, " inode=%lu dev=%02x:%02x mode=%#o"
" uid=%d gid=%d rdev=%02x:%02x",
--- linux-2.6.9/include/linux/audit.h.auditstr 2005-03-17 19:08:42.000000000 +0000
+++ linux-2.6.9/include/linux/audit.h 2005-03-17 19:10:16.000000000 +0000
@@ -232,6 +232,10 @@ extern void audit_log_format(struct
extern void audit_log_end(struct audit_buffer *ab);
extern void audit_log_end_fast(struct audit_buffer *ab);
extern void audit_log_end_irq(struct audit_buffer *ab);
+extern void audit_log_hex(struct audit_buffer *ab, const unsigned char *buf,
+ size_t len);
+extern void audit_log_untrustedstring(struct audit_buffer *ab,
+ const char *string);
extern void audit_log_d_path(struct audit_buffer *ab,
const char *prefix,
struct dentry *dentry,
@@ -254,6 +258,8 @@ extern void audit_log_lost(const ch
#define audit_log_end(b) do { ; } while (0)
#define audit_log_end_fast(b) do { ; } while (0)
#define audit_log_end_irq(b) do { ; } while (0)
+#define audit_log_hex(a,b,l) do { ; } while (0)
+#define audit_log_untrustedstring(a,s) do { ; } while (0)
#define audit_log_d_path(b,p,d,v) do { ; } while (0)
#define audit_set_rate_limit(l) do { ; } while (0)
#define audit_set_backlog_limit(l) do { ; } while (0)
--
dwmw2
1:55 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
* David Woodhouse (dwmw2(a)infradead.org) wrote:
On Thu, 2005-03-17 at 14:17 -0500, Valdis.Kletnieks(a)vt.edu wrote:
> If you include "whitespace" as a "dubious" character, that would
kill
> several birds with one stone. Then you just need 'name=A:ascii_string'
> or 'name=H:hexstring' and most parsing issues go away. Works for me...
Untested:
I like this.
--- linux-2.6.9/kernel/audit.c.auditstr 2005-03-17 19:08:42.000000000
+0000
+++ linux-2.6.9/kernel/audit.c 2005-03-17 19:24:20.000000000 +0000
@@ -731,6 +731,29 @@ void audit_log_format(struct audit_buffe
va_end(args);
}
+void audit_log_hex(struct audit_buffer *ab, const unsigned char *buf, size_t len)
+{
+ int i;
+
+ for (i=0; i<len; i++)
+ audit_log_format(ab, "%02x", buf[i]);
Could use '\0' to terminate loop instead of doing extra strlen()
+void audit_log_untrustedstring(struct audit_buffer *ab, const char
*string)
+{
+ const char *p = string;
+
+ while (*p) {
+ if (*p == '"' || *p == ' ' || *p < 0x20 || *p > 0x7f) {
1) would '*p < 0x20 || *p > 0x7f -> !isprint(*p)' look cleaner?
2) i wonder, do we need dump hex if there's a space since it's wrapped
in quotes?
3) if so, what about other space (like tab)? dunno how the parsers are
defining whitespace.
2 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thu, 2005-03-17 at 11:55 -0800, Chris Wright wrote:
Could use '\0' to terminate loop instead of doing extra
strlen()
I wanted the hex dump option to be usable standalone.
1) would '*p < 0x20 || *p > 0x7f -> !isprint(*p)'
look cleaner?
Yeah, maybe.
2) i wonder, do we need dump hex if there's a space since
it's wrapped
in quotes?
I didn't originally. I changed it in response to Valdis' message, just
before I posted it. You're probably right, but...
3) if so, what about other space (like tab)? dunno how the parsers
are defining whitespace.
... if the log parser is using space as the delimiter between
'tag=value' field, then maybe it's still useful to avoid including them.
--
dwmw2
11:57 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thu, 17 Mar 2005 19:30:58 GMT, David Woodhouse said:
+ if (*p == '"' || *p == ' ' || *p < 0x20 ||
*p > 0x7f) {
+ audit_log_hex(ab, string, strlen(string));
+ return;
+ }
+ p++;
+ }
+ audit_log_format(ab, "\"%s\"", string);
Hmm.. we either get a "string" or a stream-of-hex-digits, we can identify
which it is by looking at the first byte, we can always unambiguously find
the end, and there's no ambiguous representations, plus the code is relatively
simple. Not bad.
audit_log_hex is almost a one-liner, would probably benefit from an 'inline' or
just open-coding it. Not sure if the inner audit_log_format() call can
be further optimized.
10:20 a.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Thursday 17 March 2005 11:57 pm, Valdis.Kletnieks(a)vt.edu wrote:
On Thu, 17 Mar 2005 19:30:58 GMT, David Woodhouse said:
> + if (*p == '"' || *p == ' ' || *p < 0x20 || *p > 0x7f)
{
> + audit_log_hex(ab, string, strlen(string));
> + return;
> + }
> + p++;
> + }
> + audit_log_format(ab, "\"%s\"", string);
Hmm.. we either get a "string" or a stream-of-hex-digits, we can identify
which it is by looking at the first byte, we can always unambiguously find
the end, and there's no ambiguous representations, plus the code is
relatively simple. Not bad.
audit_log_hex is almost a one-liner, would probably benefit from an
'inline' or just open-coding it. Not sure if the inner audit_log_format()
call can be further optimized.
I'm going to hold off on the next auditfs patch to add this support into my
code and use it for my name= and filterkey= fields so we can have some
uniformity.
-tim
11:09 a.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Friday 18 March 2005 10:20 am, Timothy R. Chavez wrote:
<snip>
I'm going to hold off on the next auditfs patch to add this
support into my
code and use it for my name= and filterkey= fields so we can have some
uniformity.
-tim
So, just a heads up... I had to explicitly cast *p to unsigned char, in some
fashion, to get it to compile without warning (warning: comparison is always
false due to limited range of data type) when comparing with the hex values.
Is this correct?
-tim
12:44 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Friday 18 March 2005 11:20, Timothy R. Chavez wrote:
I'm going to hold off on the next auditfs patch to add this
support into my
code and use it for my name= and filterkey= fields so we can have some
uniformity.
I can see why you need it for name, but I cannot see a reason for filterkey.
I've tightened the communication between auditctl and the kernel in the 0.6.9
release. In this way, I can better detect and handle errors with the kernel.
I would think that you could scan filterkey when accepting the key and allow
only alpha numeric characters, underscore, and dash. Anything else generates
an EINVAL.
This would be less work for me to parse and faster for the kernel since it
doesn't spend time encoding that field.
-Steve
12:46 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Friday 18 March 2005 12:44 pm, Steve Grubb wrote:
On Friday 18 March 2005 11:20, Timothy R. Chavez wrote:
> I'm going to hold off on the next auditfs patch to add this support into
> my code and use it for my name= and filterkey= fields so we can have some
> uniformity.
I can see why you need it for name, but I cannot see a reason for
filterkey.
That's fine, if we want to consider it a trusted string. I'll make the change
right now.
-Steve
-tim
12:57 p.m.
New subject: [patch] Syscall auditing - move "name=" field to the end
On Friday 18 March 2005 13:48, Timothy R. Chavez wrote:
Oh, did you want me to encapsulate it in quotations?
No. I want the string validated when its accepted. If any character is not
alphanumeric, underscore, or dash - reject the whole rule with -EINVAL. This
way we never have a question about the string whenever its emitted.
Name, you'll have to take whatever gets sent. But we can be choosy about
filterkey.
-Steve
7219
days inactive
7220
days old
linux-audit@lists.linux-audit.osci.io
12 comments
6 participants
participants (6)
-
Casey Schaufler -
Chris Wright -
David Woodhouse -
Steve Grubb -
Timothy R. Chavez -
Valdis.Kletnieks＠vt.edu