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

From: Steve G <linux_4ever(at)yahoo.com>
Date: Sat Sep 27 2003 - 10:01:01 EDT

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.
Can we help you?
In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software:
Learn more about Pantek's Technical Support offerings
Place an order for 24/7/365 Technical Support here
Learn more about Pantek, who we are, and what we do
Chat with a Live Operator
Related snort.org Links
snort-announce
snort-cvsinfo
snort-devel
snort-sigs
snort-users
Request more information about Pantek

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

---

This sf.net email is sponsored by:ThinkGeek Welcome to geek heaven.
http://thinkgeek.com/sf

---

Snort-devel mailing list
Snort-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/snort-devel Received on Sat Sep 27 10:09:46 2003