[Snort-devel] Re: Results from audit of the Output-plugins directory

Martin Roesch roesch at ...402...
Mon Sep 29 15:41:07 EDT 2003


Ok, a few comments:

1) All calls to calloc/malloc should be deprecated in favor of calls to 
SnortAlloc() which does return value checking.

2) spo_alert_syslog should use ErrorMessage, the plugin was probably 
implemented prior to the *Message functionality and not updated as a 
guess.

Most of the rest looks like basic cleanup.  Thanks for the audit Steve, 
we appreciate it!

      -Marty

On Saturday, September 27, 2003, at 10:01 AM, Steve G wrote:

> Hello,
>
> I've decided to go over snort with a fine toothed comb and review
> all the source code. The following is the results of a much more
> detailed review of the code. I have included the relevant
> comments from the lite/quick audit I did earlier this week. The
> review is against snort-2.0.2 and limited only to the
> output-plugins directory.
>
> The intention is not to attack this project or anyone...its to
> help point out some issues in the code that may need attention.
> Please take it in the spirit that its offered. :)
>
> -Steve Grubb
>
> ========================
>
> output-plugins/spo_alert_fast.c:
> 95: K&R style function
> 242: return value of calloc not checked
>
> output-plugins/spo_alert_full.c:
> 84: K&R style function
> 238: return value of calloc not checked
>
> output-plugins/spo_alert_sf_socket.c:
> 69: K&R style function
> 76: K&R style function
> 97: == is used to compare strings. "if (sockname[0] == 0)" is
> better.
> 112: Why is sun_path+1 used as the target for the socket_name?
> 112: The string's length is used for the copy rather than
> limiting to the sizeof(sun_path)-1
> 112: No NULL terminator is guaranteed for the string since memcpy
> was used.
> 133: String format specified, no arguments
> 150: unsigned int format specified, long unsigned int passed
>
> output-plugins/spo_alert_smb.c
> 90: K&R style function
> 150: return value of malloc not checked
> 267: bzero is using magic number for size; sizeof() is preferred
> 286: full path name to smbclient should be used. path should come
> from config or trusted directories like /usr/bin
> 299: bzero not needed
> 300: bzero not needed
>
> output-plugins/spo_alert_syslog.c
> 97: K&R style function
> 468: Why does error message go to stderr? Should it use
> LogMessage?
>
> output-plugins/spo_alert_unixsock.c:
> 90: K&R style prototype declared
> 105: K&R style function
> 278: The string's length is used rather than limiting to the
> sizeof(sun_path) -1
> 278: Terminating NULL character not guaranteed since bcopy is
> used.
>
> output-plugins/spo_csv.c:
> 104: K&R style function
> 165: return value of malloc not checked
>
> output-plugins/spo_database.c:
> 188: Constant MYSQL was previously defined in mysql.h line 175
> 207: K&R prototype
> 217: K&R protoype
> 218: K&R prototype
> 222: pv already declared in snort.h
> 441: return value of calloc not checked
> 557: return value of calloc not checked
> 668: assignment has no purpose - delete line
> 778: return value of malloc not checked
> 781: return value of malloc not checked
> 869: return value of malloc not checked
> 931: return value of malloc not checked
> 941: return value of malloc not checked
> 957-9: return value of malloc not checked
> 1025-6: return value of malloc not checked
> 1051: return value of malloc not checked
> 1064: return value of malloc not checked
> 1070: return value of malloc not checked
> 1088: return value of malloc not checked
> 1098: String format specified, ReferenceNode was passed. sb
> refNode->system->name ?
> 1488: return value of malloc not checked
> 1514: return value of malloc not checked
> 1539: return value of malloc not checked
> 1651: return value of malloc not checked
>
> spo_log_ascii.c:
> 89: K&R style function
> 173: static missing from function definition
> General comment...code should have 'const' added for literal
> strings. e.g.- const char *IcmpFileName(const Packet * p).
>
> spo_log_null.c:
> 39: #include "config.h" is missing
> 69: K&R style function
>
> spo_log_tcpdump.c:
> 89: dumpd is never used by the module
> 90: pv already declared in snort.h
> 107: K&R style function
> 169: return value of calloc not checked
> 255: filename is used without checking for NULL. filename came
> from strdup.
> General comment...the code in the file bzero's memory before
> freeing it. Other modules do not. It seems inconsistent.
>
> spo_unified.c:
> 130: thiszone already declared in snort.h
> 173: UnifiedRotateFile was already declared on line 166
> 203: K&R style function
> 225: static missing from function definition
> 303: static missing from function definition
> 343: static missing from function definition
> 353: static missing from function definition
> 429: static missing from function definition
> 440: static missing from function definition
> 537: static missing from function definition
> 561: symbol clash, index is a function name
> 613: limit is a signed number. shifting left will lose the sign.
> Should limit really be a signed or unsigned number?
> 670: static missing from function definition
> 698: static missing from function definition
> 744: static missing from function definition
> 749: static missing from function definition
> 759: static missing from function definition
> 793: static missing from function definition
> 829: UnifiedLogFileHeader declares timezone as u_int32_t,
> thiszone is declared as int. Can they not be the same type?
> 830: UnifiedLogFileHeader declares linktype as u_int32_t,
> elsewhere datalink is declared as int. Can they not be the same
> type?
> 856: static missing from function definition
> 928: a blank line is needed.
> 928: static missing from function definition.
>
> __________________________________
> Do you Yahoo!?
> The New Yahoo! Shopping - with improved product search
> http://shopping.yahoo.com
>
>
-- 
Martin Roesch - Founder/CTO, Sourcefire Inc. - (410)290-1616
Sourcefire: Snort-based Enterprise Intrusion Detection Infrastructure
roesch at ...402... - http://www.sourcefire.com
Snort: Open Source Network IDS - http://www.snort.org





More information about the Snort-devel mailing list