On Tue, 2021-06-15 at 00:30 +0100, Stuart Henderson wrote:
> 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.
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?
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.
>
> > - 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.
You're right. I overloaded the meaning of "disabling a certain class of
actions for a community" with "this MPS is disabled".
Maybe we should take this a step further and go the way of "read|write|
notify". Diff below adds 3 new flags for the 3 protocols on a per MPS
basis for listen on.
On additional issue with your original diff that I initially overlooked
is that you effectively disable trap receiving since traps currently
don't have SNPMv3 support yet (I need to enable support for users in
different engine IDs to do that). This diff has the best of both worlds:
By default all SNMPv1 and SNMPv2c support is disabled and seclevel is
set to auth. However, if someone explicitly needs lower security it can
be enabled on an per address basis, which includes not losing the
functionality of traps.
>
> > - 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"
Fixed
>
> >
> > 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".
Obsolete with my new diff; It was intended as a shorthand for
"SNMPv1 and SNMPv2 read disabled", but yeah, it was a clunky diff.
> 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.
Obsolete with my new diff; It now simply continues to state it's old
"wrong read community" message if someone enabled snmpv{1,2c} without
setting read-{only,write} community.
>
> 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,
Obsolete with my new diff; I dislike the current != SNMP_V3 because it
feels like a pitfall. However, I have an idea how I want to restructure
this code and it's not worth my time fixing this untill I get around
to that.
>
> 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)
*shrugs* you're probably right. However, this thing is getting big
enough without that baggage. I'd suggest to to leave that for a next
patch.
>
> > 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.
Obsolete with my new diff.
>
> > + 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"
Obsolete with my new diff.
>
>
> > + 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 :)
>
Hopefully you haven't spend too much on a second read.
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 15 Jun 2021 13:39:51 -0000
@@ -120,7 +120,7 @@ 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
@@ -217,7 +217,9 @@ main : LISTEN ON listenproto
free($3);
}
| READWRITE DISABLED {
- conf->sc_readonly = 1;
+ /* XXX Remove after 7.0 */
+ conf->sc_rwcommunity[0] = '\0';
+ log_warnx("'read-write disabled' is deprecated");
}
| TRAP COMMUNITY STRING {
if (strlcpy(conf->sc_trcommunity, $3,
@@ -287,6 +289,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 +300,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 +334,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;
@@ -733,6 +740,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 +1112,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_notify;
if ((conf = calloc(1, sizeof(*conf))) == NULL) {
log_warn("%s", __func__);
@@ -1112,10 +1125,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);
@@ -1141,22 +1152,35 @@ parse_config(const char *filename, u_int
if (listen_add(&ss, SOCK_DGRAM, 0) == -1)
fatal("calloc");
}
- found = 0;
+
+ if ((up = usm_check_mincred(conf->sc_min_seclevel, &errstr)) != NULL)
+ fatalx("user '%s': %s", up->uu_name, errstr);
+ found_notify = 0;
TAILQ_FOREACH(h, &conf->sc_addresses, entry) {
if (h->flags & ADDRESS_FLAG_NOTIFY)
- found = 1;
+ found_notify = 1;
}
- if (conf->sc_traphandler && !found) {
+ if (conf->sc_traphandler && !found_notify) {
log_warnx("trap handler needs at least one notify listener");
free(conf);
return (NULL);
}
- if (!conf->sc_traphandler && found) {
+ if (!conf->sc_traphandler && found_notify) {
log_warnx("notify listener needs at least one trap handler");
free(conf);
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 +1323,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 15 Jun 2021 13:39:51 -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 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 15 Jun 2021 13:39:51 -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 15 Jun 2021 13:39:51 -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 15 Jun 2021 13:39:51 -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 15 Jun 2021 13:39:51 -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 15 Jun 2021 13:39:51 -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
