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

Steve G linux_4ever at ...398...
Sat Sep 27 07:02:19 EDT 2003


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




More information about the Snort-devel mailing list