Index | Thread | Search

From:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject:
Re: snmpd(8): Be stricter with varbind value ranges/encodings
To:
tech@openbsd.org
Date:
Tue, 06 Feb 2024 22:08:42 +0100

Download raw body.

Thread
Here's the regression part. As stated, only octet string, ipaddress, and
opaque can be tested via agentx. Because the octet string exceeds the
max UDP size I've also had to move that test to tcp and reshuffle the
get code around there.

martijn@

Index: Makefile
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/Makefile,v
retrieving revision 1.12
diff -u -p -r1.12 Makefile
--- Makefile	20 Nov 2023 10:34:21 -0000	1.12
+++ Makefile	6 Feb 2024 21:08:18 -0000
@@ -146,6 +146,12 @@ BACKEND_TARGETS+=		backend_get_close_ove
 BACKEND_TARGETS+=		backend_get_disappear
 BACKEND_TARGETS+=		backend_get_disappear_overlap
 BACKEND_TARGETS+=		backend_get_disappear_doublesession
+BACKEND_TARGETS+=		backend_get_octetstring_max
+BACKEND_TARGETS+=		backend_get_octetstring_too_long
+BACKEND_TARGETS+=		backend_get_ipaddress_too_short
+BACKEND_TARGETS+=		backend_get_ipaddress_too_long
+BACKEND_TARGETS+=		backend_get_opaque_non_ber
+BACKEND_TARGETS+=		backend_get_opaque_double_value
 BACKEND_TARGETS+=		backend_getnext_selfbound
 BACKEND_TARGETS+=		backend_getnext_lowerbound
 BACKEND_TARGETS+=		backend_getnext_lowerbound_self
Index: backend.c
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/backend.c,v
retrieving revision 1.5
diff -u -p -r1.5 backend.c
--- backend.c	16 Nov 2023 13:26:45 -0000	1.5
+++ backend.c	6 Feb 2024 21:08:18 -0000
@@ -1,6 +1,7 @@
 #include <sys/socket.h>
 #include <sys/time.h>
 
+#include <ber.h>
 #include <err.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -294,12 +295,13 @@ backend_get_opaque(void)
 	struct varbind varbind = {
 		.type = TYPE_NULL,
 		.name = OID_STRUCT(MIB_BACKEND_GET, 8, 0),
-		.data.octetstring.string = "\1",
-		.data.octetstring.len = 1
 	};
 	int32_t requestid;
 	char buf[1024];
 	size_t n;
+	struct ber ber = {};
+	struct ber_element *elm;
+	ssize_t len;
 
 	ax_s = agentx_connect(axsocket);
 	sessionid = agentx_open(ax_s, 0, 0,
@@ -314,11 +316,20 @@ backend_get_opaque(void)
 	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
 	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
 
+	if ((elm = ober_add_integer(NULL, 1)) == NULL)
+		err(1, "ober_add_integer");
+	if (ober_write_elements(&ber, elm) == -1)
+		err(1, "ober_write_elements");
+	varbind.data.octetstring.len = ober_get_writebuf(
+	    &ber, (void **)&varbind.data.octetstring.string);
+	ober_free_elements(elm);
+
 	varbind.type = TYPE_OPAQUE;
 	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
 
 	snmpv2_response_validate(snmp_s, 1000, community, requestid, 0, 0,
 	    &varbind, 1);
+	ober_free(&ber);
 }
 
 void
