From c92471afbdc490ed88a6a702556e7899e9c4b54e Mon Sep 17 00:00:00 2001 From: martijn Date: Sun, 20 Jun 2021 19:55:48 +0000 Subject: [PATCH] Tighten default security for snmpd(8). This is done by doing several things: - Only allow SNMPv3 by default. SNMPv1 and SNMPv2c can be enabled by setting the new snmpv* flags on the "liston on" statements. - Remove the default community names. They're not secure to use. - Change the default seclevel to enc. Initial idea, help from and OK sthen@ --- usr.sbin/snmpd/parse.y | 53 ++++++++++++++++++--------- usr.sbin/snmpd/snmpd.conf.5 | 71 ++++++++++++++++++++----------------- usr.sbin/snmpd/snmpd.h | 17 ++++++--- usr.sbin/snmpd/snmpe.c | 47 +++++++++++++----------- usr.sbin/snmpd/usm.c | 23 +++++++++++- 5 files changed, 136 insertions(+), 75 deletions(-) diff --git a/usr.sbin/snmpd/parse.y b/usr.sbin/snmpd/parse.y index cb9d389ea90..b5bd627f714 100644 --- a/usr.sbin/snmpd/parse.y +++ b/usr.sbin/snmpd/parse.y @@ -1,4 +1,4 @@ -/* $OpenBSD: parse.y,v 1.63 2021/01/22 06:33:26 martijn Exp $ */ +/* $OpenBSD: parse.y,v 1.64 2021/06/20 19:55:48 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -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 STRING %token 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 flags) 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 flags) 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 flags) 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 flags) 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, int type, int flags) 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; diff --git a/usr.sbin/snmpd/snmpd.conf.5 b/usr.sbin/snmpd/snmpd.conf.5 index 1f542a2101a..239c78a956f 100644 --- a/usr.sbin/snmpd/snmpd.conf.5 +++ b/usr.sbin/snmpd/snmpd.conf.5 @@ -1,4 +1,4 @@ -.\" $OpenBSD: snmpd.conf.5,v 1.48 2021/06/14 12:28:58 sthen Exp $ +.\" $OpenBSD: snmpd.conf.5,v 1.49 2021/06/20 19:55:48 martijn Exp $ .\" .\" Copyright (c) 2007, 2008, 2012 Reyk Floeter .\" @@ -14,7 +14,7 @@ .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. .\" -.Dd $Mdocdate: June 14 2021 $ +.Dd $Mdocdate: June 20 2021 $ .Dt SNMPD.CONF 5 .Os .Sh NAME @@ -95,20 +95,30 @@ Routing table information will not be available, but CPU use will be 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,20 +148,17 @@ 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 -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 read-write Ic community Ar string +Specify the name of the read-write community. +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 @@ -158,13 +167,8 @@ Encryption of messages is optional. .It Ic enc Messages must be encrypted and must have a valid digest for authentication. Otherwise they will be discarded. +This is the default value. .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 diff --git a/usr.sbin/snmpd/snmpd.h b/usr.sbin/snmpd/snmpd.h index 7c2db4c8bca..308d76423e9 100644 --- a/usr.sbin/snmpd/snmpd.h +++ b/usr.sbin/snmpd/snmpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: snmpd.h,v 1.95 2021/05/20 08:53:12 martijn Exp $ */ +/* $OpenBSD: snmpd.h,v 1.96 2021/06/20 19:55:48 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -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 snmp_message *, struct ber_element *); 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 diff --git a/usr.sbin/snmpd/snmpe.c b/usr.sbin/snmpd/snmpe.c index 913fc2ae8de..49294633525 100644 --- a/usr.sbin/snmpd/snmpe.c +++ b/usr.sbin/snmpd/snmpe.c @@ -1,4 +1,4 @@ -/* $OpenBSD: snmpe.c,v 1.71 2021/05/20 08:53:12 martijn Exp $ */ +/* $OpenBSD: snmpe.c,v 1.72 2021/06/20 19:55:48 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -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 *msg) 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; diff --git a/usr.sbin/snmpd/usm.c b/usr.sbin/snmpd/usm.c index 5018f861d87..71aaa2e211f 100644 --- a/usr.sbin/snmpd/usm.c +++ b/usr.sbin/snmpd/usm.c @@ -1,4 +1,4 @@ -/* $OpenBSD: usm.c,v 1.19 2021/05/20 08:53:12 martijn Exp $ */ +/* $OpenBSD: usm.c,v 1.20 2021/06/20 19:55:48 martijn Exp $ */ /* * Copyright (c) 2012 GeNUA mbH @@ -177,6 +177,27 @@ usm_newuser(char *name, const char **errp) 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) { -- 2.20.1