So here's an elusive one that can be triggered every now and then by the
new regression test. Once an AgentX session is opened and we send an
invalid packet appl_agentx_recv() goes to appl_agentx_free(), since
there's no recovery. appl_agentx_free() tries to neatly close all
open sessions by sending a close-pdu, followed by calling
appl_agentx_send() directly.
However, if the socket has been closed in the meantime we hit
appl_agentx_send()'s error path, which also calls appl_agentx_free().
This in turn leads to use after free cases.
To fix this don't call appl_agentx_send() directly anymore, but just
schedule it via conn_wev. To make sure as much data as possible is
written out do a last unchecked courtesy flush before definitively
freeing the connection. Since appl_agentx_forceclose() arms conn_wev
move the event_del() calls down in appl_agentx_free().
Other calls of appl_agentx_send() should be fine, but just convert
all of them to be consistent and safe.
OK?
martijn@
Index: usr.sbin/snmpd/application_agentx.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/application_agentx.c,v
retrieving revision 1.12
diff -u -p -r1.12 application_agentx.c
--- usr.sbin/snmpd/application_agentx.c 24 Oct 2023 14:11:14 -0000 1.12
+++ usr.sbin/snmpd/application_agentx.c 26 Oct 2023 08:43:02 -0000
@@ -254,9 +254,6 @@ appl_agentx_free(struct appl_agentx_conn
{
struct appl_agentx_session *session;
- event_del(&(conn->conn_rev));
- event_del(&(conn->conn_wev));
-
while ((session = TAILQ_FIRST(&(conn->conn_sessions))) != NULL) {
if (conn->conn_ax == NULL)
appl_agentx_session_free(session);
@@ -265,7 +262,12 @@ appl_agentx_free(struct appl_agentx_conn
reason);
}
+ event_del(&(conn->conn_rev));
+ event_del(&(conn->conn_wev));
+
RB_REMOVE(appl_agentx_conns, &appl_agentx_conns, conn);
+ if (conn->conn_ax != NULL)
+ (void)ax_send(conn->conn_ax);
ax_free(conn->conn_ax);
if (conn->conn_backend)
fatalx("AgentX(%"PRIu32"): disappeared unexpected",
@@ -419,7 +421,7 @@ appl_agentx_recv(int fd, short event, vo
pdu->ap_header.aph_transactionid,
pdu->ap_header.aph_packetid, smi_getticks(),
APPL_ERROR_NOERROR, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
break;
case AX_PDU_TYPE_INDEXALLOCATE:
case AX_PDU_TYPE_INDEXDEALLOCATE:
@@ -431,7 +433,7 @@ appl_agentx_recv(int fd, short event, vo
APPL_ERROR_PROCESSINGERROR, 1,
pdu->ap_payload.ap_vbl.ap_varbind,
pdu->ap_payload.ap_vbl.ap_nvarbind);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
break;
case AX_PDU_TYPE_ADDAGENTCAPS:
case AX_PDU_TYPE_REMOVEAGENTCAPS:
@@ -451,7 +453,7 @@ appl_agentx_recv(int fd, short event, vo
pdu->ap_header.aph_transactionid,
pdu->ap_header.aph_packetid, smi_getticks(),
error, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
ax_pdu_free(pdu);
if (session == NULL || error != APPL_ERROR_PARSEERROR)
@@ -560,13 +562,13 @@ appl_agentx_open(struct appl_agentx_conn
ax_response(conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
return;
fail:
ax_response(conn->conn_ax, 0, pdu->ap_header.aph_transactionid,
pdu->ap_header.aph_packetid, 0, error, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
if (session != NULL)
free(session->sess_descr.aos_string);
free(session);
@@ -592,7 +594,7 @@ appl_agentx_close(struct appl_agentx_ses
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
smi_getticks(), error, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ event_add(&(conn->conn_wev), NULL);
if (error == APPL_ERROR_NOERROR)
return;
@@ -612,7 +614,7 @@ appl_agentx_forceclose(struct appl_backe
session->sess_conn->conn_ax->ax_byteorder = session->sess_byteorder;
ax_close(session->sess_conn->conn_ax, session->sess_id,
(enum ax_close_reason) reason);
- appl_agentx_send(-1, EV_WRITE, session->sess_conn);
+ event_add(&(session->sess_conn->conn_wev), NULL);
strlcpy(name, session->sess_backend.ab_name, sizeof(name));
appl_agentx_session_free(session);
@@ -671,7 +673,7 @@ appl_agentx_register(struct appl_agentx_
ax_response(session->sess_conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
smi_getticks(), error, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, session->sess_conn);
+ event_add(&(session->sess_conn->conn_wev), NULL);
}
void
@@ -698,7 +700,7 @@ appl_agentx_unregister(struct appl_agent
ax_response(session->sess_conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
smi_getticks(), error, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, session->sess_conn);
+ event_add(&(session->sess_conn->conn_wev), NULL);
}
#define AX_PDU_FLAG_INDEX (AX_PDU_FLAG_NEW_INDEX | AX_PDU_FLAG_ANY_INDEX)
@@ -748,7 +750,7 @@ appl_agentx_get(struct appl_backend *bac
requestid, context, srl, nsr) == -1)
appl_response(backend, requestid, APPL_ERROR_GENERR, 1, vblist);
else
- appl_agentx_send(-1, EV_WRITE, session->sess_conn);
+ event_add(&(session->sess_conn->conn_wev), NULL);
free(srl);
if (context != NULL)
free(context->aos_string);
@@ -801,7 +803,7 @@ appl_agentx_getnext(struct appl_backend
requestid, context, srl, nsr) == -1)
appl_response(backend, requestid, APPL_ERROR_GENERR, 1, vblist);
else
- appl_agentx_send(-1, EV_WRITE, session->sess_conn);
+ event_add(&(session->sess_conn->conn_wev), NULL);
free(srl);
if (context != NULL)
free(context->aos_string);
Index: regress/usr.sbin/snmpd/Makefile
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- regress/usr.sbin/snmpd/Makefile 24 Oct 2023 14:34:40 -0000 1.5
+++ regress/usr.sbin/snmpd/Makefile 26 Oct 2023 08:43:02 -0000
@@ -51,6 +51,7 @@ AGENTX_TARGETS+= agentx_ping_new_index
AGENTX_TARGETS+= agentx_ping_any_index
AGENTX_TARGETS+= agentx_ping_nbo_nnbo
AGENTX_TARGETS+= agentx_ping_nnbo_nbo
+AGENTX_TARGETS+= agentx_ping_invalid_version_close
AGENTX_TARGETS+= agentx_close_notopen
AGENTX_TARGETS+= agentx_close_reasonother
AGENTX_TARGETS+= agentx_close_reasonparseerror
Index: regress/usr.sbin/snmpd/agentx.c
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/agentx.c,v
retrieving revision 1.1
diff -u -p -r1.1 agentx.c
--- regress/usr.sbin/snmpd/agentx.c 24 Oct 2023 14:34:40 -0000 1.1
+++ regress/usr.sbin/snmpd/agentx.c 26 Oct 2023 08:43:02 -0000
@@ -706,6 +706,45 @@ agentx_ping_nnbo_nbo(void)
close(s);
}
+/*
+ * Test that everything continues running in double exception condition
+ */
+void
+agentx_ping_invalid_version_close(void)
+{
+ struct sockaddr_storage ss;
+ struct message msg = {};
+ struct sockaddr *sa = (struct sockaddr *)&ss;
+ socklen_t salen;
+ int snmp_s, ax_s;
+ uint32_t sessionid, packetid;
+ struct varbind varbind = {
+ .type = TYPE_NULL,
+ .name = OID_STRUCT(MIB_SUBAGENT_PING, 10, 0),
+ };
+ int32_t requestid;
+ char buf[1024];
+ size_t n;
+
+ ax_s = agentx_connect(axsocket);
+ sessionid = agentx_open(ax_s, 0, 0,
+ OID_ARG(MIB_SUBAGENT_PING, 10), __func__);
+ message_add_header(&msg, 0xFF, AGENTX_PING_PDU, INSTANCE_REGISTRATION,
+ sessionid, 0, packetid);
+
+ agentx_write(ax_s, &msg);
+ close(ax_s);
+
+ salen = snmp_resolve(SOCK_DGRAM, hostname, servname, sa);
+ snmp_s = snmp_connect(SOCK_DGRAM, sa, salen);
+ requestid = snmpv2_get(snmp_s, community, 0, &varbind, 1);
+
+ varbind.type = TYPE_NOSUCHOBJECT;
+
+ snmpv2_response_validate(snmp_s, 1000, community, requestid, 0, 0,
+ &varbind, 1);
+}
+
void
agentx_close_notopen(void)
{
Index: regress/usr.sbin/snmpd/regress.h
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/regress.h,v
retrieving revision 1.1
diff -u -p -r1.1 regress.h
--- regress/usr.sbin/snmpd/regress.h 24 Oct 2023 14:34:40 -0000 1.1
+++ regress/usr.sbin/snmpd/regress.h 26 Oct 2023 08:43:02 -0000
@@ -170,6 +170,7 @@ void agentx_ping_new_index(void);
void agentx_ping_any_index(void);
void agentx_ping_nbo_nnbo(void);
void agentx_ping_nnbo_nbo(void);
+void agentx_ping_invalid_version_close(void);
void agentx_close_notopen(void);
void agentx_close_reasonother(void);
void agentx_close_reasonparseerror(void);
Index: regress/usr.sbin/snmpd/snmpd_regress.c
===================================================================
RCS file: /cvs/src/regress/usr.sbin/snmpd/snmpd_regress.c,v
retrieving revision 1.1
diff -u -p -r1.1 snmpd_regress.c
--- regress/usr.sbin/snmpd/snmpd_regress.c 24 Oct 2023 14:34:40 -0000
1.1
+++ regress/usr.sbin/snmpd/snmpd_regress.c 26 Oct 2023 08:43:02 -0000
@@ -36,6 +36,7 @@ const struct {
{ "agentx_ping_any_index", agentx_ping_any_index },
{ "agentx_ping_nbo_nnbo", agentx_ping_nbo_nnbo },
{ "agentx_ping_nnbo_nbo", agentx_ping_nnbo_nbo },
+ { "agentx_ping_invalid_version_close",
agentx_ping_invalid_version_close },
{ "agentx_close_notopen", agentx_close_notopen },
{ "agentx_close_reasonother", agentx_close_reasonother },
{ "agentx_close_reasonparseerror", agentx_close_reasonparseerror },
