CVE-2019-20892: Double Free (CWE-415)
value: net-snmp before 5.8.1.pre1 has a double free in usm_free_usmStateReference in snmplib/snmpusm.c via an SNMPv3 GetBulk request. NOTE: this affects net-snmp packages shipped to end users by multiple Linux distributions, but might not affect an upstream release.
CWE description: The product calls free() twice on the same memory address, potentially leading to modification of unexpected memory locations.
CVSS v3 base score: 6.5
Published: 2020-06-25T10:15Z
Repository: https://github.com/net-snmp/net-snmp
Language: C
Files changed: snmplib/snmp_client.c, snmplib/snmpusm.c
Commit date: 2019-07-29 21:22:13 -0700
Commit message: libsnmp, USM: Introduce a reference count in struct usmStateReference

This patch fixes https://sourceforge.net/p/net-snmp/bugs/2956/.

Diff: diff --git a/snmplib/snmp_client.c b/snmplib/snmp_client.c
index 918327f6f..ea48df8ab 100644
--- a/snmplib/snmp_client.c
+++ b/snmplib/snmp_client.c
@@ -403,32 +403,20 @@ _clone_pdu_header(netsnmp_pdu *pdu)
                           pdu->transport_data_length)) {
         snmp_free_pdu(newpdu);
         return NULL;
     }
 
