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

From: Steve G <linux_4ever(at)yahoo.com>
Date: Tue Sep 30 2003 - 17:55:37 EDT

Hello,

I am continuing to go through the snort code. I have included the relevant comments from the lite/quick audit I did last week. The review is against snort-2.0.2 and limited only to the detection-plugins directory.

A general comment, the use of the xor ^ is unusual. The result of a boolean test is xor'ed with a 0 or a 1 to fire off a rule. While it may work, its unusual. I think its worth adding some words to help people understand it.

Most of this is code cleanup, but there are a couple of bugs. strtok is used on the data parameter. strtok destroys the data passed to it. If the data buffer is used for any other plugins, it will be destroyed. strtok should be used on a copy of data. I think some plugins do this.

Also, sp_rpc_check.c, tmp is not initialized if *data == *. It is used potentially uninitialized.

-Steve Grubb

---

General Comment for this directory:
the char *data parameter in most function calls should be const.

sp_byte_check.c:
73: #include "config.h" is missing

105: file_name is already declared in parser.h
108: file_line is already declared in parser.h
142: K&R style function
224: idx->bytes_to_compare is uint32, strtol is called. strtoul

 would be a better fit. Also, if the data has a real posibility of being negative, it is advisable to catch it in a signed variable and test for a negative value.
224: errno should be cleared before this function call and checked after the function call. From SUSv3. 265: Character format specified, pointer passed. 285: errno should be cleared before this function call and checked after the function call. From SUSv3. 285: idx->offset is uint32, strtol is called. strtoul would be a better fit. Also, if the data has a real posibility of being negative, it is advisable to catch it in a signed variable and test for a negative value.
375: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.
498: TEXTLEN macro defined at 103 is not referenced.

sp_byte_jump.c:

47: #include "config.h" is missing
67: file_name is already declared in parser.h
70: file_line is already declared in parser.h

103: K&R style function
185: errno should be cleared before this function call and checked after the function call. From SUSv3. 200: idx->offset is uint32, strtol is called. strtoul would be a better fit. Also, if the data has a real posibility of being negative, it is advisable to catch it in a signed variable and test for a negative value.
200: errno should be cleared before this function call and checked after the function call. From SUSv3. 292: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.
417: plugin_enum.h is not used by this module.

sp_clientserver.c:
9: spelling - elminate
29: #include "config.h" is missing
76: K&R style function
290, 331, 364, 390: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.

sp_dsize_check.c:
65: K&R style function
139: strtok is used on data instead of a copy of data. 139, 140, 166: atoi is a deprecated function. strtol is recommended.
185, 225, 265, 303: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder. 194, 233, 273, 311, 312: signed/unsigned in the comparison. Should dsize be uint32? Is a negative value likely or possible? 324: Macros EQ, GT, & LT are never used. They are declared starting at 36.

sp_icmp_id_check.c :
41: #include "config.h" is missing
80: K&R style function
156: atoi is a deprecated function. strtol is recommended. 156: Should any range checking be done before converting it to short?
176: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.

sp_icmp_seq_check.c:
41: #include "config.h" is missing
56: unsigned short used here, sp_icmp_id_check.c used u_short. There is a consistency issue here.
80: K&R style function
155: atoi is a deprecated function. strtol is recommended. 155: Should any range checking be done before converting it to short?
175: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.

sp_icmp_type_check.c:
20: #include "config.h" is missing
52: K&R style function

117: type is never used - delete
125: type is never used - delete
129: Several format specifiers in format string. No arguments

passed.
138: Several format specifiers in format string. No arguments passed.

146: atoi is a deprecated function. strtol is recommended.
149: return is unnecessary. There's no other code to execute.
173: Packet *p  & _OptTreeNode *otn, should be const to assure

that nothing is done to alter contents of the packet before passing to the next decoder.
179: signed/unsigned in the comparison. Should icmp_type be uint32? Is a negative value likely or possible?

sp_ip_fragbits.c:
43: #include "config.h" is missing
112: K&R style function
264: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.

350: K&R style function
375: Return value of calloc not checked.
444: atoi is a deprecated function. strtoul is recommended.
465: Packet *p  & _OptTreeNode *otn, should be const to assure

that nothing is done to alter contents of the packet before passing to the next decoder.
490: This line has an unusual construct. An exclusive or operator is used against the results of a boolean test. It may be correct, if so I would encourage some comments so other know why an XOR is being used.
504, 511: Signed unsigned in comparison. offset is unit16, should p_offset also be uint16? Is there any possibility the math at 468 could overflow a uint16?

sp_ip_id_check.c:
20: #include "config.h" is missing
55: K&R style function

