[prev in list] [next in list] [prev in thread] [next in thread]
List: net-snmp-coders
Subject: The fix for usm_free_usmStateReference()
From: Hanh Nguyen <hanh.nguyen.t () gmail ! com>
Date: 2022-04-30 0:39:16
Message-ID: CACJJpxz6-wg-tQprF_A44RKAL0msQ3q=Zw5ThYFr+0mZhZy1Ug () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Hi,
I'm new to the net-snmp community. I'm looking for the changes for bug 2923
-Avoid that snmpv3 bulkget errors result in a double free
I downloaded net-snmp-5.9.1. The ChangeLog indicates that the fix is in
net-snmp-5.9.1. Sam Tannous posted the v3doublefree2.patch in the
discussion thread - https://sourceforge.net/p/net-snmp/bugs/search/?q=2923.
I checked the changes in the v3doublefree2.patch and the net-snmp-5.9.1
source code. They don't seem to match.
Any idea how I can find the commit for this fix?
Thanks in advance for any suggestions.
Hanh
[Attachment #5 (text/html)]
<div dir="ltr">Hi,<div><br><div>
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-as \
ian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures">I'm new to \
the net-snmp community. I'm looking for the changes for bug<span \
class="gmail-Apple-converted-space"> </span>2923 -Avoid that snmpv3 bulkget errors \
result in a double free</span></p> <p class="gmail-p2" \
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stre \
tch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0);min-height:13px"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures"></span><br></p> \
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-as \
ian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures">I downloaded \
net-snmp-5.9.1. The ChangeLog indicates that the fix is in net-snmp-5.9.1. Sam \
Tannous posted the v3doublefree2.patch in the discussion thread - <a \
href="https://sourceforge.net/p/net-snmp/bugs/search/?q=2923">https://sourceforge.net/p/net-snmp/bugs/search/?q=2923</a>.</span></p>
<p class="gmail-p2" \
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stre \
tch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0);min-height:13px"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures"></span><br></p> \
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-as \
ian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures">I checked the \
changes in the v3doublefree2.patch and the net-snmp-5.9.1 source code. They don't \
seem to match.</span></p> <p class="gmail-p2" \
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stre \
tch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0);min-height:13px"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures"></span><br></p> \
<p class="gmail-p1" style="margin:0px;font-variant-numeric:normal;font-variant-east-as \
ian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span \
class="gmail-s1" style="font-variant-ligatures:no-common-ligatures">Any idea how I \
can find the commit for this fix?</span></p><p class="gmail-p1" \
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stre \
tch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><br></p></div><div><p \
style="font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal; \
font-size:11px;line-height:normal;font-family:Menlo;margin:0px;color:rgb(0,0,0)"><span \
style="font-variant-ligatures:no-common-ligatures">Thanks in advance for any \
suggestions.</span></p><font color="#888888"><p \
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stre \
tch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span \
style="font-variant-ligatures:no-common-ligatures">Hanh</span></p></font></div></div></div>
--0000000000001489d405ddd46669--
["v3doublefree2.patch" (application/octet-stream)]
diff --git a/agent/snmp_agent.c b/agent/snmp_agent.c
index d6a56e0..596ad97 100644
--- a/agent/snmp_agent.c
+++ b/agent/snmp_agent.c
@@ -1604,6 +1604,13 @@ free_agent_snmp_session(netsnmp_agent_session *asp)
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 --git a/include/net-snmp/pdu_api.h b/include/net-snmp/pdu_api.h
index 125595d..270aff0 100644
--- a/include/net-snmp/pdu_api.h
+++ b/include/net-snmp/pdu_api.h
@@ -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 --git a/snmplib/snmp_api.c b/snmplib/snmp_api.c
index 6754fa6..48463a5 100644
--- a/snmplib/snmp_api.c
+++ b/snmplib/snmp_api.c
@@ -4008,7 +4008,7 @@ snmpv3_parse(netsnmp_pdu *pdu,
return SNMPERR_SUCCESS;
} /* end snmpv3_parse() */
-static void
+void
free_securityStateRef(netsnmp_pdu* pdu)
{
struct snmp_secmod_def *sptr = find_sec_mod(pdu->securityModel);
@@ -4028,6 +4028,17 @@ free_securityStateRef(netsnmp_pdu* pdu)
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
diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
index 3cfa126..873bd89 100644
--- a/snmplib/snmpusm.c
+++ b/snmplib/snmpusm.c
@@ -299,16 +299,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_auth_key) {
+ 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_auth_protocol_length)
+ SNMP_FREE(old_ref->usr_priv_protocol);
+
+ 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);
}
@@ -1039,7 +1043,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
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;
}
@@ -1091,7 +1094,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
thePrivProtocolLength) == 1) {
DEBUGMSGTL(("usm", "Unsupported Security Level (%d)\n",
theSecLevel));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
}
@@ -1121,7 +1123,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
&msgAuthParmLen, &msgPrivParmLen, &otstlen,
&seq_len, &msgSecParmLen) == -1) {
DEBUGMSGTL(("usm", "Failed calculating offsets.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_GENERICERROR;
}
@@ -1143,7 +1144,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
ptr = *wholeMsg = globalData;
if (theTotalLength > *wholeMsgLen) {
DEBUGMSGTL(("usm", "Message won't fit in buffer.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_GENERICERROR;
}
@@ -1169,7 +1169,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
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;
}
}
@@ -1185,7 +1184,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
&ptr[privParamsOffset])
== -1)) {
DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_GENERICERROR;
}
}
@@ -1198,7 +1196,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
&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
@@ -1226,7 +1223,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
if ((encrypted_length != (theTotalLength - dataOffset))
|| (salt_length != msgPrivParmLen)) {
DEBUGMSGTL(("usm", "encryption length error.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_ENCRYPTIONERROR;
}
@@ -1362,7 +1358,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
if (temp_sig == NULL) {
DEBUGMSGTL(("usm", "Out of memory.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_GENERICERROR;
}
@@ -1376,7 +1371,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
SNMP_ZERO(temp_sig, temp_sig_len);
SNMP_FREE(temp_sig);
DEBUGMSGTL(("usm", "Signing failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_AUTHENTICATIONFAILURE;
}
@@ -1384,7 +1378,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
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;
}
@@ -1398,7 +1391,6 @@ usm_generate_out_msg(int msgProcModel, /* (UNUSED) */
/*
* endif -- create keyed hash
*/
- usm_free_usmStateReference(secStateRef);
DEBUGMSGTL(("usm", "USM processing completed.\n"));
@@ -1548,7 +1540,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
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;
}
@@ -1601,7 +1592,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGMSGTL(("usm", "Unsupported Security Level or type (%d)\n",
theSecLevel));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_UNSUPPORTEDSECURITYLEVEL;
}
@@ -1636,7 +1626,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGMSGTL(("usm",
"couldn't malloc %d bytes for encrypted PDU\n",
(int)ciphertextlen));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_MALLOC;
}
@@ -1652,7 +1641,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
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;
}
@@ -1667,7 +1655,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
thePrivKeyLength - 8,
iv) == -1)) {
DEBUGMSGTL(("usm", "Can't set DES-CBC salt.\n"));
- usm_free_usmStateReference(secStateRef);
SNMP_FREE(ciphertext);
return SNMPERR_USM_GENERICERROR;
}
@@ -1686,7 +1673,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
scopedPdu, scopedPduLen,
ciphertext, &ciphertextlen) != SNMP_ERR_NOERROR) {
DEBUGMSGTL(("usm", "encryption error.\n"));
- usm_free_usmStateReference(secStateRef);
SNMP_FREE(ciphertext);
return SNMPERR_USM_ENCRYPTIONERROR;
}
@@ -1703,7 +1689,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
ciphertext, ciphertextlen);
if (rc == 0) {
DEBUGMSGTL(("usm", "Encryption failed.\n"));
- usm_free_usmStateReference(secStateRef);
SNMP_FREE(ciphertext);
return SNMPERR_USM_ENCRYPTIONERROR;
}
@@ -1743,7 +1728,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGINDENTLESS();
if (rc == 0) {
DEBUGMSGTL(("usm", "building privParams failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1766,7 +1750,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGINDENTLESS();
if (rc == 0) {
DEBUGMSGTL(("usm", "building authParams failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1789,7 +1772,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGINDENTLESS();
if (rc == 0) {
DEBUGMSGTL(("usm", "building authParams failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1805,7 +1787,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
if (rc == 0) {
DEBUGMSGTL(("usm",
"building msgAuthoritativeEngineTime failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1821,7 +1802,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
if (rc == 0) {
DEBUGMSGTL(("usm",
"building msgAuthoritativeEngineBoots failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1833,7 +1813,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
DEBUGINDENTLESS();
if (rc == 0) {
DEBUGMSGTL(("usm", "building msgAuthoritativeEngineID failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1846,7 +1825,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
*offset - sp_offset);
if (rc == 0) {
DEBUGMSGTL(("usm", "building usm security parameters failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1860,7 +1838,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
if (rc == 0) {
DEBUGMSGTL(("usm", "building msgSecurityParameters failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1870,7 +1847,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
while ((*wholeMsgLen - *offset) < globalDataLen) {
if (!asn_realloc(wholeMsg, wholeMsgLen)) {
DEBUGMSGTL(("usm", "building global data failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
}
@@ -1886,7 +1862,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
ASN_CONSTRUCTOR), *offset);
if (rc == 0) {
DEBUGMSGTL(("usm", "building master packet sequence failed.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_TOO_LONG;
}
@@ -1904,7 +1879,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
if (temp_sig == NULL) {
DEBUGMSGTL(("usm", "Out of memory.\n"));
- usm_free_usmStateReference(secStateRef);
return SNMPERR_USM_GENERICERROR;
}
@@ -1915,14 +1889,12 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
!= 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;
}
@@ -1933,7 +1905,6 @@ usm_rgenerate_out_msg(int msgProcModel, /* (UNUSED) */
/*
* endif -- create keyed hash
*/
- usm_free_usmStateReference(secStateRef);
DEBUGMSGTL(("usm", "USM processing completed.\n"));
return SNMPERR_SUCCESS;
} /* end usm_rgenerate_out_msg() */
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic