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"

diff --git a/usr.sbin/snmpd/control.c b/usr.sbin/snmpd/control.c
index 7791576..79d8425 100644 (file)
--- 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 <reyk@openbsd.org>
@@ -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