Lock down snmp as read-only and sanitize input

external.mk

@@ -18,6 +18,13 @@ endef
 
 FRR_POST_BUILD_HOOKS += FRR_POST_BUILD_HOOK
 
+#
+# The SNMP agent is read-only, see doc/snmp.md.  Drop SET support from
+# the build rather than leave it to the generated VACM configuration to
+# withhold, so a mistake there cannot become a writable agent.
+#
+NETSNMP_CONF_OPTS += --enable-read-only
+
 #
 # External pre-built toolchains do not carry their own license.
 #

src/confd/src/snmp.c

@@ -29,6 +29,25 @@
 /* Default when /snmp/engine/listen names no port. */
 #define SNMP_PORT "161"
 
+/*
+ * infix-snmp restricts the leaves we own so a line break cannot reach
+ * here, but /system/contact and /system/location belong to ietf-system
+ * and are free text.  Drop anything that would not sit on one line of
+ * snmpd.conf rather than let it add a directive.
+ */
+static int safe(const char *str)
+{
+	if (!str)
+		return 0;
+
+	for (; *str; str++) {
+		if (!isprint((unsigned char)*str))
+			return 0;
+	}
+
+	return 1;
+}
+
 static int is_v6(const char *addr)
 {
 	return strchr(addr, ':') != NULL;
@@ -157,11 +176,13 @@ static const char *community_name(struct lyd_node *community)
 	const char *name = lydx_get_cattr(community, "text-name");
 
 	if (name)
-		return name;
+		return safe(name) ? name : NULL;
 	if (lydx_get_cattr(community, "binary-name"))
 		return NULL;
 
-	return lydx_get_cattr(community, "security-name");
+	name = lydx_get_cattr(community, "security-name");
+
+	return safe(name) ? name : NULL;
 }
 
 /*
@@ -174,7 +195,7 @@ static const char *community_secname(struct lyd_node *snmp, struct lyd_node *com
 	const char *secname = lydx_get_cattr(community, "security-name");
 	const char *tag;
 
-	if (!secname || !community_name(community))
+	if (!safe(secname) || !community_name(community))
 		return NULL;
 
 	tag = lydx_get_cattr(community, "target-tag");
@@ -193,7 +214,7 @@ static void communities(FILE *fp, struct lyd_node *snmp)
 		const char *secname, *name, *tag;
 
 		secname = lydx_get_cattr(community, "security-name");
-		if (!secname)
+		if (!safe(secname))
 			continue;
 
 		name = community_name(community);
@@ -305,10 +326,10 @@ static int generate(struct lyd_node *config, struct lyd_node *snmp)
 	system = lydx_get_xpathf(config, XPATH_SYSTEM_);
 	if (system) {
 		str = lydx_get_cattr(system, "contact");
-		if (str)
+		if (str && safe(str))
 			fprintf(fp, "syscontact %s\n", str);
 		str = lydx_get_cattr(system, "location");
-		if (str)
+		if (str && safe(str))
 			fprintf(fp, "syslocation %s\n", str);
 	}
 

src/confd/yang/confd/infix-snmp.yang

@@ -31,6 +31,36 @@ module infix-snmp {
     reference "internal";
   }
 
+  /*
+   * The agent is configured by generating snmpd.conf, which is line
+   * oriented, so a line break in any of these would start a directive
+   * of the operator's choosing.  Derive from the original types so the
+   * lengths RFC 7407 gives them still apply.
+   */
+  deviation "/snmp:snmp/snmp:community/snmp:index" {
+    deviate replace {
+      type snmp:identifier {
+        pattern '[^\n\r]*';
+      }
+    }
+  }
+
+  deviation "/snmp:snmp/snmp:community/snmp:security-name" {
+    deviate replace {
+      type snmp:security-name {
+        pattern '[^\n\r]*';
+      }
+    }
+  }
+
+  deviation "/snmp:snmp/snmp:community/snmp:name/snmp:text-name/snmp:text-name" {
+    deviate replace {
+      type string {
+        pattern '[^\n\r]*';
+      }
+    }
+  }
+
   deviation "/snmp:snmp/snmp:vacm" {
     deviate not-supported;
     description "Access control is not view-based here.  Every configured