[PATCH] Don't crash on unknown S_IFMT file modes

[PATCH] Don't crash on unknown S_IFMT file modes

[Miloslav Trmac]

Hello,
ausearch -i and libauparse currently crash (access NULL) if a mode= field contains an unknown file type.  Such records are generated by the kernel for IPC, e.g.

    node=jcdx156 type=IPC msg=audit(1237915952.720:2294): ouid=500 ogid=1106 mode=0600 obj=siterep_u:siterep_r:siterep_t:s0-s15:c0.c1023

The attached patch:
* Modifies ausearch and libauparse to output the file format in octal if it is unknown.
* Modifies libauparse to use the same interpreted field format as ausearch (without a space in the middle).
* Modifies comma handling in libauparse to avoid a strcat() call.

    Mirek

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[LC Bruzenak]

On Thu, 2009-03-26 at 08:06 -0400, Miloslav Trmac wrote:
> Hello,
> ausearch -i and libauparse currently crash (access NULL) if a mode= field contains an unknown file type.  Such records are generated by the kernel for IPC, e.g.
> 
>     node=jcdx156 type=IPC msg=audit(1237915952.720:2294): ouid=500 ogid=1106 mode=0600 obj=siterep_u:siterep_r:siterep_t:s0-s15:c0.c1023
> 
> The attached patch:
> * Modifies ausearch and libauparse to output the file format in octal if it is unknown.
> * Modifies libauparse to use the same interpreted field format as ausearch (without a space in the middle).
> * Modifies comma handling in libauparse to avoid a strcat() call.
> 
>     Mirek

Mirek,

Thank you for this patch...wherever it may be.
:)
I really appreciate you fixing this!

Do you have a standard auparse test you use to track these down?
If so, does it use auparse_feed?

Thanks again,
LCB.

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[Steve Grubb]

On Thursday 26 March 2009 08:06:12 am Miloslav Trmac wrote:
> The attached patch:
> * Modifies ausearch and libauparse to output the file format in octal if it
> is unknown. * Modifies libauparse to use the same interpreted field format
> as ausearch (without a space in the middle). * Modifies comma handling in
> libauparse to avoid a strcat() call.

Applied.

-Steve

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[Miloslav Trmac]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

----- "LC Bruzenak" <lenny@magitekltd.com> wrote:
> Thank you for this patch...wherever it may be.
> :)
Ooops :/


> Do you have a standard auparse test you use to track these down?
No, I only have a small Python program to use auparse to interpret a supplied log file (attached).  There is also (make check).
    Mirek

