On 2021/06/14 19:40, Martijn van Duren wrote:
> 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.
thanks for looking.
> - 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.
I do still think enc is the safer default (i.e. "the user has to do
something to weaken things") though I don't strongly object to auth
instead of enc.
> - 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.
nice.
> - 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.
agreed.
> - Log the disabling of SNMPv{1,2c} properly and increment
> snmp_inbadversions.
this feels like it's overloading snmp_inbadversions a bit,
previously this refers to things which are actually invalid
and since ro and rw can be disabled independently it's hard to
give an accurate log message.
> - 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.
makes sense.
> 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@
from a first read through:
> 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
mistake carried over from my diff; should be "read-only community"
>
> 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;
> + }
> + }
english word order: "read community disabled". i'm not quite sure about
using this errstr/counter though. RO and RW can be disabled independently
and it seems wrong to print "read disabled" only if _both_ RO+RW are
disabled otherwise "wrong read community" if read is disabled and
write is allowed.
on the other hand, this (unset sc_rdcommunity and sc_rwcommunity) is
exactly the case when someone upgrades without allowing for the change,
and they're the main people who will see such a message.
stylistic nit, i'm not a fan of mixing "if (msg->sm_version == SNMP_V1 ||
msg->sm_version == SNMP_V2)" with the existing "if (msg->sm_version !=
SNMP_V3)", they both mean the same at this point (as there's a switch
above) but it seems like unneeded extra cognitive load for the reader
to use 2 different conditionals meaning the same thing. something
like this feels a better fit with the surrounding code to me,
if (msg->sm_version != SNMP_V3 &&
env->sc_rdcommunity[0] == '\0' &&
env->sc_rwcommunity[0] == '\0') {
msg->sm_errstr = "read community disabled";
> 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";
now that there are more things which can be disabled (community as well
as address), i think it would make sense to add "for address" for this
(and the similar errstr for read/notify)
> 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;
> + }
> + }
we're only dealing with the rw community here so my wondering above
about errstr (which i'm undecided about) doesn't apply. my == / != nit
and word ordering "write community disabled" does though.
> 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) {
we are !SNMP_V3 only here because of the above "SNMPv3 doesn't support
traps yet" goto fail, and the block below is also one that would need
!= SNMP_V3 if the above "goto fail" was changed, so i think it would
make sense to either drop the version check, or add a check to the
block below.
> + msg->sm_errstr = "community notify disabled";
this community is defined with config command "trap community" so
probably better use that instead of notify, "trap community 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)
> {
>
>
and i'll try to have another read through and actually test it
in the morning :)
