From 85b47614ef202e179cc9c0a417acc2d068a4b38e Mon Sep 17 00:00:00 2001 From: reyk Date: Sat, 17 Oct 2015 10:20:33 +0000 Subject: [PATCH] Tighten up snmpd's control socket: do not allow users to terminate the daemon by sending corrupted imsgs to snmpd. This is especially important for the optional world-writeable restricted socket that is used for AgentX. In particular, don't fatal() in the daemon when imsg size checks on control messages fail, do stricter validation of expected messages (even assert zero-length imsgs), don't continue and close the control socket on suspicious input, print a debug log message on error. OK gilles@ "the rationale behind it is quite clear" --- usr.sbin/snmpd/control.c | 61 +++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/usr.sbin/snmpd/control.c b/usr.sbin/snmpd/control.c index 77915764f5b..79d84257b80 100644 --- a/usr.sbin/snmpd/control.c +++ b/usr.sbin/snmpd/control.c @@ -1,4 +1,4 @@ -/* $OpenBSD: control.c,v 1.30 2015/10/02 13:13:05 reyk Exp $ */ +/* $OpenBSD: control.c,v 1.31 2015/10/17 10:20:33 reyk Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -42,7 +42,7 @@ struct ctl_connlist ctl_conns; static int agentx_sessionid = 1; void control_accept(int, short, void *); -void control_close(struct ctl_conn *); +void control_close(struct ctl_conn *, const char *, struct imsg *); void control_dispatch_imsg(int, short, void *); void control_dispatch_agentx(int, short, void *); void control_imsg_forward(struct imsg *); @@ -202,10 +202,17 @@ control_accept(int listenfd, short event, void *arg) } void -control_close(struct ctl_conn *c) +control_close(struct ctl_conn *c, const char *msg, struct imsg *imsg) { struct control_sock *cs = c->cs; + if (imsg) { + log_debug("%s: %s, imsg %d datalen %u", __func__, msg, + imsg->hdr.type, IMSG_DATA_SIZE(imsg)); + imsg_free(imsg); + } else + log_debug("%s: %s", __func__, msg); + msgbuf_clear(&c->iev.ibuf.w); TAILQ_REMOVE(&ctl_conns, c, entry); @@ -233,20 +240,20 @@ control_dispatch_imsg(int fd, short event, void *arg) if (event & EV_READ) { if ((n = imsg_read_nofd(&c->iev.ibuf)) == -1 || n == 0) { - control_close(c); + control_close(c, "could not read imsg", NULL); return; } } if (event & EV_WRITE) { if (msgbuf_write(&c->iev.ibuf.w) <= 0 && errno != EAGAIN) { - control_close(c); + control_close(c, "could not write imsg", NULL); return; } } for (;;) { if ((n = imsg_get(&c->iev.ibuf, &imsg)) == -1) { - control_close(c); + control_close(c, "could not get imsg", NULL); return; } @@ -261,10 +268,9 @@ control_dispatch_imsg(int fd, short event, void *arg) case IMSG_SNMP_LOCK: break; default: - log_debug("control_dispatch_imsg: " - "client requested restricted command"); - imsg_free(&imsg); - control_close(c); + control_close(c, + "client requested restricted command", + &imsg); return; } } @@ -273,6 +279,9 @@ control_dispatch_imsg(int fd, short event, void *arg) switch (imsg.hdr.type) { case IMSG_CTL_NOTIFY: + if (IMSG_DATA_SIZE(&imsg)) + return control_close(c, "invalid size", &imsg); + if (c->flags & CTL_CONN_NOTIFY) { log_debug("%s: " "client requested notify more than once", @@ -285,29 +294,32 @@ control_dispatch_imsg(int fd, short event, void *arg) break; case IMSG_SNMP_LOCK: + if (IMSG_DATA_SIZE(&imsg)) + return control_close(c, "invalid size", &imsg); + /* enable restricted control mode */ c->flags |= CTL_CONN_LOCKED; break; case IMSG_SNMP_AGENTX: + if (IMSG_DATA_SIZE(&imsg)) + return control_close(c, "invalid size", &imsg); /* rendezvous with the client */ imsg_compose_event(&c->iev, IMSG_CTL_OK, 0, 0, -1, NULL, 0); if (imsg_flush(&c->iev.ibuf) == -1) { - log_debug("control_dispatch_imsg: " - "could not rendezvous with client"); - imsg_free(&imsg); - control_close(c); + control_close(c, + "could not rendezvous with agentx client", + &imsg); return; } /* enable AgentX socket */ c->handle = snmp_agentx_alloc(c->iev.ibuf.fd); if (c->handle == NULL) { - log_debug("control_dispatch_imsg: " - "could not allocate restricted socket"); - imsg_free(&imsg); - control_close(c); + control_close(c, + "could not allocate agentx socket", + &imsg); return; } /* disable IMSG notifications */ @@ -317,7 +329,8 @@ control_dispatch_imsg(int fd, short event, void *arg) break; case IMSG_CTL_VERBOSE: - IMSG_SIZE_CHECK(&imsg, &v); + if (IMSG_DATA_SIZE(&imsg) != sizeof(v)) + return control_close(c, "invalid size", &imsg); memcpy(&v, imsg.data, sizeof(v)); log_verbose(v); @@ -329,13 +342,15 @@ control_dispatch_imsg(int fd, short event, void *arg) } break; case IMSG_CTL_RELOAD: + if (IMSG_DATA_SIZE(&imsg)) + return control_close(c, "invalid size", &imsg); proc_forward_imsg(&env->sc_ps, &imsg, PROC_PARENT, -1); break; default: - log_debug("%s: error handling imsg %d", - __func__, imsg.hdr.type); - break; + control_close(c, "invalid type", &imsg); + return; } + imsg_free(&imsg); } @@ -642,7 +657,7 @@ control_dispatch_agentx(int fd, short event, void *arg) purge_registered_oids(&c->oids); if (varcpy) free(varcpy); - control_close(c); + control_close(c, "agentx teardown", NULL); } void -- 2.20.1