From: martijn Date: Thu, 1 Sep 2022 14:34:17 +0000 (+0000) Subject: Add privilege separation to snmpd. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=1985d3eb5f6acde8e42cb4da09957b79eaa3d1e3;p=openbsd Add privilege separation to snmpd. This uses the just imported snmpd_metrics as a new (agentx-based) backend. Snmpd(8) executes all files in /usr/libexec/snmpd and treats regions registered by these binaries as authorative, so that no other agentx backends can overwrite them. The snmpe process is now pledged "stdio recvfd inet unix". This removes quite a few entries from the sysORTable, but the current entries are non-compliant anyway and should be completely revisisted at a later time. Reduces the time for a full walk by about a factor of 4, bringing us close to the original speed before application.c was introduced. General design discussed with claudio@ Tested by and OK sthen Release build test and OK tb@ --- diff --git a/usr.sbin/snmpd/application.h b/usr.sbin/snmpd/application.h index 7a9a4c1d697..8b2c5678c3a 100644 --- a/usr.sbin/snmpd/application.h +++ b/usr.sbin/snmpd/application.h @@ -1,4 +1,4 @@ -/* $OpenBSD: application.h,v 1.4 2022/08/29 13:19:05 martijn Exp $ */ +/* $OpenBSD: application.h,v 1.5 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2021 Martijn van Duren @@ -140,6 +140,7 @@ void appl_legacy_shutdown(void); void appl_agentx(void); void appl_agentx_init(void); void appl_agentx_shutdown(void); +void appl_agentx_backend(int); /* application_blocklist.c */ void appl_blocklist_init(void); diff --git a/usr.sbin/snmpd/application_agentx.c b/usr.sbin/snmpd/application_agentx.c index 3b773842cfe..c7a2a2691a3 100644 --- a/usr.sbin/snmpd/application_agentx.c +++ b/usr.sbin/snmpd/application_agentx.c @@ -1,4 +1,4 @@ -/* $OpenBSD: application_agentx.c,v 1.3 2022/08/30 14:54:18 martijn Exp $ */ +/* $OpenBSD: application_agentx.c,v 1.4 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2022 Martijn van Duren * @@ -41,6 +41,13 @@ struct appl_agentx_connection { uint32_t conn_id; + /* + * A backend has several overruling properties: + * - If it exits, snmpd crashes + * - All registrations are priority 1 + * - All registrations own the subtree. + */ + int conn_backend; struct ax *conn_ax; struct event conn_rev; struct event conn_wev; @@ -193,6 +200,7 @@ appl_agentx_accept(int masterfd, short event, void *cookie) goto fail; } + conn->conn_backend = 0; TAILQ_INIT(&(conn->conn_sessions)); if ((conn->conn_ax = ax_new(fd)) == NULL) { log_warn(NULL); @@ -216,6 +224,30 @@ appl_agentx_accept(int masterfd, short event, void *cookie) free(conn); } +void +appl_agentx_backend(int fd) +{ + struct appl_agentx_connection *conn; + + if ((conn = malloc(sizeof(*conn))) == NULL) + fatal(NULL); + + conn->conn_backend = 1; + TAILQ_INIT(&(conn->conn_sessions)); + if ((conn->conn_ax = ax_new(fd)) == NULL) + fatal("ax_new"); + + do { + conn->conn_id = arc4random(); + } while (RB_INSERT(appl_agentx_conns, + &appl_agentx_conns, conn) != NULL); + + event_set(&(conn->conn_rev), fd, EV_READ | EV_PERSIST, + appl_agentx_recv, conn); + event_add(&(conn->conn_rev), NULL); + event_set(&(conn->conn_wev), fd, EV_WRITE, appl_agentx_send, conn); +} + void appl_agentx_free(struct appl_agentx_connection *conn) { @@ -234,6 +266,9 @@ appl_agentx_free(struct appl_agentx_connection *conn) RB_REMOVE(appl_agentx_conns, &appl_agentx_conns, conn); ax_free(conn->conn_ax); + if (conn->conn_backend) + fatalx("AgentX(%"PRIu32"): disappeared unexpected", + conn->conn_id); free(conn); } @@ -529,12 +564,17 @@ appl_agentx_register(struct appl_agentx_session *session, struct ax_pdu *pdu) uint32_t timeout; struct ber_oid oid; enum appl_error error; + int subtree = 0; timeout = pdu->ap_payload.ap_register.ap_timeout; timeout = timeout != 0 ? timeout : session->sess_timeout != 0 ? session->sess_timeout : AGENTX_DEFAULTTIMEOUT; timeout *= 100; + if (session->sess_conn->conn_backend) { + pdu->ap_payload.ap_register.ap_priority = 1; + subtree = 1; + } if (appl_agentx_oid2ber_oid( &(pdu->ap_payload.ap_register.ap_subtree), &oid) == NULL) { log_warnx("%s: Failed to register: oid too small", @@ -545,8 +585,8 @@ appl_agentx_register(struct appl_agentx_session *session, struct ax_pdu *pdu) error = appl_register(pdu->ap_context.aos_string, timeout, pdu->ap_payload.ap_register.ap_priority, &oid, - pdu->ap_header.aph_flags & AX_PDU_FLAG_INSTANCE_REGISTRATION, 0, - pdu->ap_payload.ap_register.ap_range_subid, + pdu->ap_header.aph_flags & AX_PDU_FLAG_INSTANCE_REGISTRATION, + subtree, pdu->ap_payload.ap_register.ap_range_subid, pdu->ap_payload.ap_register.ap_upper_bound, &(session->sess_backend)); diff --git a/usr.sbin/snmpd/mib.c b/usr.sbin/snmpd/mib.c index b034ab8eac6..35eae856ea6 100644 --- a/usr.sbin/snmpd/mib.c +++ b/usr.sbin/snmpd/mib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mib.c,v 1.103 2022/06/30 11:28:36 martijn Exp $ */ +/* $OpenBSD: mib.c,v 1.104 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2012 Joel Knight @@ -3941,6 +3941,7 @@ mib_init(void) /* SNMP-USER-BASED-SM-MIB */ smi_mibtree(usm_mib); +#if 0 /* HOST-RESOURCES-MIB */ smi_mibtree(hr_mib); @@ -3961,4 +3962,5 @@ mib_init(void) /* OPENBSD-MIB */ smi_mibtree(openbsd_mib); +#endif } diff --git a/usr.sbin/snmpd/snmpd.c b/usr.sbin/snmpd/snmpd.c index bc32cc96e7c..627ac94e9d9 100644 --- a/usr.sbin/snmpd/snmpd.c +++ b/usr.sbin/snmpd/snmpd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: snmpd.c,v 1.46 2021/08/10 06:52:03 martijn Exp $ */ +/* $OpenBSD: snmpd.c,v 1.47 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -24,6 +24,7 @@ #include +#include #include #include #include @@ -46,6 +47,7 @@ void snmpd_shutdown(struct snmpd *); void snmpd_sig_handler(int, short, void *); int snmpd_dispatch_snmpe(int, struct privsep_proc *, struct imsg *); int check_child(pid_t, const char *); +void snmpd_backend(struct snmpd *); struct snmpd *snmpd_env; @@ -226,7 +228,10 @@ main(int argc, char *argv[]) gettimeofday(&env->sc_starttime, NULL); env->sc_engine_boots = 0; +/* Remove after 7.2 */ +#if 0 pf_init(); +#endif proc_init(ps, procs, nitems(procs), debug, argc0, argv0, proc_id); if (!debug && daemon(0, 0) == -1) @@ -252,6 +257,7 @@ main(int argc, char *argv[]) signal_add(&ps->ps_evsigusr1, NULL); proc_connect(ps); + snmpd_backend(env); if (pledge("stdio dns sendfd proc exec id", NULL) == -1) fatal("pledge"); @@ -347,3 +353,61 @@ snmpd_engine_time(void) gettimeofday(&now, NULL); return now.tv_sec; } + +void +snmpd_backend(struct snmpd *env) +{ + DIR *dir; + struct dirent *file; + int pair[2]; + char *argv[8]; + char execpath[PATH_MAX]; + size_t i = 0; + + if ((dir = opendir(SNMPD_BACKEND)) == NULL) + fatal("opendir \"%s\"", SNMPD_BACKEND); + + argv[i++] = execpath; + if (env->sc_rtfilter) { + argv[i++] = "-C"; + argv[i++] = "filter-routes"; + } + if (env->sc_flags & SNMPD_F_VERBOSE) + argv[i++] = "-vv"; + if (env->sc_flags & SNMPD_F_DEBUG) { + argv[i++] = "-d"; + argv[i++] = "-x"; + argv[i++] = "3"; + } else { + argv[i++] = "-x"; + argv[i++] = "0"; + } + argv[i] = NULL; + while ((file = readdir(dir)) != NULL) { + if (file->d_name[0] == '.') + continue; + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) == -1) + fatal("socketpair"); + switch (fork()) { + case -1: + fatal("fork"); + case 0: + close(pair[1]); + if (dup2(pair[0], + env->sc_flags & SNMPD_F_DEBUG ? 3 : 0) == -1) + fatal("dup2"); + if (closefrom(env->sc_flags & SNMPD_F_DEBUG ? 4 : 1) == -1) + fatal("closefrom"); + (void)snprintf(execpath, sizeof(execpath), "%s/%s", + SNMPD_BACKEND, file->d_name); + execv(argv[0], argv); + fatal("execv"); + default: + close(pair[0]); + if (proc_compose_imsg(&env->sc_ps, PROC_SNMPE, -1, + IMSG_AX_FD, -1, pair[1], NULL, 0) == -1) + fatal("proc_compose_imsg"); + continue; + } + } +} diff --git a/usr.sbin/snmpd/snmpd.h b/usr.sbin/snmpd/snmpd.h index faa26cd3f3a..5f3f2716fa5 100644 --- a/usr.sbin/snmpd/snmpd.h +++ b/usr.sbin/snmpd/snmpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: snmpd.h,v 1.104 2022/08/23 08:56:21 martijn Exp $ */ +/* $OpenBSD: snmpd.h,v 1.105 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -50,6 +50,7 @@ #define CONF_FILE "/etc/snmpd.conf" #define SNMPD_SOCKET "/var/run/snmpd.sock" +#define SNMPD_BACKEND "/usr/libexec/snmpd" #define SNMPD_USER "_snmpd" #define SNMP_PORT "161" #define SNMPTRAP_PORT "162" @@ -96,7 +97,8 @@ enum imsg_type { IMSG_CTL_VERBOSE, IMSG_CTL_RELOAD, IMSG_CTL_PROCFD, - IMSG_TRAP_EXEC + IMSG_TRAP_EXEC, + IMSG_AX_FD }; struct imsgev { diff --git a/usr.sbin/snmpd/snmpe.c b/usr.sbin/snmpd/snmpe.c index 3fa22de026f..366604e9aff 100644 --- a/usr.sbin/snmpd/snmpe.c +++ b/usr.sbin/snmpd/snmpe.c @@ -1,4 +1,4 @@ -/* $OpenBSD: snmpe.c,v 1.83 2022/08/23 08:56:21 martijn Exp $ */ +/* $OpenBSD: snmpe.c,v 1.84 2022/09/01 14:34:17 martijn Exp $ */ /* * Copyright (c) 2007, 2008, 2012 Reyk Floeter @@ -44,6 +44,7 @@ #include "mib.h" void snmpe_init(struct privsep *, struct privsep_proc *, void *); +int snmpe_dispatch_parent(int, struct privsep_proc *, struct imsg *); int snmpe_parse(struct snmp_message *); void snmpe_tryparse(int, struct snmp_message *); int snmpe_parsevarbinds(struct snmp_message *); @@ -62,7 +63,7 @@ static const struct timeval snmpe_tcp_timeout = { 10, 0 }; /* 10s */ struct snmp_messages snmp_messages = RB_INITIALIZER(&snmp_messages); static struct privsep_proc procs[] = { - { "parent", PROC_PARENT } + { "parent", PROC_PARENT, snmpe_dispatch_parent } }; void @@ -102,8 +103,11 @@ snmpe_init(struct privsep *ps, struct privsep_proc *p, void *arg) struct snmpd *env = ps->ps_env; struct address *h; +/* Remove after 7.2 */ +#if 0 kr_init(); timer_init(); +#endif usm_generate_keys(); appl_init(); @@ -124,8 +128,8 @@ snmpe_init(struct privsep *ps, struct privsep_proc *p, void *arg) /* no filesystem visibility */ if (unveil("/", "") == -1) fatal("unveil /"); - if (unveil(NULL, NULL) == -1) - fatal("unveil"); + if (pledge("stdio recvfd inet unix", NULL) == -1) + fatal("pledge"); log_info("snmpe %s: ready", tohexstr(env->sc_engineid, env->sc_engineid_len)); @@ -143,10 +147,25 @@ snmpe_shutdown(void) event_del(&h->evt); close(h->fd); } +/* Remove after 7.2 */ +#if 0 kr_shutdown(); +#endif appl_shutdown(); } +int +snmpe_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg) +{ + switch (imsg->hdr.type) { + case IMSG_AX_FD: + appl_agentx_backend(imsg->fd); + return 0; + default: + return -1; + } +} + int snmpe_bind(struct address *addr) {