Re: [PATCH] prelude plugin for specter

Le mercredi 07 février 2007 à 08:43 +0100, Maik Hentsche a écrit : 
> Note: This is a resend, because a network problem caused my mailserver
> to send the last version of this mail without the patch.
> 
> Maik Hentsche <[email protected]> wrote:
> 
> > Hi,
> > I'm currently working on getting firewall (netfilter/iptables)
> > logmessages into prelude. For this is wrote a plugin for specter
> > (http://joker.linuxstuff.pl/specter/), a daemon, that handles
> > logging through the ULOG target (as ulogd does, if anyone knows
> > that).

> I've been away from prelude development longer than I have wished to
> but now I am proud to present an improved version of my patch. Find it
> attached. I took all suggestion I got for the first version of my
> patch and I feel that it is much improved now. I'd appreciate if you
> could give it a try and tell me what you think about it. All
> suggestions, criticisms and suggestions are valuable. 
> 
> Even though I think the plugin is usable as it is, I'm planning for
> some improvements in the future. One of this is a way for the
> administrator to influence the verboseness of the syslog mode.
> Furthermore, I'm not sure if it is possible for some fields like IP
> addresses to have NULL pointers if pred_init() returned successfully.
> I'm going to evaluate this. 

Hi Maik,

First, let me thank you for your contribution effort. Here is a report
from a first code review. You will find information concerning both the
code and the generated alert.

I was planning to do a complete review of your code, but would not be
able to do so with a decent deadline since my time is limited. So I
guess it's better to provide you with what I currently have, then follow
up latter when you'll have corrected the first issues.


- Rather than defining manually the size for specter_local_ret_t array
of structure, which may lead to error when adding new member, I'd
suggest to use:

#define XXX_PRED_NUM (sizeof(pred_xxx) / sizeof(*pred_xxx))

- A static buffer is used to hold generated string. The string are
generated in sub-function that don't know the size of the buffer. To
avoid potential problem, using snprintf() would be advised. Even better
would be to use the prelude-string implementation.

- Your add_idmef_object() function take a value parameter that you
generate prior each call to this function using sprintf(). The code
would probably look better if you used an implementation like:

static int add_idmef_object(idmef_message_t *message, const char *object, const char *fmt, ...)
{
       int ret;
       va_list ap;
       idmef_value_t *val;
       idmef_path_t *path;
       prelude_string_t *buf;
               
       ret = idmef_path_new(&path, object);
       if ( ret < 0 )
               return ret;
       
       ret = prelude_string_new(&buf);
       if ( ret < 0 ) {
                idmef_path_destroy(path);
                return ret;
       }
       
       va_start(ap, fmt);
       ret = prelude_string_vprintf(buf, fmt, ap);
       va_end(ap);
       
       if ( ret < 0 )
                goto err;
       
       ret = idmef_value_new_from_path(&val, path, prelude_string_get_string(buf));
       if ( ret < 0 )
                goto err;
       
       ret = idmef_path_set(path, message, val);
       
  err:
       prelude_string_destroy(buf);
       idmef_value_destroy(val);
       idmef_path_destroy(path);
        
       return ret;
}



- In some place, you Capitalize IDMEF object name, for example:
alert.source(0).node.Address(0).category. Although this is accepted by
the upstream code, you should use lower case for any path.

- I'd suggest using size_t in place of int to hold s(n)printf return
value.

- You should propagate error return and use error reporting function to
handle prelude error (prelude_perror(), prelude_strerror(), etc.).

- You should probably change the "prelude_name" option to "profile",
since this is what is used by most other product. The "prelude_name" is
misleading (this really is a profile).

- Since you fill a lot of IP headers fields, you should consider using
the same AdditionalData meanings that Snort use. This will enable
Prewikka to show the user a graphical representation of the packets.

- A consensus for the analyzer manufacturer field is to put the URL for
the project (in this case the URL for the specter project).

- The "Log Analyzer" class doesn't seem to fit well for Specter. Is it
really a log analyzer? What about using "Log Daemon"?

- You should add an Assessment class to the generated message.

- Classification name should probably be simplified and normalized, you
can, move all the details to impact.description. 

- Please check that the generated IDMEF messages validate, using the
xmlmod reporting plugin combined with it's DTD validation option. I
guess it currently won't, since the message doesn't carry any assessment
information.

Hope this help,
Regards,

-- 
Yoann Vandoorselaere | Responsable R&D / CTO | PreludeIDS Technologies
Tel: +33 (0)8 70 70 21 58                  Fax: +33(0)4 78 42 21 58
http://www.prelude-ids.com

_______________________________________________
Prelude-devel site list
[email protected]
http://www.prelude-ids.org/mailman/listinfo/prelude-devel