From eb6ea173c1ab1320d1dfbf35c07e747499699de0 Mon Sep 17 00:00:00 2001 From: martijn Date: Fri, 27 Oct 2023 10:23:58 +0000 Subject: [PATCH] Use event_add(), instead of calling appl_agentx_send() directly. If an error occurs the connection will be freed and if the caller uses the connection afterwards it will lead to a use after free. OK tb@ --- usr.sbin/snmpd/application_agentx.c | 32 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/usr.sbin/snmpd/application_agentx.c b/usr.sbin/snmpd/application_agentx.c index 2d13f04e8a0..c661cda6192 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.12 2023/10/24 14:11:14 martijn Exp $ */ +/* $OpenBSD: application_agentx.c,v 1.13 2023/10/27 10:23:58 martijn Exp $ */ /* * Copyright (c) 2022 Martijn van Duren * @@ -254,9 +254,6 @@ appl_agentx_free(struct appl_agentx_connection *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_connection *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, void *cookie) 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, void *cookie) 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, void *cookie) 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_connection *conn, struct ax_pdu *pdu) 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_session *session, struct ax_pdu *pdu) 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_backend *backend, 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_session *session, struct ax_pdu *pdu) 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_agentx_session *session, struct ax_pdu *pdu) 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 *backend, int32_t transactionid, 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 *backend, int32_t transactionid, 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); -- 2.20.1