On Tue, 2021-06-15 at 17:39 +0100, Stuart Henderson wrote: > > > > - 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. > > > > I agree that it's safer, but do we want to break the config of more > > people than needed for the goal of preventing simple amplification > > attacks? > > Can we take a straw poll of readers of this email who are using SNMPv3 > (if any ;-) -- are you using auth+enc, just auth, or no authentication? > I'm thinking that somebody who went to the trouble of using v3 > probably uses auth+enc though I could be wrong..
Since the limited of people that responded all responded with auth+enc and you prefer to go with auth+enc I've updated the diff. > > > Then again, I don't get the feeling many people use snmpd at this time > > and maybe it's a good moment to bite the bullet and go for safest > > defaults possible at this time. But if that's the case I would like to > > follow up with a diff to changes the default auth to hmac-sha512, > > because snmp drops trailing bytes of the result and enc to aes instead > > of des. > > This is the change that feels most likely to affect existing SNMPv3 users. > Support in management software beyond aes/sha1 is a bit lacking and prone > to incompatibility (I had issues with net-snmp and snmpd using hmac-sha256 > though it seems it will work with hmac-sha512..) > > > > and i'll try to have another read through and actually test it > > > in the morning :) > > > > > Hopefully you haven't spend too much on a second read. > > didn't get there yet, i have spent the best part of 8 hours on 2 emails > so far today ;) > > > | READWRITE DISABLED { > > - conf->sc_readonly = 1; > > + /* XXX Remove after 7.0 */ > > + conf->sc_rwcommunity[0] = '\0'; > > + log_warnx("'read-write disabled' is deprecated"); > > if it's going, might as well just disable it? almost nobody will react > to that warning unless it refuses to start, it's not like this will > lock someone out of their machine if it doesn't run. Fair enough. Removed in new diff. > > > +.It Ic write > > +Specifies if the listen address accepts set requests. > > +.It Ic notify > > +Specifies if the listen address accepts trapv1 and trapv2 requests. > > +.It Ic snmpv1 > > +Enables SNMPv1 subsystem on the listen address. > > +.It Ic snmpv2c > > +Enables SNMPv2c subsystem on the listen address. > > +.It Ic snmpv3 > > +Enables SNMPv3 subsystem on the listen address. > > I like this! I guess we could also do > > listen on 127.0.0.1 snmpv2c read > listen on 0.0.0.0 read > listen on :: read > read-only community public > > to allow localhost requests with v2c for quick lookups and require > something better on the network. > > I'll do some testing and get back to you. > I didn't change the example, since the example below shows how to set up snmpv3 and this example's accompanying text is already on the long side. I did change the text a little to "for SNMPv2c messages only", so that it's clearer that this does disable snmpv3. OK? martijn@ Index: usr.sbin/snmpd/parse.y =================================================================== RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v retrieving revision 1.63 diff -u -p -r1.63 parse.y --- usr.sbin/snmpd/parse.y 22 Jan 2021 06:33:26 -0000 1.63 +++ usr.sbin/snmpd/parse.y 20 Jun 2021 10:08:00 -0000 @@ -120,10 +120,10 @@ typedef struct { %} %token INCLUDE -%token LISTEN ON READ WRITE NOTIFY +%token LISTEN ON READ WRITE NOTIFY SNMPV1 SNMPV2 SNMPV3 %token SYSTEM CONTACT DESCR LOCATION NAME OBJECTID SERVICES RTFILTER %token READONLY READWRITE OCTETSTRING INTEGER COMMUNITY TRAP RECEIVER -%token SECLEVEL NONE AUTH ENC USER AUTHKEY ENCKEY ERROR DISABLED +%token SECLEVEL NONE AUTH ENC USER AUTHKEY ENCKEY ERROR %token HANDLE DEFAULT SRCADDR TCP UDP PFADDRFILTER PORT %token <v.string> STRING %token <v.number> NUMBER @@ -216,9 +216,6 @@ main : LISTEN ON listenproto } free($3); } - | READWRITE DISABLED { - conf->sc_readonly = 1; - } | TRAP COMMUNITY STRING { if (strlcpy(conf->sc_trcommunity, $3, sizeof(conf->sc_trcommunity)) >= @@ -287,6 +284,9 @@ listenopts : /* empty */ { $$ = 0; } listenopt : READ { $$ = ADDRESS_FLAG_READ; } | WRITE { $$ = ADDRESS_FLAG_WRITE; } | NOTIFY { $$ = ADDRESS_FLAG_NOTIFY; } + | SNMPV1 { $$ = ADDRESS_FLAG_SNMPV1; } + | SNMPV2 { $$ = ADDRESS_FLAG_SNMPV2; } + | SNMPV3 { $$ = ADDRESS_FLAG_SNMPV3; } ; listen_udp : STRING port listenopts { @@ -295,7 +295,8 @@ listen_udp : STRING port listenopts { char *port = $2; if (port == NULL) { - if ($3 == ADDRESS_FLAG_NOTIFY) + if (($3 & ADDRESS_FLAG_PERM) == + ADDRESS_FLAG_NOTIFY) port = SNMPTRAP_PORT; else port = SNMP_PORT; @@ -328,7 +329,8 @@ listen_tcp : STRING port listenopts { char *port = $2; if (port == NULL) { - if ($3 == ADDRESS_FLAG_NOTIFY) + if (($3 & ADDRESS_FLAG_PERM) == + ADDRESS_FLAG_NOTIFY) port = SNMPTRAP_PORT; else port = SNMP_PORT; @@ -711,7 +713,6 @@ lookup(char *s) { "contact", CONTACT }, { "default", DEFAULT }, { "description", DESCR }, - { "disabled", DISABLED}, { "enc", ENC }, { "enckey", ENCKEY }, { "filter-pf-addresses", PFADDRFILTER }, @@ -733,6 +734,9 @@ lookup(char *s) { "receiver", RECEIVER }, { "seclevel", SECLEVEL }, { "services", SERVICES }, + { "snmpv1", SNMPV1 }, + { "snmpv2c", SNMPV2 }, + { "snmpv3", SNMPV3 }, { "source-address", SRCADDR }, { "string", OCTETSTRING }, { "system", SYSTEM }, @@ -1102,7 +1106,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 +1119,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 | SNMP_MSGFLAG_PRIV; if ((file = pushfile(filename, 0)) == NULL) { free(conf); @@ -1141,6 +1146,10 @@ parse_config(const char *filename, u_int if (listen_add(&ss, SOCK_DGRAM, 0) == -1) fatal("calloc"); } + + if ((up = usm_check_mincred(conf->sc_min_seclevel, &errstr)) != NULL) + fatalx("user '%s': %s", up->uu_name, errstr); + found = 0; TAILQ_FOREACH(h, &conf->sc_addresses, entry) { if (h->flags & ADDRESS_FLAG_NOTIFY) @@ -1157,6 +1166,16 @@ parse_config(const char *filename, u_int return (NULL); } + if (conf->sc_trcommunity[0] == '\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. */ TAILQ_FOREACH_SAFE(sym, &symhead, entry, next) { if ((conf->sc_flags & SNMPD_F_VERBOSE) && !sym->used) @@ -1299,12 +1318,14 @@ listen_add(struct sockaddr_storage *ss, h->port = ntohs(sin6->sin6_port); } h->type = type; - if ((h->flags = flags) == 0) { + if (((h->flags = flags) & ADDRESS_FLAG_PERM) == 0) { if (h->port == 162) - h->flags = ADDRESS_FLAG_NOTIFY; + h->flags |= ADDRESS_FLAG_NOTIFY; else - h->flags = ADDRESS_FLAG_READ | ADDRESS_FLAG_WRITE; + h->flags |= ADDRESS_FLAG_READ | ADDRESS_FLAG_WRITE; } + if ((h->flags & ADDRESS_FLAG_MPS) == 0) + h->flags |= ADDRESS_FLAG_SNMPV3; TAILQ_INSERT_TAIL(&(conf->sc_addresses), h, entry); return 0; Index: usr.sbin/snmpd/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 --- usr.sbin/snmpd/snmpd.conf.5 14 Jun 2021 12:28:58 -0000 1.48 +++ usr.sbin/snmpd/snmpd.conf.5 20 Jun 2021 10:08:00 -0000 @@ -95,20 +95,30 @@ Routing table information will not be av reduced during bulk updates. The default is .Ic no . -.It Ic listen on Oo Ic tcp | udp Oc Ar address Oo Ic port Ar port Oc Op Ic read | Ic write | Ic notify +.It Ic listen on Oo Ic tcp | udp Oc Ar address Oo Ic port Ar port Oc Op Ar flags Specify the local address .Xr snmpd 8 should listen on for incoming SNMP messages. +.Pp The -.Ic read -flag specifies if the listen address accepts get, getnext and bulkget +.Ar flags +are as follows: +.Bl -tag -width Ds +.It Ic read +Specifies if the listen address accepts get, getnext and bulkget requests. -The -.Ic write -flag specifies if the listen address accepts set requests -and the -.Ic notify -flag specifies if the listen address accepts trapv1 and trapv2 requests. +.It Ic write +Specifies if the listen address accepts set requests. +.It Ic notify +Specifies if the listen address accepts trapv1 and trapv2 requests. +.It Ic snmpv1 +Enables SNMPv1 subsystem on the listen address. +.It Ic snmpv2c +Enables SNMPv2c subsystem on the listen address. +.It Ic snmpv3 +Enables SNMPv3 subsystem on the listen address. +.El +.Pp Multiple .Ic listen on statements are supported. @@ -118,17 +128,19 @@ The default .Ar port is 161, unless .Ic notify -is the only listen flags -which sets the +is the only permission flag; which sets the .Ar port to 162. -If no flags are specified it defaults to +If no permission flags are specified it defaults to .Dq Ic read Ic write , or .Ic notify when .Ar port is 162. +If no subsystem flags are specified it defaults to +.Ic snmpv3 . +.Pp Having .Ic notify set requires at least one @@ -136,35 +148,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 +210,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 @@ -326,10 +329,12 @@ Example configuration file. .Sh EXAMPLES The following example will tell .Xr snmpd 8 -to listen on localhost, override the default system OID, set the -magic services value and provides some custom OID values: +to listen on localhost for SNMPv2c messages only with the public community, +override the default system OID, set the magic services value and provides some +custom OID values: .Bd -literal -offset indent -listen on 127.0.0.1 +listen on 127.0.0.1 snmpv2c +read-only community public system oid 1.3.6.1.4.1.30155.23.2 system services 74 Index: usr.sbin/snmpd/snmpd.h =================================================================== RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v retrieving revision 1.95 diff -u -p -r1.95 snmpd.h --- usr.sbin/snmpd/snmpd.h 20 May 2021 08:53:12 -0000 1.95 +++ usr.sbin/snmpd/snmpd.h 20 Jun 2021 10:08:00 -0000 @@ -498,9 +498,16 @@ struct address { }; TAILQ_HEAD(addresslist, address); -#define ADDRESS_FLAG_READ 0x1 -#define ADDRESS_FLAG_WRITE 0x2 -#define ADDRESS_FLAG_NOTIFY 0x4 +#define ADDRESS_FLAG_READ 0x01 +#define ADDRESS_FLAG_WRITE 0x02 +#define ADDRESS_FLAG_NOTIFY 0x04 +#define ADDRESS_FLAG_PERM \ + (ADDRESS_FLAG_READ | ADDRESS_FLAG_WRITE | ADDRESS_FLAG_NOTIFY) +#define ADDRESS_FLAG_SNMPV1 0x10 +#define ADDRESS_FLAG_SNMPV2 0x20 +#define ADDRESS_FLAG_SNMPV3 0x40 +#define ADDRESS_FLAG_MPS \ + (ADDRESS_FLAG_SNMPV1 | ADDRESS_FLAG_SNMPV2 | ADDRESS_FLAG_SNMPV3) struct trap_address { struct sockaddr_storage ss; @@ -576,7 +583,6 @@ struct snmpd { int sc_pfaddrfilter; int sc_min_seclevel; - int sc_readonly; int sc_traphandler; struct privsep sc_ps; @@ -740,6 +746,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: usr.sbin/snmpd/snmpe.c =================================================================== RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v retrieving revision 1.71 diff -u -p -r1.71 snmpe.c --- usr.sbin/snmpd/snmpe.c 20 May 2021 08:53:12 -0000 1.71 +++ usr.sbin/snmpd/snmpe.c 20 Jun 2021 10:08:01 -0000 @@ -254,19 +254,31 @@ snmpe_parse(struct snmp_message *msg) msg->sm_version = ver; switch (msg->sm_version) { case SNMP_V1: + if (!(msg->sm_aflags & ADDRESS_FLAG_SNMPV1)) { + msg->sm_errstr = "SNMPv1 disabled"; + goto badversion; + } case SNMP_V2: - if (env->sc_min_seclevel != 0) + if (msg->sm_version == SNMP_V2 && + !(msg->sm_aflags & ADDRESS_FLAG_SNMPV2)) { + msg->sm_errstr = "SNMPv2c disabled"; 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; case SNMP_V3: + if (!(msg->sm_aflags & ADDRESS_FLAG_SNMPV3)) { + msg->sm_errstr = "SNMPv3 disabled"; + goto badversion; + } if (ober_scanf_elements(a, "{iisi$}e", &msg->sm_msgid, &msg->sm_max_msg_size, &flagstr, &msg->sm_secmodel, &a) != 0) @@ -295,9 +307,9 @@ snmpe_parse(struct snmp_message *msg) msg->sm_ctxname[len] = '\0'; break; default: - badversion: + msg->sm_errstr = "unsupported snmp version"; +badversion: stats->snmp_inbadversions++; - msg->sm_errstr = "bad snmp version"; goto fail; } @@ -332,8 +344,7 @@ snmpe_parse(struct snmp_message *msg) } 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; @@ -347,8 +358,7 @@ snmpe_parse(struct snmp_message *msg) goto fail; } 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 @@ -498,16 +508,13 @@ 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) { + stats->snmp_intotalsetvars++; + break; } msg->sm_error = SNMP_ERROR_READONLY; goto varfail; Index: usr.sbin/snmpd/usm.c =================================================================== RCS file: /cvs/src/usr.sbin/snmpd/usm.c,v retrieving revision 1.19 diff -u -p -r1.19 usm.c --- usr.sbin/snmpd/usm.c 20 May 2021 08:53:12 -0000 1.19 +++ usr.sbin/snmpd/usm.c 20 Jun 2021 10:08:01 -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) { Index: regress/usr.sbin/snmpd/snmpd.sh =================================================================== RCS file: /cvs/src/regress/usr.sbin/snmpd/snmpd.sh,v retrieving revision 1.13 diff -u -p -r1.13 snmpd.sh --- regress/usr.sbin/snmpd/snmpd.sh 22 Jan 2021 06:35:26 -0000 1.13 +++ regress/usr.sbin/snmpd/snmpd.sh 20 Jun 2021 10:08:01 -0000 @@ -64,14 +64,19 @@ echo "\nConfiguration: default community cat > ${OBJDIR}/snmpd.conf <<EOF # This is config template (1) for snmpd regression testing # Restrict daemon to listen on localhost only -listen on 127.0.0.1 -listen on 127.0.0.1 notify -listen on ::1 -listen on ::1 notify +listen on 127.0.0.1 snmpv1 snmpv2c snmpv3 +listen on 127.0.0.1 snmpv2c notify +listen on ::1 snmpv1 snmpv2c snmpv3 +listen on ::1 snmpv2c notify # Specify a number of trap receivers trap receiver localhost +# Specify communities +read-only community public +read-write community private +trap community public + trap handle 1.2.3.4 "/usr/bin/touch ${TMPFILE}" EOF @@ -130,7 +135,7 @@ carp_allow="$(eval $snmp_command)" carp_allow="${carp_allow##.1.3.6.1.4.1.30155.6.1.1.0 }" if [ "$carp" -ne "$carp_allow" ] then - echo "Retrieval of carp.allow with default ro cummunity string failed." + echo "Retrieval of carp.allow with default ro community string failed." FAILED=1 fi @@ -258,8 +263,8 @@ boe="$(printf '\303')" cat > ${OBJDIR}/snmpd.conf <<EOF # This is config template (4) for snmpd regression testing # Restrict daemon to listen on localhost only -listen on 127.0.0.1 -listen on ::1 +listen on 127.0.0.1 snmpv1 snmpv2c +listen on ::1 snmpv1 snmpv2c read-only community non-default-ro @@ -288,7 +293,7 @@ carp_allow="$(eval $snmp_command)" carp_allow="${carp_allow##.iso.org.dod.internet.private.enterprises.openBSD.carpMIBObjects.carpSysctl.carpAllow.0 = }" if [ "$carp" -ne "$carp_allow" ] then - echo "Retrieval test with default ro cummunity string failed." + echo "Retrieval test with default ro community string failed." FAILED=1 fi Index: regress/usr.bin/snmp/Makefile =================================================================== RCS file: /cvs/src/regress/usr.bin/snmp/Makefile,v retrieving revision 1.1 diff -u -p -r1.1 Makefile --- regress/usr.bin/snmp/Makefile 9 Mar 2021 17:38:24 -0000 1.1 +++ regress/usr.bin/snmp/Makefile 20 Jun 2021 10:08:01 -0000 @@ -23,11 +23,14 @@ snmpd.conf: Makefile printf 'listen_addr="127.0.0.1"\n' > snmpd.conf printf 'listen6_addr="::1"\n\n' >> snmpd.conf printf '# Restrict daemon to listen on localhost only\n' >> snmpd.conf - printf 'listen on $$listen_addr\n' >> snmpd.conf - printf 'listen on tcp $$listen_addr\n' >> snmpd.conf - printf 'listen on $$listen6_addr\n' >> snmpd.conf - printf 'listen on tcp $$listen6_addr\n' >> snmpd.conf - printf 'listen on $$listen_addr notify\n\n' >> snmpd.conf + printf 'listen on $$listen_addr snmpv1 snmpv2c snmpv3\n' >> snmpd.conf + printf 'listen on tcp $$listen_addr snmpv1 snmpv2c snmpv3\n' >> snmpd.conf + printf 'listen on $$listen6_addr snmpv1 snmpv2c snmpv3\n' >> snmpd.conf + printf 'listen on tcp $$listen6_addr snmpv1 snmpv2c snmpv3\n' >> snmpd.conf + printf 'listen on $$listen_addr notify snmpv1 snmpv2c snmpv3\n\n' >> snmpd.conf + printf 'read-only community public\n' >> snmpd.conf + printf 'read-write community private\n' >> snmpd.conf + printf 'trap community public\n\n' >> snmpd.conf printf '# (ab)use sysContact for DisplayString (255a) testing\n' >> snmpd.conf printf 'system contact "Reyk Fl\303\266ter"\n' >> snmpd.conf printf 'system services 74\n\n' >> snmpd.conf