-    if (pdu->securityStateRef &&
-        pdu->command == SNMP_MSG_TRAP2) {
-
-        netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
-        ret = usm_clone_usmStateReference((struct usmStateReference *) pdu->securityStateRef,
-                (struct usmStateReference **) &newpdu->securityStateRef );
-
-        if (ret)
-        {
+    sptr = find_sec_mod(newpdu->securityModel);
+    if (sptr && sptr->pdu_clone) {
+        /* call security model if it needs to know about this */
+        ret = sptr->pdu_clone(pdu, newpdu);
+        if (ret) {
             snmp_free_pdu(newpdu);
             return NULL;
         }
     }
 
-    if ((sptr = find_sec_mod(newpdu->securityModel)) != NULL &&
-        sptr->pdu_clone != NULL) {
-        /*
-         * call security model if it needs to know about this 
-         */
-        (*sptr->pdu_clone) (pdu, newpdu);
-    }
-
     return newpdu;
 }
 
 static
 netsnmp_variable_list *
diff --git a/snmplib/snmpusm.c b/snmplib/snmpusm.c
index ba108a432..1659e8fd8 100644
--- a/snmplib/snmpusm.c
+++ b/snmplib/snmpusm.c
@@ -83,10 +83,11 @@ netsnmp_feature_child_of(usm_all, libnetsnmp)
 netsnmp_feature_child_of(usm_support, usm_all)
 
 netsnmp_feature_require(usm_support)
 
 struct usmStateReference {
+    int             refcnt;
     char           *usr_name;
     size_t          usr_name_length;
     u_char         *usr_engine_id;
     size_t          usr_engine_id_length;
     oid            *usr_auth_protocol;
@@ -278,46 +279,67 @@ free_enginetime_on_shutdown(int majorid, int minorid, void *serverarg,
 }
 
 static struct usmStateReference *
 usm_malloc_usmStateReference(void)
 {
-    struct usmStateReference *retval = (struct usmStateReference *)
-        calloc(1, sizeof(struct usmStateReference));
+    struct usmStateReference *retval;
+
+    retval = calloc(1, sizeof(struct usmStateReference));
+    if (retval)
+        retval->refcnt = 1;
 
     return retval;
 }                               /* end usm_malloc_usmStateReference() */
 
+static int
+usm_clone(netsnmp_pdu *pdu, netsnmp_pdu *new_pdu)
+{
+    struct usmStateReference *ref = pdu->securityStateRef;
+    struct usmStateReference **new_ref =
+        (struct usmStateReference **)&new_pdu->securityStateRef;
+    int ret = 0;
+
+    if (!ref)
+        return ret;
+
+    if (pdu->command == SNMP_MSG_TRAP2) {
+        netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
+        ret = usm_clone_usmStateReference(ref, new_ref);
+    } else {
+        netsnmp_assert(ref == *new_ref);
+        ref->refcnt++;
+    }
+
+    return ret;
+}
+
 static void
 usm_free_usmStateReference(void *old)
 {
-    struct usmStateReference *old_ref = (struct usmStateReference *) old;
-
-    if (old_ref) {
-
-        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_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_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);
-        }
+    struct usmStateReference *ref = old;
+
+    if (!ref)
+        return;
+
+    if (--ref->refcnt > 0)
+        return;
 
-        SNMP_ZERO(old_ref, sizeof(*old_ref));
-        SNMP_FREE(old_ref);
+    SNMP_FREE(ref->usr_name);
+    SNMP_FREE(ref->usr_engine_id);
+    SNMP_FREE(ref->usr_auth_protocol);
+    SNMP_FREE(ref->usr_priv_protocol);
 
+    if (ref->usr_auth_key_length && ref->usr_auth_key) {
+        SNMP_ZERO(ref->usr_auth_key, ref->usr_auth_key_length);
+        SNMP_FREE(ref->usr_auth_key);
+    }
+    if (ref->usr_priv_key_length && ref->usr_priv_key) {
+        SNMP_ZERO(ref->usr_priv_key, ref->usr_priv_key_length);
+        SNMP_FREE(ref->usr_priv_key);
     }
 
+    SNMP_FREE(ref);
 }                               /* end usm_free_usmStateReference() */
 
 struct usmUser *
 usm_get_userList(void)
 {
@@ -4914,10 +4936,11 @@ init_usm(void)
      * XXX: def->init_sess_secmod move stuff from snmp_api.c
      */
     def->encode_reverse = usm_secmod_rgenerate_out_msg;
     def->encode_forward = usm_secmod_generate_out_msg;
     def->decode = usm_secmod_process_in_msg;
+    def->pdu_clone = usm_clone;
     def->pdu_free_state_ref = usm_free_usmStateReference;
     def->session_setup = usm_session_init;
     def->handle_report = usm_handle_report;
     def->probe_engineid = usm_discover_engineid;
     def->post_probe_engineid = usm_create_user_from_session_hook;

Vulnerable code:     if (pdu->securityStateRef &&
        pdu->command == SNMP_MSG_TRAP2) {

        netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
        ret = usm_clone_usmStateReference((struct usmStateReference *) pdu->securityStateRef,
                (struct usmStateReference **) &newpdu->securityStateRef );

        if (ret)
        {
    if ((sptr = find_sec_mod(newpdu->securityModel)) != NULL &&
        sptr->pdu_clone != NULL) {
        /*
         * call security model if it needs to know about this 
         */
        (*sptr->pdu_clone) (pdu, newpdu);
    }

    struct usmStateReference *retval = (struct usmStateReference *)
        calloc(1, sizeof(struct usmStateReference));
    struct usmStateReference *old_ref = (struct usmStateReference *) old;

    if (old_ref) {

        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_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_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);
        }
        SNMP_ZERO(old_ref, sizeof(*old_ref));
        SNMP_FREE(old_ref);
Fixed code:     sptr = find_sec_mod(newpdu->securityModel);
    if (sptr && sptr->pdu_clone) {
        /* call security model if it needs to know about this */
        ret = sptr->pdu_clone(pdu, newpdu);
        if (ret) {
    int             refcnt;
    struct usmStateReference *retval;

    retval = calloc(1, sizeof(struct usmStateReference));
    if (retval)
        retval->refcnt = 1;
static int
usm_clone(netsnmp_pdu *pdu, netsnmp_pdu *new_pdu)
{
    struct usmStateReference *ref = pdu->securityStateRef;
    struct usmStateReference **new_ref =
        (struct usmStateReference **)&new_pdu->securityStateRef;
    int ret = 0;

    if (!ref)
        return ret;

    if (pdu->command == SNMP_MSG_TRAP2) {
        netsnmp_assert(pdu->securityModel == SNMP_DEFAULT_SECMODEL);
        ret = usm_clone_usmStateReference(ref, new_ref);
    } else {
        netsnmp_assert(ref == *new_ref);
        ref->refcnt++;
    }

    return ret;
}

    struct usmStateReference *ref = old;

    if (!ref)
        return;

    if (--ref->refcnt > 0)
        return;
    SNMP_FREE(ref->usr_name);
    SNMP_FREE(ref->usr_engine_id);
    SNMP_FREE(ref->usr_auth_protocol);
    SNMP_FREE(ref->usr_priv_protocol);
    if (ref->usr_auth_key_length && ref->usr_auth_key) {
        SNMP_ZERO(ref->usr_auth_key, ref->usr_auth_key_length);
        SNMP_FREE(ref->usr_auth_key);
    }
    if (ref->usr_priv_key_length && ref->usr_priv_key) {
        SNMP_ZERO(ref->usr_priv_key, ref->usr_priv_key_length);
        SNMP_FREE(ref->usr_priv_key);
    SNMP_FREE(ref);
    def->pdu_clone = usm_clone;