129: atoi is a deprecated function. strtoul is recommended.
146: comment says void function?
149: Packet *p  & _OptTreeNode *otn, should be const to assure

that nothing is done to alter contents of the packet before passing to the next decoder.

sp_ipoption_check.c:
61: K&R style function

131: Format string has no arguments specified. Two are passed.
207: comment says void function?
210: Packet *p  & _OptTreeNode *otn, should be const to assure

that nothing is done to alter contents of the packet before passing to the next decoder.

sp_ip_proto.c
63: K&R style function
91: Commented out code. Should it be deleted?

101: Code commented out with C++ style comments
161: atoi is a deprecated function. strtoul is recommended.
194: Packet *p  & _OptTreeNode *otn, should be const to assure

that nothing is done to alter contents of the packet before passing to the next decoder.

199: Code commented out with C++ style comments
210: magic number, Should NOT_FLAG be defined and used here?
211: This line has an unusual construct. An exclusive or operator

is used against the results of a boolean test. It may be correct, if so I would encourage some comments so other know why an XOR is being used.

sp_ip_same_check.c:
40: Data structure not used - delete.
62: K&R style function
128: Unreached code. Comments indicate something is wrong with the
function If the Same IP option is working, I'd recommend deleting the code
152: comment says void function?
155: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.

sp_ip_tos_check.c:
66: K&R style function
144: This parser seems awkward. TTL appears to have the same syntax, so shouldn't they be closer in coding? 146: atoi is a deprecated function. strtoul is recommended. 152: errno should be cleared before calling strtol and checked upon return
175: Comment says void function
178: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.
185: This line has an unusual construct. An exclusive or operator is used against the results of a boolean test. It may be correct, if so I would encourage some comments so other know why an XOR is being used.

sp_pattern_match.c:

20: config.h should be the first include.
63: function is never defined - delete line.
73: file_name is already declared in parser.h
74: line_number is already declared in parser.h
77: K&R style function
166: This function does nothing. No matter what, it returns 0.
169: Commented out code
430: static missing from function definition
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
456: static missing from function definition
497: static missing from function definition
531: static missing from function definition
572: static missing from function definition
605: static missing from function definition
625: static missing from function definition
675: static missing from function definition
725: static missing from function definition
728: i is not used - delete.
746: assigned value is never used - delete line.

874: sizeof() is preferred of magic number 933: Use of bzero & memset is not as efficient as strncpy(hex_buf, "00", 3)
1256: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.
1333: frazes_count should be moved inside the #ifdef DEBUG

sp_react.c
51: #include "config.h" is missing
103: K&R style function
183-188: buffers should be marked const
200: strtok is called on data instead of a copy of data. 224: errno should be cleared before calling strtoul and checked afterwards.
236: proxy_port_nr is checked for < 0, however it was assigned using strtoul. Can this really be negative? Should proxy_port_nr be an unsigned number, too.

sp_respond.c:
33: #include "config.h" is missing
66, 67: K&R style function declaration

137: return is not needed
148: Comment says void function
166: strtok is called on type which is an alias of data.
228: K&R style function
248: K&R style function
278: Comment says void function

sp_rpc_check.c:
20: #include "config.h" is missing
67: K&R style function
147, 167: errno should be cleared prior to calling strtoul & checked after
151: tmp is not guranteed to be initialized ! 186: Packet *p & _OptTreeNode *otn, should be const to assure that nothing is done to alter contents of the packet before passing to the next decoder.
190: xid is never used and can be deleted 233: xid is assigned a value that is never used.

sp_session.c:

104: K&R style function
129: Spelling all sb allow
134: commented out code
256: add const to conv

324,330,334,342,348,352: snprintf should be used.

sp_tcp_ack_check.c:
20: #include "config.h" is missing
56: K&R style function
129: errno should be cleared prior to calling strtoul & checked after
146: comment says void function

sp_tcp_flag_check.c:
20: #include "config.h" is missing
53: K&R style function

sp_tcp_seq_check.c:
20 #include "config.h" is missing
58: K&R style function
131: errno should be cleared prior to calling strtoul & checked after
149: comment says void function

sp_tcp_win_check.c:
67: K&R style function
153: atoi is a deprecated function. strtoul is recommended. 159, 163: errno should be cleared prior to calling strtol & checked after
186: comment says void function
196: This line has an unusual construct. An exclusive or operator is used against the results of a boolean test. It may be correct, if so I would encourage some comments so other know why an XOR is being used.

sp_ttl_check.c:
20: #include "config.h" is missing
60: K&R style function
144, 167,173,179: atoi is a deprecated function. strtol is recommended.

---

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 Tue Sep 30 18:03:26 2003