@@ -1380,6 +1391,261 @@ backend_get_disappear_doublesession(void
 	varbind.type = TYPE_NOSUCHOBJECT;
 	snmpv2_response_validate(snmp_s, 1000, community, requestid, 0, 0,
 	    &varbind, 1);
+}
+
+void
+backend_get_octetstring_max(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	char vbbuf[65535] = {};
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 34, 0),
+		.data.octetstring.string = vbbuf,
+		.data.octetstring.len = sizeof(vbbuf)
+	};
+	int32_t requestid;
+	char buf[1024];
+	size_t n;
+
+	memset(vbbuf, 'a', sizeof(vbbuf));
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 34), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 34), 0);
+
+	/* Too big for SOCK_DGRAM */
+	salen = snmp_resolve(SOCK_STREAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_STREAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	varbind.type = TYPE_OCTETSTRING;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, 0, 0,
+	    &varbind, 1);
+}
+
+void
+backend_get_octetstring_too_long(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	char vbbuf[65536];
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 35, 0),
+		.data.octetstring.string = vbbuf,
+		.data.octetstring.len = sizeof(vbbuf)
+	};
+	int32_t requestid;
+	char buf[1024];
+	size_t n;
+
+	memset(vbbuf, 'a', sizeof(vbbuf));
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 35), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 35), 0);
+
+	salen = snmp_resolve(SOCK_STREAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_STREAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	varbind.type = TYPE_OCTETSTRING;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	varbind.type = TYPE_NULL;
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, GENERR, 1,
+	    &varbind, 1);
+}
+
+void
+backend_get_ipaddress_too_short(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 36, 0),
+		.data.octetstring.string = "\0\0\0",
+		.data.octetstring.len = 3
+	};
+	int32_t requestid;
+	char buf[1024];
+	size_t n;
+
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 36), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 36), 0);
+
+	salen = snmp_resolve(SOCK_DGRAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_DGRAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	varbind.type = TYPE_IPADDRESS;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	varbind.type = TYPE_NULL;
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, GENERR, 1,
+	    &varbind, 1);
+}
+
+void
+backend_get_ipaddress_too_long(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 37, 0),
+		.data.octetstring.string = "\0\0\0\0\0",
+		.data.octetstring.len = 5
+	};
+	int32_t requestid;
+	char buf[1024];
+	size_t n;
+
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 37), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 37), 0);
+
+	salen = snmp_resolve(SOCK_DGRAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_DGRAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	varbind.type = TYPE_IPADDRESS;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	varbind.type = TYPE_NULL;
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, GENERR, 1,
+	    &varbind, 1);
+}
+
+void
+backend_get_opaque_non_ber(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 38, 0),
+		.data.octetstring.string = "\1",
+		.data.octetstring.len = 1
+	};
+	int32_t requestid;
+	char buf[1024];
+	size_t n;
+	ssize_t len;
+
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 38), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 38), 0);
+
+	salen = snmp_resolve(SOCK_DGRAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_DGRAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	varbind.type = TYPE_OPAQUE;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	varbind.type = TYPE_NULL;
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, GENERR, 1,
+	    &varbind, 1);
+}
+
+void
+backend_get_opaque_double_value(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *sa = (struct sockaddr *)&ss;
+	socklen_t salen;
+	int snmp_s, ax_s;
+	uint32_t sessionid;
+	char vbbuf[1024];
+	struct varbind varbind = {
+		.type = TYPE_NULL,
+		.name = OID_STRUCT(MIB_BACKEND_GET, 39, 0),
+		.data.octetstring.string = vbbuf
+	};
+	int32_t requestid;
+	void *berdata;
+	char buf[1024];
+	size_t n;
+	struct ber ber = {};
+	struct ber_element *elm;
+	ssize_t len;
+
+	ax_s = agentx_connect(axsocket);
+	sessionid = agentx_open(ax_s, 0, 0,
+	    OID_ARG(MIB_SUBAGENT_BACKEND_GET, 39), __func__);
+	agentx_register(ax_s, sessionid, 0, 0, 127, 0,
+	    OID_ARG(MIB_BACKEND_GET, 39), 0);
+
+	salen = snmp_resolve(SOCK_DGRAM, hostname, servname, sa);
+	snmp_s = snmp_connect(SOCK_DGRAM, sa, salen);
+	requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+	n = agentx_read(ax_s, buf, sizeof(buf), 1000);
+	agentx_get_handle(__func__, buf, n, 0, sessionid, &varbind, 1);
+
+	if ((elm = ober_add_integer(NULL, 1)) == NULL)
+		err(1, "ober_add_integer");
+	if (ober_write_elements(&ber, elm) == -1)
+		err(1, "ober_write_elements");
+	len = ober_get_writebuf(&ber, &berdata);
+	ober_free_elements(elm);
+
+	memcpy(vbbuf, berdata, len);
+	memcpy(vbbuf + len, berdata, len);
+	varbind.data.octetstring.len = 2 * len;
+
+	varbind.type = TYPE_OPAQUE;
+	agentx_response(ax_s, buf, NOERROR, 0, &varbind, 1);
+
+	varbind.type = TYPE_NULL;
+	snmpv2_response_validate(snmp_s, 1000, community, requestid, GENERR, 1,
+	    &varbind, 1);
+	ober_free(&ber);
 }
 
 void
