2008/5/14 BOYA SUN <[EMAIL PROTECTED]>:
> In our very recent research, we applied
> inter-procedural code analysis and data mining technique on the version
> 5.3.2 of net-snmp project, and we have found 20 potential bugs, as indicated
> at the end of this email..... It would also be
> greately appreciated if you can reply to this email after you have gone over
> the bugs and confirmed any of them, since these information are really
> valuable for us for testing our current method.
OK - I've worked through this list, and most of the issues you have
identified are indeed problems worth fixing. I've applied suitable patches
to the active lines for some of these - others may need more attention (or
in some cases nothing needs to be done).
Looking at your bugs in turn:
> BUG#1: parse_config_logOption()
> 528: snmp_log_options( cptr, my_argc, my_argv );
Although this code does indeed ignore the return value,
the snmp_log_options() call will already have displayed
an error message to the user. There is nothing to be
gained from checking the return code here.
Not fixed.
> BUG#2:usm_parse_config_usmUser()
> 3605: uptr = usm_read_user(line);
> 3606: usm_add_user(uptr);
Confirmed - Potential SegV crash.
Fixed.
> BUG#3:apps/snmptrap.c:main():
> 329: pdu = snmp_pdu_create(inform ? SNMP_MSG_INFORM : SNMP_MSG_TRAP2);
> 342: snmp_add_var(pdu, ....);
> 350: if (snmp_add_var(pdu, ...) != 0) {
Confirmed - Potential SegV crash.
Fixed.
> BUG#4: netsnmp_cache_reqinfo_extract()
> 133: cache = netsnmp_cache_reqinfo_extract(…);
> 134: cinfo = (netsnmp_stash_cache_info *) cache->magic;
Confirmed - Potential SegV crash.
Fixed.
> BUG#5:netsnmp_request_get_list_data(),
> 803: netsnmp_table_iterator_helper_handler (……) {
> 698: ti_info = netsnmp_request_get_list_data(…);
> 701: if (!ti_info->results) {
Although this does appear to be a similar bug, in fact this
particular call cannot actually return NULL. Earlier in
the same routine, there's another block of code preparing
the handling of GETNEXT requests, which also retrieves
the list_data, and *does* check for NULL (and returns).
So if processing gets as far as the code marked, it's
guaranteed that the get_list_data() call will succeed.
It might be worth checking anyway, and logging this as
an impossible error, but I'll leave that for input from the
other developers.
Not Fixed.
> BUG#6:_mfd_ipSystemStatsTable_post_request()
> 570: packet_rc = netsnmp_check_all_requests_error(agtreq_info->asp, 0);
> 571: rc = ipSystemStatsTable_post_request(ipSystemStatsTable_if_ctx.
> 572: user_ctx, packet_rc);
Pass. I think that you are probably correct, and the post_request()
call should simply pass back the error code that it received.
But this is automatically-generated code, so any fix should be applied
to the original code template, as well as any implementations based
on it.
It's also not my area of expertise, so I'll pass this one on to Robert.
Left Open
> BUG#7:register_oid_index()
> 726: if (subid != -1) {
> 728: res = register_oid_index(……);
> 731: } else
> 732" res = register_oid_index(……);
Strictly speaking, you are correct and the test_oid_register
routine will leak memory. However this code is part of an
optional testing block, which will only ever be used as a
standalone test application, and all memory will be released
when the program exits. So it's not worth worrying about.
Not Fixed.
> BUG#8,15: usm_generate_out_msg
> 1135: asn_build_string(...);
> 1121: cp = asn_build_int(…);
> 1128: cp = asn_build_int(…);
Strictly speaking, you are correct and this routine doesn't
check for failures at every individual step. However:
- the routine has already checked that there is room
to build the packet (which would be the cause of these
calls failing).
- it would be obvious if building the request failed.
- this particular routine is never actually used
(all requests are constructed using "reverse encoding"
i,e, the usm_rgenerate_out_msg() call instead)
Not Fixed
> BUG#9: metTrigger_run()
> 611: if ( entry->flags & MTE_TRIGGER_FLAG_VWILD ) {
> 612: netsnmp_query_walk( var, entry->session );
> ……
> 616: if ( n != SNMP_ERR_NOERROR ) {
> It is possible that programmers forgot to assign return value of
> netsnmp_query_walk() to a variable "n" on line 612.
Confirmed - this block (including line 614) do indeed fail to
assign the value of the internal requests to 'n'. Oops!
Fixed.
> BUG#10: extend_parse_config()
> 394: oid_len = MAX_OID_LEN - 2;
> 395: read_objid( exec_name, oid_buf, &oid_len );
> 396: cptr = copy_nword(cptr, exec_name, sizeof(exec_name));
> File Name: /snmp/apps/snmptest.c:input_variable()
> 483: vp->val_len = MAX_OID_LEN;;
> 484: read_objid(buf, (oid *) value, &vp->val_len);
> 485: vp->val_len *= sizeof(oid);
Confirmed.
Fixed
> BUG#11:netsnmp_create_handler_registration()
> ... some callers of netsnmp_create_handler_registration() fails
> to check if the return value of netsnmp_create_handler_registration() is a
> null pointer.
netsnmp_create_handler_registration() is used in a large number of
places - normally as a parameter to some form of netsnmp_register_xxx()
call. It is almost never checked explicitly - if creating the registration
structure fails, then this will be detected by the core registration routine
(netsnmp_inject_handler_before())
It's not necessary to check this result for each module individually.
Not Fixed
> BUG#12: agentx_close_session()
> 171: (void) agentx_synch_response(ss, pdu, &response);
> However, the output of agentx_synch_response() is not
> checked in the function agentx_close_session().
Confirmed.
It doesn't really matter whether sending the close request
succeeds or not (since the session is being closed anyway).
But a successul request needs to release the response PDU
structure, which wasn't happening.
Fixed.
> BUG#13: usm_rgenerate_out_msg()
> 4495: asn_parse_string(…);
Ummm.... I'm not sure what you are referring to here.
The file 'snmplib/snmpusm.c' is less than 4000 lines long,
there is no mention of asn_parse_string in the routine
mentioned, and all uses of this call are checked.
Invalid Report
BUG#14: usm_rgenerate_out_msg()
> 1555: rc = asn_realloc_rbuild_string();
Confirmed.
I'm not sure whether this could actually happen, given the
size of the message buffer, but it's easy enough to check for.
Fixed
> BUG#15:
See above (Bug #8)
> BUG#16: snmp_pdu_parse()
> 4436: asn_parse_int();
> BUG#17: snmp_generate_out_msg
> 1103: asn_build_header();
> 1150: asn_build_header();
> 1171: asn_build_header();
> 1184: asn_build_header();
Strictly speaking, you are correct and these routine ought
to check the results of the various asn_xxx calls.
However these routines are fairly reliable, and unlikely
to fail. And if they do (which typically means out of memory),
then this will be picked up pretty fast when other mallocs
start failing too.
I'm going to continue being lazy here :-)
Not Fixed.
> BUG#18: handle_nsDebugTable()
> 419: debug_entry = (netsnmp_token_descr*)
> 420: netsnmp_extract_iterator_context(request);
> 421: debug_entry->enabled =
> 422: (*request->requestvb->val.integer == RS_ACTIVE);
Strictly speaking, this could in principle lead to a SegV crash.
However the design of the iterator helper is such that the
user-provided handler should only be called if there is a
valid row data structure to be retrieved.
So this problem Should Never Happen.
Not Fixed
> BUG#19: getbulk_table_entries()
> 852: sprint_realloc_objid();
I'm not immediately sure what the consequences of this call failing
would be. If the OID name is simply going to be displayed later on,
then there doesn't seem much point in checking this explicitly.
But this needs checking.
Left Open
> agent/agent_registry.c: dump_registry()
> 1760: sprint_realloc_objid(...);
Failing to check this conversion isn't too significant,
since this is simply part of some debug output.
But it's probably sensible to check it, and skip any
offending or malformed entries.
Fixed
> BUG#20: apps/snmpusm.c:main()
> 859: dhpdu = snmp_pdu_create(SNMP_MSG_GET);
> 862: snmp_add_null_var(dhpdu, usmDHParameters, usmDHParameters_len);
Confirmed - Potential SegV crash.
Fixed.
Thanks for these reports.
They've been very useful in cleaning up the Net-SNMP code.
And good luck with your continued research.
Dave
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
