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

Steve G linux_4ever at ...398...
Tue Sep 30 14:56:06 EDT 2003


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
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




More information about the Snort-devel mailing list