From adc9b71aba9168ec64149345ea37a1acc11875c6 Mon Sep 17 00:00:00 2001
From: Sam Tannous <stannous@cumulusnetworks.com>
Date: Wed, 10 Apr 2019 06:57:21 -0700
Subject: [PATCH] snmpd: Avoid that snmpv3 bulkget errors result in a double
 free

See also https://sourceforge.net/p/net-snmp/bugs/2923/.
See also https://sourceforge.net/p/net-snmp/patches/1388/.
---
 agent/snmp_agent.c         |  7 ++++++
 include/net-snmp/pdu_api.h |  2 ++
 snmplib/snmp_api.c         | 11 ++++++++
 snmplib/snmpusm.c          | 51 ++++++++------------------------------
 4 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/agent/snmp_agent.c b/agent/snmp_agent.c
index 26653f4e6..100c4d001 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 125595d9a..270aff054 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 554767a83..321a48f1b 100644
--- a/snmplib/snmp_api.c
+++ b/snmplib/snmp_api.c
@@ -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 3cfa1267a..873bd890f 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() */
