Vulnerability: The csnmp_read_table function in snmp.c in the SNMP plugin in collectd before 5.6.3 is susceptible to a double free in a certain error case, which could lead to a crash (or potentially have other impact).
Vulnerability ID: CVE-2017-16820
CWE: CWE-415
Severity score: 10
Project: https://github.com/collectd/collectd
Published: 2017-11-14
File: src/snmp.c
Commit message: snmp plugin: Fix double free of request PDU

snmp_sess_synch_response() always frees request PDU, in both case of request
error and success. If error condition occurs inside of `while (status == 0)`
loop, double free of `req` happens.

Issue: #2291
Signed-off-by: Florian Forster <octo@collectd.org>
Commit author: Pavel Rochnyack

Vulnerable version (before fix):
 static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {     if (oid_list_todo_num == 0) {       /* The request is still empty - so we are finished */       DEBUG("snmp plugin: all variables have left their subtree");       status = 0;       break;     }      res = NULL;     status = snmp_sess_synch_response(host->sess_handle, req, &res);     if ((status != STAT_SUCCESS) || (res == NULL)) {       char *errstr = NULL; static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {         snmp_free_pdu(res);       res = NULL;        /* snmp_synch_response already freed our PDU */       req = NULL;       sfree(errstr);       csnmp_host_close_session(host);  static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {     snmp_free_pdu(res);   res = NULL;    if (req != NULL)     snmp_free_pdu(req);   req = NULL;    if (status == 0)     csnmp_dispatch_table(host, data, instance_list_head, value_list_head);

Fixed version (after fix):
 static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {     if (oid_list_todo_num == 0) {       /* The request is still empty - so we are finished */       DEBUG("snmp plugin: all variables have left their subtree");       snmp_free_pdu(req);       status = 0;       break;     }      res = NULL;     /* snmp_sess_synch_response always frees our req PDU */     status = snmp_sess_synch_response(host->sess_handle, req, &res);     if ((status != STAT_SUCCESS) || (res == NULL)) {       char *errstr = NULL; static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {         snmp_free_pdu(res);       res = NULL;        sfree(errstr);       csnmp_host_close_session(host);  static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {     snmp_free_pdu(res);   res = NULL;     if (status == 0)     csnmp_dispatch_table(host, data, instance_list_head, value_list_head);

Patch:
patch: @@ -1357,11 +1357,13 @@ static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {
     if (oid_list_todo_num == 0) {
       /* The request is still empty - so we are finished */
       DEBUG("snmp plugin: all variables have left their subtree");
+      snmp_free_pdu(req);
       status = 0;
       break;
     }
 
     res = NULL;
+    /* snmp_sess_synch_response always frees our req PDU */
     status = snmp_sess_synch_response(host->sess_handle, req, &res);
     if ((status != STAT_SUCCESS) || (res == NULL)) {
       char *errstr = NULL;
@@ -1376,8 +1378,6 @@ static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {
         snmp_free_pdu(res);
       res = NULL;
 
-      /* snmp_synch_response already freed our PDU */
-      req = NULL;
       sfree(errstr);
       csnmp_host_close_session(host);
 
@@ -1492,9 +1492,6 @@ static int csnmp_read_table(host_definition_t *host, data_definition_t *data) {
     snmp_free_pdu(res);
   res = NULL;
 
-  if (req != NULL)
-    snmp_free_pdu(req);
-  req = NULL;
 
   if (status == 0)
     csnmp_dispatch_table(host, data, instance_list_head, value_list_head);
path: src/snmp.c