snmplib: Improve handling of zero-length ASN OCTET STRINGs (#2163)

When converting zero-length objects to c-strings and terminating their
values, do not write to c-string storage buffer at offset `-1`.

Also reject 128-byte SNMP community values because our storage buffer
does not allow for their zero-termination. Such values were probably
silently truncated to 127 bytes.

Also reject SNMP community values with ASCII NUL characters because some
of our code does not support such values.

diff --git a/lib/snmplib/snmp_msg.c b/lib/snmplib/snmp_msg.c
index e9c2de8187c6525022a6ab831a4e9f4b06f81c41..edfa64a7a2b8b0933b00547596f84ddcbb82780a 100644 (file)
--- a/lib/snmplib/snmp_msg.c
+++ b/lib/snmplib/snmp_msg.c
@@ -87,6 +87,9 @@
 #if HAVE_NETDB_H
 #include <netdb.h>
 #endif
+#if HAVE_ASSERT_H
+#include <assert.h>
+#endif
 
 #include "asn1.h"
 #include "snmp.h"
@@ -272,16 +275,26 @@ snmp_msg_Decode(u_char * Packet, int *PacketLenP,
         snmplib_debug(4, "snmp_msg_Decode:Error decoding SNMP Message Header (Version)!\n");
         ASN_PARSE_ERROR(NULL);
     }
-    int terminatorPos = *CommLenP - 1;
+    int communityBufferLimit = *CommLenP;
+
     bufp = asn_parse_string(bufp, PacketLenP, &type, Community, CommLenP);
     if (bufp == NULL) {
         snmplib_debug(4, "snmp_msg_Decode:Error decoding SNMP Message Header (Community)!\n");
         ASN_PARSE_ERROR(NULL);
     }
-    if (*CommLenP < terminatorPos) {
-        terminatorPos = *CommLenP;
+
+    if (*CommLenP == communityBufferLimit) {
+        snmplib_debug(4, "snmp_msg_Decode:Cannot zero-terminate a %d byte-long Community value\n", *CommLenP);
+        ASN_PARSE_ERROR(NULL);
+    }
+    assert(*CommLenP >= 0);
+    assert(*CommLenP < communityBufferLimit);
+    Community[*CommLenP] = '\0';
+
+    if (memchr(Community, '\0', (size_t)*CommLenP)) {
+        snmplib_debug(4, "snmp_msg_Decode:Community contained an unsupported ASCII nul character\n");
+        ASN_PARSE_ERROR(NULL);
     }
-    Community[terminatorPos] = '\0';
 
     if ((*Version != SNMP_VERSION_1) &&
             (*Version != SNMP_VERSION_2)) {

diff --git a/lib/snmplib/snmp_vars.c b/lib/snmplib/snmp_vars.c
index 6184220630ebe1f34cb21f63eca4c693c89e8f09..f1d7e49fd5b62f38f1bb5babe7ea2b2e91588292 100644 (file)
--- a/lib/snmplib/snmp_vars.c
+++ b/lib/snmplib/snmp_vars.c
@@ -502,20 +502,16 @@ snmp_var_DecodeVarBind(u_char * Buffer, int *BufLen,
         case ASN_OCTET_STR:
         case SMI_IPADDRESS:
         case SMI_OPAQUE:
-            Var->val_len = *&ThisVarLen;    /* String is this at most */
-            Var->val.string = (u_char *) xmalloc((unsigned) Var->val_len);
+            Var->val_len = ThisVarLen >= 0 ? ThisVarLen : 0; // input contains at most this many bytes
+            Var->val.string = (u_char *) xmalloc((unsigned) Var->val_len + 1);
             if (Var->val.string == NULL) {
                 snmp_set_api_error(SNMPERR_OS_ERR);
                 PARSE_ERROR;
             }
-            int terminatorPos = Var->val_len - 1;
             bufp = asn_parse_string(DataPtr, &ThisVarLen,
                                     &Var->type, Var->val.string,
                                     &Var->val_len);
-            if (Var->val_len < terminatorPos) {
-                terminatorPos = Var->val_len;
-            }
-            Var->val.string[terminatorPos] = '\0';
+            Var->val.string[Var->val_len] = '\0'; // for cases where the parsed value is treated as a c-string
 #if DEBUG_VARS_DECODE
             printf("VARS: Decoded string '%s' (length %d) (%d bytes left)\n",
                    (Var->val.string), Var->val_len, ThisVarLen);