Index: regress.h
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/regress.h,v
retrieving revision 1.8
diff -u -p -r1.8 regress.h
--- regress.h	20 Nov 2023 10:34:21 -0000	1.8
+++ regress.h	6 Feb 2024 21:08:18 -0000
@@ -289,6 +289,12 @@ void backend_get_close_overlap(void);
 void backend_get_disappear(void);
 void backend_get_disappear_overlap(void);
 void backend_get_disappear_doublesession(void);
+void backend_get_octetstring_max(void);
+void backend_get_octetstring_too_long(void);
+void backend_get_ipaddress_too_short(void);
+void backend_get_ipaddress_too_long(void);
+void backend_get_opaque_non_ber(void);
+void backend_get_opaque_double_value(void);
 void backend_getnext_selfbound(void);
 void backend_getnext_lowerbound(void);
 void backend_getnext_lowerbound_self(void);
Index: snmp.c
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/snmp.c,v
retrieving revision 1.3
diff -u -p -r1.3 snmp.c
--- snmp.c	20 Nov 2023 10:34:21 -0000	1.3
+++ snmp.c	6 Feb 2024 21:08:18 -0000
@@ -5,6 +5,7 @@
 
 #include <ber.h>
 #include <err.h>
+#include <errno.h>
 #include <inttypes.h>
 #include <netdb.h>
 #include <poll.h>
@@ -288,7 +289,7 @@ snmpv2_response_validate(int s, int time
     struct varbind *varbindlist, size_t nvarbind)
 {
 	struct ber_element *message, *pdu;
-	char buf[1024];
+	char buf[100000];
 	size_t buflen = sizeof(buf);
 
 	message = snmp_recv(s, timeout, buf, &buflen);
@@ -311,22 +312,30 @@ snmp_recv(int s, int timeout, void *buf,
 	struct ber ber = {};
 	struct ber_element *message;
 	ssize_t n;
+	size_t ntot = 0;
 	int ret;
 
+ again:
 	if ((ret = poll(&pfd, 1, timeout)) == -1)
 		err(1, "poll");
-	if (ret == 0)
-		errx(1, "%s: timeout", __func__);
-	if ((n = read(s, buf, *buflen)) == -1)
-		err(1, "agentx read");
-
-	*buflen = n;
+	if (ret == 0) {
+		if (ntot == 0)
+			errx(1, "%s: timeout", __func__);
+		errc(1, ECANCELED, "%s: unable to decode message", __func__);
+	}
+	if ((n = read(s, buf + ntot, *buflen - ntot)) == -1)
+		err(1, "snmp read");
+	ntot += n;
 
 	ober_set_application(&ber, smi_application);
-	ober_set_readbuf(&ber, buf, n);
+	ober_set_readbuf(&ber, buf, ntot);
 
-	if ((message = ober_read_elements(&ber, NULL)) == NULL)
+	if ((message = ober_read_elements(&ber, NULL)) == NULL) {
+		if (errno == ECANCELED)
+			goto again;
 		errx(1, "%s: unable to decode message", __func__);
+	}
+	*buflen = n;
 	if (verbose) {
 		printf("SNMP received(%d):\n", s);
 		smi_debug_elements(message);
Index: snmpd_regress.c
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/snmpd_regress.c,v
retrieving revision 1.8
diff -u -p -r1.8 snmpd_regress.c
--- snmpd_regress.c	20 Nov 2023 10:34:21 -0000	1.8
+++ snmpd_regress.c	6 Feb 2024 21:08:18 -0000
@@ -118,6 +118,12 @@ const struct {
 	{ "backend_get_disappear", backend_get_disappear },
 	{ "backend_get_disappear_overlap", backend_get_disappear_overlap },
 	{ "backend_get_disappear_doublesession", backend_get_disappear_doublesession },
+	{ "backend_get_octetstring_max", backend_get_octetstring_max },
+	{ "backend_get_octetstring_too_long", backend_get_octetstring_too_long },
+	{ "backend_get_ipaddress_too_short", backend_get_ipaddress_too_short },
+	{ "backend_get_ipaddress_too_long", backend_get_ipaddress_too_long },
+	{ "backend_get_opaque_non_ber", backend_get_opaque_non_ber },
+	{ "backend_get_opaque_double_value", backend_get_opaque_double_value },
 	{ "backend_getnext_selfbound", backend_getnext_selfbound },
 	{ "backend_getnext_lowerbound", backend_getnext_lowerbound },
 	{ "backend_getnext_lowerbound_self", backend_getnext_lowerbound_self },