diff -urNp x/agent/snmp_agent.c y/agent/snmp_agent.c --- x/agent/snmp_agent.c 2019-09-12 10:07:52.536297633 +0200 +++ y/agent/snmp_agent.c 2019-09-12 10:10:10.900666975 +0200 @@ -1428,6 +1428,13 @@ free_agent_snmp_session(netsnmp_agent_se DEBUGMSGTL(("verbose:asp", "asp %p reqinfo %p freed\n", asp, asp->reqinfo)); + + /* Clean up securityStateRef here to prevent a double free */ + if (asp->orig_pdu && asp->orig_pdu->securityStateRef) + snmp_free_securityStateRef(asp->orig_pdu); + if (asp->pdu && asp->pdu->securityStateRef) + snmp_free_securityStateRef(asp->pdu); + if (asp->orig_pdu) snmp_free_pdu(asp->orig_pdu); if (asp->pdu) diff -urNp x/include/net-snmp/pdu_api.h y/include/net-snmp/pdu_api.h --- x/include/net-snmp/pdu_api.h 2019-09-12 10:07:52.548297751 +0200 +++ y/include/net-snmp/pdu_api.h 2019-09-12 10:11:26.562411701 +0200 @@ -19,6 +19,8 @@ NETSNMP_IMPORT netsnmp_pdu *snmp_fix_pdu( netsnmp_pdu *pdu, int idx); NETSNMP_IMPORT void snmp_free_pdu( netsnmp_pdu *pdu); +NETSNMP_IMPORT +void snmp_free_securityStateRef( netsnmp_pdu *pdu); #ifdef __cplusplus } diff -urNp x/snmplib/snmp_api.c y/snmplib/snmp_api.c --- x/snmplib/snmp_api.c 2019-09-12 10:07:52.570297968 +0200 +++ y/snmplib/snmp_api.c 2019-09-13 08:53:53.734689426 +0200 @@ -3837,6 +3837,42 @@ snmpv3_parse(netsnmp_pdu *pdu, return SNMPERR_SUCCESS; } /* end snmpv3_parse() */ +static void +free_securityStateRef(netsnmp_pdu* pdu) +{ + struct snmp_secmod_def *sptr; + + if(!pdu->securityStateRef) + return; + + sptr = find_sec_mod(pdu->securityModel); + if (sptr) { + if (sptr->pdu_free_state_ref) { + (*sptr->pdu_free_state_ref) (pdu->securityStateRef); + } else { + snmp_log(LOG_ERR, + "Security Model %d can't free state references\n", + pdu->securityModel); + } + } else { + snmp_log(LOG_ERR, + "Can't find security model to free ptr: %d\n", + pdu->securityModel); + } + pdu->securityStateRef = NULL; +} + +/* + * This function is here to provide a separate call to + * free the securityStateRef memory. This is needed to prevent + * a double free if this memory is freed in snmp_free_pdu. + */ +void +snmp_free_securityStateRef(netsnmp_pdu* pdu) +{ + free_securityStateRef(pdu); +} + #define ERROR_STAT_LENGTH 11 int @@ -3858,7 +3894,6 @@ snmpv3_make_report(netsnmp_pdu *pdu, int oid *err_var; int err_var_len; int stat_ind; - struct snmp_secmod_def *sptr; switch (error) { case SNMPERR_USM_UNKNOWNENGINEID: @@ -3919,21 +3954,7 @@ snmpv3_make_report(netsnmp_pdu *pdu, int * which cached values to use */ if (pdu->securityStateRef) { - sptr = find_sec_mod(pdu->securityModel); - if (sptr) { - if (sptr->pdu_free_state_ref) { - (*sptr->pdu_free_state_ref) (pdu->securityStateRef); - } else { - snmp_log(LOG_ERR, - "Security Model %d can't free state references\n", - pdu->securityModel); - } - } else { - snmp_log(LOG_ERR, - "Can't find security model to free ptr: %d\n", - pdu->securityModel); - } - pdu->securityStateRef = NULL; + free_securityStateRef(pdu); } if (error == SNMPERR_USM_NOTINTIMEWINDOW) { @@ -5192,7 +5213,6 @@ _sess_process_packet(void *sessp, netsnm struct session_list *slp = (struct session_list *) sessp; netsnmp_pdu *pdu; netsnmp_request_list *rp, *orp = NULL; - struct snmp_secmod_def *sptr; int ret = 0, handled = 0; DEBUGMSGTL(("sess_process_packet", @@ -5262,21 +5282,7 @@ _sess_process_packet(void *sessp, netsnm * Call the security model to free any securityStateRef supplied w/ msg. */ if (pdu->securityStateRef != NULL) { - sptr = find_sec_mod(pdu->securityModel); - if (sptr != NULL) { - if (sptr->pdu_free_state_ref != NULL) { - (*sptr->pdu_free_state_ref) (pdu->securityStateRef); - } else { - snmp_log(LOG_ERR, - "Security Model %d can't free state references\n", - pdu->securityModel); - } - } else { - snmp_log(LOG_ERR, - "Can't find security model to free ptr: %d\n", - pdu->securityModel); - } - pdu->securityStateRef = NULL; + free_securityStateRef(pdu); } snmp_free_pdu(pdu); return -1; @@ -5287,21 +5293,7 @@ _sess_process_packet(void *sessp, netsnm * Call USM to free any securityStateRef supplied with the message. */ if (pdu->securityStateRef) { - sptr = find_sec_mod(pdu->securityModel); - if (sptr) { - if (sptr->pdu_free_state_ref) { - (*sptr->pdu_free_state_ref) (pdu->securityStateRef); - } else { - snmp_log(LOG_ERR, - "Security Model %d can't free state references\n", - pdu->securityModel); - } - } else { - snmp_log(LOG_ERR, - "Can't find security model to free ptr: %d\n", - pdu->securityModel); - } - pdu->securityStateRef = NULL; + free_securityStateRef(pdu); } for (rp = isp->requests; rp; orp = rp, rp = rp->next_request) { @@ -5454,21 +5446,7 @@ _sess_process_packet(void *sessp, netsnm */ if (pdu != NULL && pdu->securityStateRef && pdu->command == SNMP_MSG_TRAP2) { - sptr = find_sec_mod(pdu->securityModel); - if (sptr) { - if (sptr->pdu_free_state_ref) { - (*sptr->pdu_free_state_ref) (pdu->securityStateRef); - } else { - snmp_log(LOG_ERR, - "Security Model %d can't free state references\n", - pdu->securityModel); - } - } else { - snmp_log(LOG_ERR, - "Can't find security model to free ptr: %d\n", - pdu->securityModel); - } - pdu->securityStateRef = NULL; + free_securityStateRef(pdu); } if (!handled) { diff -urNp x/snmplib/snmpusm.c y/snmplib/snmpusm.c --- x/snmplib/snmpusm.c 2019-09-12 10:07:52.567297938 +0200 +++ y/snmplib/snmpusm.c 2019-09-12 10:57:52.780861077 +0200 @@ -206,16 +206,20 @@ usm_free_usmStateReference(void *old) if (old_ref) { - SNMP_FREE(old_ref->usr_name); - SNMP_FREE(old_ref->usr_engine_id); - SNMP_FREE(old_ref->usr_auth_protocol); - SNMP_FREE(old_ref->usr_priv_protocol); + if (old_ref->usr_name_length) + SNMP_FREE(old_ref->usr_name); + if (old_ref->usr_engine_id_length) + SNMP_FREE(old_ref->usr_engine_id); + if (old_ref->usr_auth_protocol_length) + SNMP_FREE(old_ref->usr_auth_protocol); + if (old_ref->usr_priv_protocol_length) + SNMP_FREE(old_ref->usr_priv_protocol); - if (old_ref->usr_auth_key) { + if (old_ref->usr_auth_key_length && old_ref->usr_auth_key) { SNMP_ZERO(old_ref->usr_auth_key, old_ref->usr_auth_key_length); SNMP_FREE(old_ref->usr_auth_key); } - if (old_ref->usr_priv_key) { + if (old_ref->usr_priv_key_length && old_ref->usr_priv_key) { SNMP_ZERO(old_ref->usr_priv_key, old_ref->usr_priv_key_length); SNMP_FREE(old_ref->usr_priv_key); } @@ -946,7 +950,6 @@ usm_generate_out_msg(int msgProcModel, if ((user = usm_get_user(secEngineID, secEngineIDLen, secName)) == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) { DEBUGMSGTL(("usm", "Unknown User(%s)\n", secName)); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_UNKNOWNSECURITYNAME; } @@ -998,7 +1001,6 @@ usm_generate_out_msg(int msgProcModel, thePrivProtocolLength) == 1) { DEBUGMSGTL(("usm", "Unsupported Security Level (%d)\n", theSecLevel)); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL; } @@ -1028,7 +1030,6 @@ usm_generate_out_msg(int msgProcModel, &msgAuthParmLen, &msgPrivParmLen, &otstlen, &seq_len, &msgSecParmLen) == -1) { DEBUGMSGTL(("usm", "Failed calculating offsets.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } @@ -1050,7 +1051,6 @@ usm_generate_out_msg(int msgProcModel, ptr = *wholeMsg = globalData; if (theTotalLength > *wholeMsgLen) { DEBUGMSGTL(("usm", "Message won't fit in buffer.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } @@ -1078,7 +1078,6 @@ usm_generate_out_msg(int msgProcModel, htonl(boots_uint), htonl(time_uint), &ptr[privParamsOffset]) == -1) { DEBUGMSGTL(("usm", "Can't set AES iv.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } } @@ -1091,7 +1090,6 @@ usm_generate_out_msg(int msgProcModel, &ptr[privParamsOffset]) == -1)) { DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } } @@ -1104,7 +1102,6 @@ usm_generate_out_msg(int msgProcModel, &ptr[dataOffset], &encrypted_length) != SNMP_ERR_NOERROR) { DEBUGMSGTL(("usm", "encryption error.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_ENCRYPTIONERROR; } #ifdef NETSNMP_ENABLE_TESTING_CODE @@ -1132,7 +1129,6 @@ usm_generate_out_msg(int msgProcModel, if ((encrypted_length != (theTotalLength - dataOffset)) || (salt_length != msgPrivParmLen)) { DEBUGMSGTL(("usm", "encryption length error.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_ENCRYPTIONERROR; } @@ -1268,7 +1264,6 @@ usm_generate_out_msg(int msgProcModel, if (temp_sig == NULL) { DEBUGMSGTL(("usm", "Out of memory.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } @@ -1282,7 +1277,6 @@ usm_generate_out_msg(int msgProcModel, SNMP_ZERO(temp_sig, temp_sig_len); SNMP_FREE(temp_sig); DEBUGMSGTL(("usm", "Signing failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_AUTHENTICATIONFAILURE; } @@ -1290,7 +1284,6 @@ usm_generate_out_msg(int msgProcModel, SNMP_ZERO(temp_sig, temp_sig_len); SNMP_FREE(temp_sig); DEBUGMSGTL(("usm", "Signing lengths failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_AUTHENTICATIONFAILURE; } @@ -1304,7 +1297,6 @@ usm_generate_out_msg(int msgProcModel, /* * endif -- create keyed hash */ - usm_free_usmStateReference(secStateRef); DEBUGMSGTL(("usm", "USM processing completed.\n")); @@ -1458,7 +1450,6 @@ usm_rgenerate_out_msg(int msgProcModel, if ((user = usm_get_user(secEngineID, secEngineIDLen, secName)) == NULL && secLevel != SNMP_SEC_LEVEL_NOAUTH) { DEBUGMSGTL(("usm", "Unknown User\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_UNKNOWNSECURITYNAME; } @@ -1511,7 +1502,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGMSGTL(("usm", "Unsupported Security Level or type (%d)\n", theSecLevel)); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL; } @@ -1544,7 +1534,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGMSGTL(("usm", "couldn't malloc %d bytes for encrypted PDU\n", (int)ciphertextlen)); - usm_free_usmStateReference(secStateRef); return SNMPERR_MALLOC; } @@ -1560,7 +1549,6 @@ usm_rgenerate_out_msg(int msgProcModel, htonl(boots_uint), htonl(time_uint), iv) == -1) { DEBUGMSGTL(("usm", "Can't set AES iv.\n")); - usm_free_usmStateReference(secStateRef); SNMP_FREE(ciphertext); return SNMPERR_USM_GENERICERROR; } @@ -1575,7 +1563,6 @@ usm_rgenerate_out_msg(int msgProcModel, thePrivKeyLength - 8, iv) == -1)) { DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n")); - usm_free_usmStateReference(secStateRef); SNMP_FREE(ciphertext); return SNMPERR_USM_GENERICERROR; } @@ -1594,7 +1581,6 @@ usm_rgenerate_out_msg(int msgProcModel, scopedPdu, scopedPduLen, ciphertext, &ciphertextlen) != SNMP_ERR_NOERROR) { DEBUGMSGTL(("usm", "encryption error.\n")); - usm_free_usmStateReference(secStateRef); SNMP_FREE(ciphertext); return SNMPERR_USM_ENCRYPTIONERROR; } @@ -1614,7 +1600,6 @@ usm_rgenerate_out_msg(int msgProcModel, ciphertext, ciphertextlen); if (rc == 0) { DEBUGMSGTL(("usm", "Encryption failed.\n")); - usm_free_usmStateReference(secStateRef); SNMP_FREE(ciphertext); return SNMPERR_USM_ENCRYPTIONERROR; } @@ -1654,7 +1639,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGINDENTLESS(); if (rc == 0) { DEBUGMSGTL(("usm", "building privParams failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1675,7 +1659,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGINDENTLESS(); if (rc == 0) { DEBUGMSGTL(("usm", "building authParams failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1698,7 +1681,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGINDENTLESS(); if (rc == 0) { DEBUGMSGTL(("usm", "building authParams failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1714,7 +1696,6 @@ usm_rgenerate_out_msg(int msgProcModel, if (rc == 0) { DEBUGMSGTL(("usm", "building msgAuthoritativeEngineTime failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1730,7 +1711,6 @@ usm_rgenerate_out_msg(int msgProcModel, if (rc == 0) { DEBUGMSGTL(("usm", "building msgAuthoritativeEngineBoots failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1742,7 +1722,6 @@ usm_rgenerate_out_msg(int msgProcModel, DEBUGINDENTLESS(); if (rc == 0) { DEBUGMSGTL(("usm", "building msgAuthoritativeEngineID failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1755,7 +1734,6 @@ usm_rgenerate_out_msg(int msgProcModel, *offset - sp_offset); if (rc == 0) { DEBUGMSGTL(("usm", "building usm security parameters failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1769,7 +1747,6 @@ usm_rgenerate_out_msg(int msgProcModel, if (rc == 0) { DEBUGMSGTL(("usm", "building msgSecurityParameters failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1779,7 +1756,6 @@ usm_rgenerate_out_msg(int msgProcModel, while ((*wholeMsgLen - *offset) < globalDataLen) { if (!asn_realloc(wholeMsg, wholeMsgLen)) { DEBUGMSGTL(("usm", "building global data failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } } @@ -1795,7 +1771,6 @@ usm_rgenerate_out_msg(int msgProcModel, ASN_CONSTRUCTOR), *offset); if (rc == 0) { DEBUGMSGTL(("usm", "building master packet sequence failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_TOO_LONG; } @@ -1813,7 +1788,6 @@ usm_rgenerate_out_msg(int msgProcModel, if (temp_sig == NULL) { DEBUGMSGTL(("usm", "Out of memory.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_GENERICERROR; } @@ -1824,14 +1798,12 @@ usm_rgenerate_out_msg(int msgProcModel, != SNMP_ERR_NOERROR) { SNMP_FREE(temp_sig); DEBUGMSGTL(("usm", "Signing failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_AUTHENTICATIONFAILURE; } if (temp_sig_len != msgAuthParmLen) { SNMP_FREE(temp_sig); DEBUGMSGTL(("usm", "Signing lengths failed.\n")); - usm_free_usmStateReference(secStateRef); return SNMPERR_USM_AUTHENTICATIONFAILURE; } @@ -1842,7 +1814,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* * endif -- create keyed hash */ - usm_free_usmStateReference(secStateRef); DEBUGMSGTL(("usm", "USM processing completed.\n")); return SNMPERR_SUCCESS; } /* end usm_rgenerate_out_msg() */