[-- Attachment #2: audit-interpret.py --]
[-- Type: application/octet-stream, Size: 1409 bytes --]

#! /usr/bin/python
import sys

import auparse
import audit

def none_to_null(s):
    if s is None:
        return '(null)'
    else:
        return s

def walk_test(au):
    au.reset()
    while True:
        if not au.first_record():
            print "Error getting first record"
            sys.exit(1)

        print "%d records:" % (au.get_num_records(),)

        while True:
            print "    raw: %s" % (none_to_null(au.get_record_text()))
            print "    type %d(%s) has %d fields" % \
                  (au.get_type(), audit.audit_msg_type_to_name(au.get_type()),
                   au.get_num_fields())
            print "    line=%d file=%s" % (au.get_line_number(), au.get_filename())
            event = au.get_timestamp()
            if event is None:
                print "Error getting timestamp - aborting"
                sys.exit(1)

            print "    event time: %d.%d:%d, host=%s" % (event.sec, event.milli, event.serial, none_to_null(event.host))
            au.first_field()
            while True:
                print "        %s=%s (%s)" % (au.get_field_name(), au.get_field_str(), au.interpret_field())
                if not au.next_field(): break
            print
            if not au.next_record(): break
        if not au.parse_next_event(): break

if __name__ == '__main__':
    au = auparse.AuParser(auparse.AUSOURCE_FILE, sys.argv[1])
    walk_test(au)

[-- Attachment #3: audit-ifmt.patch --]
[-- Type: application/octet-stream, Size: 2157 bytes --]

Index: src/ausearch-report.c
===================================================================
--- src/ausearch-report.c	(revision 268)
+++ src/ausearch-report.c	(working copy)
@@ -548,6 +548,7 @@
 
 static void print_mode(const char *val)
 {
+	const char *name;
 	unsigned int ival;
 
 	errno = 0;
@@ -558,8 +559,17 @@
 	}
 
 	// print the file type
-	printf("%s,", audit_ftype_to_name(ival & S_IFMT));
+	name = audit_ftype_to_name(ival & S_IFMT);
+	if (name != NULL)
+		printf("%s,", name);
+	else {
+		unsigned first_ifmt_bit;
 
+		// The lowest-valued "1" bit in S_IFMT
+		first_ifmt_bit = S_IFMT & ~(S_IFMT - 1);
+		printf("%03o,", (ival & S_IFMT) / first_ifmt_bit);
+	}
+
 	// check on special bits
 	if (S_ISUID & ival)
 		printf("suid,");
Index: auparse/interpret.c
===================================================================
--- auparse/interpret.c	(revision 268)
+++ auparse/interpret.c	(working copy)
@@ -453,6 +453,7 @@
 {
         unsigned int ival;
 	char *out, buf[48];
+	const char *name;
 
         errno = 0;
         ival = strtoul(val, NULL, 8);
@@ -461,22 +462,28 @@
                 return out;
         }
 
-	buf[0] = 0;
+        // detect the file type
+	name = audit_ftype_to_name(ival & S_IFMT);
+	if (name != NULL)
+		strcpy(buf, name);
+	else {
+		unsigned first_ifmt_bit;
 
-        // detect tthe file type
-        strcat(buf, audit_ftype_to_name(ival & S_IFMT));
-	strcat(buf, ",");
+		// The lowest-valued "1" bit in S_IFMT
+		first_ifmt_bit = S_IFMT & ~(S_IFMT - 1);
+		sprintf(buf, "%03o", (ival & S_IFMT) / first_ifmt_bit);
+	}
 
         // check on special bits
         if (S_ISUID & ival)
-                strcat(buf, "suid,");
+                strcat(buf, ",suid");
         if (S_ISGID & ival)
-                strcat(buf, "sgid,");
+                strcat(buf, ",sgid");
         if (S_ISVTX & ival)
-                strcat(buf, "sticky,");
+                strcat(buf, ",sticky");
 
 	// and the read, write, execute flags in octal
-        asprintf(&out, "%s %03o",  buf, (S_IRWXU|S_IRWXG|S_IRWXO) & ival);
+        asprintf(&out, "%s,%03o",  buf, (S_IRWXU|S_IRWXG|S_IRWXO) & ival);
 	return out;
 }
 

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

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[LC Bruzenak]

Mirek,

After applying this patch my build fails in the parse test section due
to a difference of no space after a comma:

diff -u ../../auparse/test/auparse_test.ref auparse_test.cur
--- ../../auparse/test/auparse_test.ref 2009-02-24 15:11:35.000000000
-0600
+++ auparse_test.cur    2009-03-27 10:21:34.000000000 -0500
...
-        mode=040730 (dir, 730)
+        mode=040730 (dir,730)

I have made some local changes myself but I don't think it is me (which
of course probably means it IS).

Do you think your changes would cause this?

Thx,
LCB.

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[Miloslav Trmac]

----- "LC Bruzenak" <lenny@magitekltd.com> wrote:
> After applying this patch my build fails in the parse test section due
> to a difference of no space after a comma:
> 
> -        mode=040730 (dir, 730)
> +        mode=040730 (dir,730)
> 
> Do you think your changes would cause this?
Yes, that change was intentional and documented in the patch.  I forgot to run (make check) and update the test case.
    Mirek

Re: [PATCH] Don't crash on unknown S_IFMT file modes

[LC Bruzenak]

On Fri, 2009-03-27 at 10:44 -0500, LC Bruzenak wrote:
> Mirek,
> 
> After applying this patch my build fails in the parse test section due
> to a difference of no space after a comma:
> 
> diff -u ../../auparse/test/auparse_test.ref auparse_test.cur
> --- ../../auparse/test/auparse_test.ref 2009-02-24 15:11:35.000000000
> -0600
> +++ auparse_test.cur    2009-03-27 10:21:34.000000000 -0500
> ...
> -        mode=040730 (dir, 730)
> +        mode=040730 (dir,730)
> 
> I have made some local changes myself but I don't think it is me (which
> of course probably means it IS).
> 
> Do you think your changes would cause this?
> 
> Thx,
> LCB.
> 

This change fixed it for me - adding a space. You likely want to apply
the same fix, or if you like this formatting, change the checked
reference output.

 	// and the read, write, execute flags in octal
-        asprintf(&out, "%s %03o",  buf, (S_IRWXU|S_IRWXG|S_IRWXO) &
ival);
+        asprintf(&out, "%s, %03o",  buf, (S_IRWXU|S_IRWXG|S_IRWXO) &
ival);

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com