On Mon, 2021-06-14 at 12:55 +0100, Stuart Henderson wrote:
> By default, snmpd responds to the frequently abused community strings
> "public" and "private".
>
> To prevent this, at present you must either use "seclevel auth" or
> "seclevel enc" (if you would like to only use SNMPv3), set an explicit
> string for the read-only community, or set either an explicit string
> or "disabled" for the read-write community.
>
> I would like to remove the defaults. If somebody really wants to use
> the strings "public" or "private" they can set them themselves. The
> internet doesn't need any more unintentional UDP amplifiers than
> necessary.
I fully agree.
>
> Additionally if somebody goes to the trouble of configuring SNMPv3,
> the common use case is to want authentication+encryption; anything
> wider than that, it's reasonable to expect the user to make it
> explicit, so I've changed it to "seclevel enc" by default iff an
> SNMPv3 user is created.
> This works as expected in the use-cases I've tried. Any concerns/
> comments/OKs? (I'll write an faq/current.html entry if it goes in).
>
> Note, it's not possible to require auth+enc for SNMPv3 requests while
> also allowing SNMPv1/2; that isn't new and I haven't changed it.
>
I think that your approach is a nice first stab, but is incomplete.
- if the concern is amplification attacks then setting the minlevel to
authpriv is too high, since you'll silently break logins for users
that miss the enckey parameter.
I changed this to always default to seclevel auth.
- Make sure that no users can be created with lower privs then seclevel,
so that people don't have to start debugging this issue when already
running in production with loglevels that are always too low.
- If we disable SNMPv{1,2c} by default I reckon it's dumb to overload
seclevel with said protocols. I decoupled them (this addresses your
last statement)
- Add a disallow for empty community strings. This effectively
disables SNMPv{1,2c} in all cases if we set an empty string.
- Log the disabling of SNMPv{1,2c} properly and increment
snmp_inbadversions.
- Setting an empty string is equal to disallow, so make
"read-write community" equal to an empty string and remove the now
useless sc_readonly.
- When sc_trcommunity is disabled we don't have the backup for
"trap receiver". Check for this explicitly during startup.
At some point I want to rework the MPS subsystem (which snmpe.c
basically mostly is). But I think this diff is a nice stopgap until
I get around to it.
Only lightly tested through regress and written quickly in between
jobs, so OKs welcome, but some modifications before commit can't
be ruled out. :-)
martijn@
Index: parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.63
diff -u -p -r1.63 parse.y
--- parse.y 22 Jan 2021 06:33:26 -0000 1.63
+++ parse.y 14 Jun 2021 17:39:47 -0000
@@ -217,7 +217,7 @@ main : LISTEN ON listenproto
free($3);
}
| READWRITE DISABLED {
- conf->sc_readonly = 1;
+ conf->sc_rwcommunity[0] = '\0';
}
| TRAP COMMUNITY STRING {
if (strlcpy(conf->sc_trcommunity, $3,
@@ -1102,7 +1102,10 @@ parse_config(const char *filename, u_int
struct sockaddr_storage ss;
struct sym *sym, *next;
struct address *h;
- int found;
+ struct trap_address *tr;
+ const struct usmuser *up;
+ const char *errstr;
+ int found;
if ((conf = calloc(1, sizeof(*conf))) == NULL) {
log_warn("%s", __func__);
@@ -1112,10 +1115,8 @@ parse_config(const char *filename, u_int
conf->sc_flags = flags;
conf->sc_confpath = filename;
TAILQ_INIT(&conf->sc_addresses);
- strlcpy(conf->sc_rdcommunity, "public", SNMPD_MAXCOMMUNITYLEN);
- strlcpy(conf->sc_rwcommunity, "private", SNMPD_MAXCOMMUNITYLEN);
- strlcpy(conf->sc_trcommunity, "public", SNMPD_MAXCOMMUNITYLEN);
TAILQ_INIT(&conf->sc_trapreceivers);
+ conf->sc_min_seclevel = SNMP_MSGFLAG_AUTH;
if ((file = pushfile(filename, 0)) == NULL) {
free(conf);
@@ -1130,6 +1131,8 @@ parse_config(const char *filename, u_int
endservent();
+ if ((up = usm_check_mincred(conf->sc_min_seclevel, &errstr)) != NULL)
+ fatalx("user '%s': %s", up->uu_name, errstr);
/* Setup default listen addresses */
if (TAILQ_EMPTY(&conf->sc_addresses)) {
if (host("0.0.0.0", SNMP_PORT, SOCK_DGRAM, &ss, 1) != 1)
@@ -1155,6 +1158,17 @@ parse_config(const char *filename, u_int
log_warnx("notify listener needs at least one trap handler");
free(conf);
return (NULL);
+ }
+
+ if (conf->sc_trcommunity[0] == '\0') {
+ found = 0;
+ TAILQ_FOREACH(tr, &conf->sc_trapreceivers, entry) {
+ if (tr->sa_community == NULL) {
+ log_warnx("trap receiver: missing community");
+ free(conf);
+ return (NULL);
+ }
+ }
}
/* Free macros and check which have not been used. */
Index: snmpd.conf.5
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.conf.5,v
retrieving revision 1.48
diff -u -p -r1.48 snmpd.conf.5
--- snmpd.conf.5 14 Jun 2021 12:28:58 -0000 1.48
+++ snmpd.conf.5 14 Jun 2021 17:39:47 -0000
@@ -136,35 +136,27 @@ set requires at least one
statement.
.It Ic read-only community Ar string
Specify the name of the read-only community.
-The default value is
-.Ar public .
-.It Ic read-write Pq Ic community Ar string Ic | disabled
+There is no default value.
+.It Ic read-write Ic community Ar string
Specify the name of the read-write community, or disallow writes completely.
-The default value is
-.Ar private .
+There is no default value.
.It Ic seclevel Pq Ic none | auth | enc
Specify the lowest security level that
.Xr snmpd 8
-accepts:
+accepts on SNMPv3:
.Bl -tag -width "auth" -offset ident
.It Ic none
Both authentication and encryption of messages is optional.
-This is the default value.
.It Ic auth
Authentication of messages is mandatory.
.Xr snmpd 8
will discard any messages that don't have a valid digest.
Encryption of messages is optional.
+This is the default value.
.It Ic enc
Messages must be encrypted and must have a valid digest for authentication.
Otherwise they will be discarded.
.El
-.Pp
-If the chosen value is different from
-.Ic none
-.Xr snmpd 8
-will accept only SNMPv3 requests since older versions neither support
-authentication nor encryption.
.It Ic system contact Ar string
Specify the name or description of the system contact, typically a
name or an email address.
@@ -206,8 +198,7 @@ description in the SNMP MIB for details.
.\"XXX describe the complicated services alg here
.It Ic trap community Ar string
Specify the name of the trap community.
-The default value is
-.Ar public .
+There is no default value.
.It Ic trap handle Ar oid Qq Ar command
Execute
.Ic command
@@ -330,6 +321,7 @@ to listen on localhost, override the def
magic services value and provides some custom OID values:
.Bd -literal -offset indent
listen on 127.0.0.1
+read community public
system oid 1.3.6.1.4.1.30155.23.2
system services 74
Index: snmpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
retrieving revision 1.95
diff -u -p -r1.95 snmpd.h
--- snmpd.h 20 May 2021 08:53:12 -0000 1.95
+++ snmpd.h 14 Jun 2021 17:39:47 -0000
@@ -576,7 +576,6 @@ struct snmpd {
int sc_pfaddrfilter;
int sc_min_seclevel;
- int sc_readonly;
int sc_traphandler;
struct privsep sc_ps;
@@ -740,6 +739,7 @@ struct ber_element *usm_encode(struct sn
struct ber_element *usm_encrypt(struct snmp_message *, struct ber_element *);
void usm_finalize_digest(struct snmp_message *, char *, ssize_t);
void usm_make_report(struct snmp_message *);
+const struct usmuser *usm_check_mincred(int, const char **);
/* proc.c */
enum privsep_procid
Index: snmpe.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.71
diff -u -p -r1.71 snmpe.c
--- snmpe.c 20 May 2021 08:53:12 -0000 1.71
+++ snmpe.c 14 Jun 2021 17:39:47 -0000
@@ -255,14 +255,13 @@ snmpe_parse(struct snmp_message *msg)
switch (msg->sm_version) {
case SNMP_V1:
case SNMP_V2:
- if (env->sc_min_seclevel != 0)
- goto badversion;
if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != 0)
goto parsefail;
if (strlcpy(msg->sm_community, comn,
- sizeof(msg->sm_community)) >= sizeof(msg->sm_community)) {
+ sizeof(msg->sm_community)) >= sizeof(msg->sm_community) ||
+ msg->sm_community[0] == '\0') {
stats->snmp_inbadcommunitynames++;
- msg->sm_errstr = "community name too long";
+ msg->sm_errstr = "invalid community name";
goto fail;
}
break;
@@ -295,9 +294,9 @@ snmpe_parse(struct snmp_message *msg)
msg->sm_ctxname[len] = '\0';
break;
default:
- badversion:
- stats->snmp_inbadversions++;
msg->sm_errstr = "bad snmp version";
+badversion:
+ stats->snmp_inbadversions++;
goto fail;
}
@@ -330,10 +329,17 @@ snmpe_parse(struct snmp_message *msg)
msg->sm_errstr = "read requests disabled";
goto fail;
}
+ if (env->sc_rdcommunity[0] == '\0' &&
+ env->sc_rwcommunity[0] == '\0') {
+ if (msg->sm_version == SNMP_V1 ||
+ msg->sm_version == SNMP_V2) {
+ msg->sm_errstr = "community read disabled";
+ goto badversion;
+ }
+ }
if (msg->sm_version != SNMP_V3 &&
strcmp(env->sc_rdcommunity, msg->sm_community) != 0 &&
- (env->sc_readonly ||
- strcmp(env->sc_rwcommunity, msg->sm_community) != 0)) {
+ strcmp(env->sc_rwcommunity, msg->sm_community) != 0) {
stats->snmp_inbadcommunitynames++;
msg->sm_errstr = "wrong read community";
goto fail;
@@ -346,9 +352,15 @@ snmpe_parse(struct snmp_message *msg)
msg->sm_errstr = "write requests disabled";
goto fail;
}
+ if (env->sc_rwcommunity[0] == '\0') {
+ if (msg->sm_version == SNMP_V1 ||
+ msg->sm_version == SNMP_V2) {
+ msg->sm_errstr = "community write disabled";
+ goto badversion;
+ }
+ }
if (msg->sm_version != SNMP_V3 &&
- (env->sc_readonly ||
- strcmp(env->sc_rwcommunity, msg->sm_community) != 0)) {
+ strcmp(env->sc_rwcommunity, msg->sm_community) != 0) {
if (strcmp(env->sc_rdcommunity, msg->sm_community) != 0)
stats->snmp_inbadcommunitynames++;
else
@@ -384,6 +396,13 @@ snmpe_parse(struct snmp_message *msg)
msg->sm_errstr = "SNMPv3 doesn't support traps yet";
goto fail;
}
+ if (env->sc_trcommunity[0] == '\0') {
+ if (msg->sm_version == SNMP_V1 ||
+ msg->sm_version == SNMP_V2) {
+ msg->sm_errstr = "community notify disabled";
+ goto badversion;
+ }
+ }
if (strcmp(env->sc_trcommunity, msg->sm_community) != 0) {
stats->snmp_inbadcommunitynames++;
msg->sm_errstr = "wrong trap community";
@@ -498,19 +517,16 @@ snmpe_parsevarbinds(struct snmp_message
stats->snmp_intotalreqvars++;
break;
case SNMP_C_SETREQ:
- if (snmpd_env->sc_readonly == 0) {
- /*
- * XXX A set varbind should only be committed if
- * all variables are staged
- */
- if (mps_setreq(msg, value, &o) == 0) {
- /* XXX Adjust after fixing staging */
- stats->snmp_intotalsetvars++;
- break;
- }
+ /*
+ * XXX A set varbind should only be committed if
+ * all variables are staged
+ */
+ if (mps_setreq(msg, value, &o) != 0) {
+ msg->sm_error = SNMP_ERROR_READONLY;
+ goto varfail;
}
- msg->sm_error = SNMP_ERROR_READONLY;
- goto varfail;
+ stats->snmp_intotalsetvars++;
+ break;
case SNMP_C_GETBULKREQ:
rvarbind = NULL;
if (mps_getbulkreq(msg, &rvarbind, &end, &o,
Index: usm.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/usm.c,v
retrieving revision 1.19
diff -u -p -r1.19 usm.c
--- usm.c 20 May 2021 08:53:12 -0000 1.19
+++ usm.c 14 Jun 2021 17:39:47 -0000
@@ -177,6 +177,27 @@ usm_newuser(char *name, const char **err
return up;
}
+const struct usmuser *
+usm_check_mincred(int minlevel, const char **errstr)
+{
+ struct usmuser *up;
+
+ if (minlevel == 0)
+ return NULL;
+
+ SLIST_FOREACH(up, &usmuserlist, uu_next) {
+ if (minlevel & SNMP_MSGFLAG_PRIV && up->uu_privkey == NULL) {
+ *errstr = "missing enckey";
+ return up;
+ }
+ if (minlevel & SNMP_MSGFLAG_AUTH && up->uu_authkey == NULL) {
+ *errstr = "missing authkey";
+ return up;
+ }
+ }
+ return NULL;
+}
+
struct usmuser *
usm_finduser(char *name)
{
