From 69014b584bf8ac74d9ea974654ab2ee01fdae6f6 Mon Sep 17 00:00:00 2001 From: martijn Date: Tue, 24 Oct 2023 12:57:40 +0000 Subject: [PATCH] Verify if supplied AgentX PDU header flags are valid for given PDU type inside appl_agentx_recv(). While here clean up the logging a bit. OK tb@ --- usr.sbin/snmpd/application_agentx.c | 108 ++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/usr.sbin/snmpd/application_agentx.c b/usr.sbin/snmpd/application_agentx.c index 8062307b5ba..3a67f9dc0f1 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.6 2023/10/24 09:00:53 martijn Exp $ */ +/* $OpenBSD: application_agentx.c,v 1.7 2023/10/24 12:57:40 martijn Exp $ */ /* * Copyright (c) 2022 Martijn van Duren * @@ -276,13 +276,16 @@ void appl_agentx_recv(int fd, short event, void *cookie) { struct appl_agentx_connection *conn = cookie; - struct appl_agentx_session *session; + struct appl_agentx_session *session = NULL; struct ax_pdu *pdu; + enum appl_error error; + char name[100]; + snprintf(name, sizeof(name), "AgentX(%"PRIu32")", conn->conn_id); if ((pdu = ax_recv(conn->conn_ax)) == NULL) { if (errno == EAGAIN) return; - log_warn("AgentX(%"PRIu32")", conn->conn_id); + log_warn("%s", name); /* * Either the connection is dead, or we had garbage on the line. * Both make sure we can't continue on this stream. @@ -306,16 +309,12 @@ appl_agentx_recv(int fd, short event, void *cookie) break; } if (session == NULL) { - log_warnx("AgentX(%"PRIu32"): Session %"PRIu32" not " - "found for request", conn->conn_id, - pdu->ap_header.aph_sessionid); - ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid, - pdu->ap_header.aph_transactionid, - pdu->ap_header.aph_packetid, smi_getticks(), - APPL_ERROR_NOTOPEN, 0, NULL, 0); - appl_agentx_send(-1, EV_WRITE, conn); + log_warnx("%s: Session %"PRIu32" not found for request", + name, pdu->ap_header.aph_sessionid); + error = APPL_ERROR_NOTOPEN; goto fail; } + strlcpy(name, session->sess_backend.ab_name, sizeof(name)); /* * RFC2741 section 7.1.1 bullet 4 is unclear on what byte order * the response should be. My best guess is that it makes more @@ -327,6 +326,51 @@ appl_agentx_recv(int fd, short event, void *cookie) */ } + if (pdu->ap_header.aph_flags & AX_PDU_FLAG_INSTANCE_REGISTRATION) { + if (pdu->ap_header.aph_type != AX_PDU_TYPE_REGISTER) { + log_warnx("%s: %s: Invalid INSTANCE_REGISTRATION flag", + name, ax_pdutype2string(pdu->ap_header.aph_flags)); + error = APPL_ERROR_PARSEERROR; + goto fail; + } + } + if (pdu->ap_header.aph_flags & AX_PDU_FLAG_NEW_INDEX) { + if (pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE && + pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE) { + log_warnx("%s: %s: Invalid NEW_INDEX flag", name, + ax_pdutype2string(pdu->ap_header.aph_flags)); + error = APPL_ERROR_PARSEERROR; + goto fail; + } + } + if (pdu->ap_header.aph_flags & AX_PDU_FLAG_ANY_INDEX) { + if (pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE && + pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE) { + log_warnx("%s: %s: Invalid ANY_INDEX flag", name, + ax_pdutype2string(pdu->ap_header.aph_flags)); + error = APPL_ERROR_PARSEERROR; + goto fail; + } + } + if (pdu->ap_header.aph_flags & AX_PDU_FLAG_NON_DEFAULT_CONTEXT) { + if (pdu->ap_header.aph_type != AX_PDU_TYPE_REGISTER && + pdu->ap_header.aph_type != AX_PDU_TYPE_UNREGISTER && + pdu->ap_header.aph_type != AX_PDU_TYPE_ADDAGENTCAPS && + pdu->ap_header.aph_type != AX_PDU_TYPE_REMOVEAGENTCAPS && + pdu->ap_header.aph_type != AX_PDU_TYPE_GET && + pdu->ap_header.aph_type != AX_PDU_TYPE_GETNEXT && + pdu->ap_header.aph_type != AX_PDU_TYPE_GETBULK && + pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE && + pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE && + pdu->ap_header.aph_type != AX_PDU_TYPE_NOTIFY && + pdu->ap_header.aph_type != AX_PDU_TYPE_TESTSET && + pdu->ap_header.aph_type != AX_PDU_TYPE_PING) { + log_warnx("%s: %s: Invalid NON_DEFAULT_CONTEXT flag", + name, ax_pdutype2string(pdu->ap_header.aph_flags)); + error = APPL_ERROR_PARSEERROR; + goto fail; + } + } switch (pdu->ap_header.aph_type) { case AX_PDU_TYPE_OPEN: appl_agentx_open(conn, pdu); @@ -347,16 +391,12 @@ appl_agentx_recv(int fd, short event, void *cookie) case AX_PDU_TYPE_COMMITSET: case AX_PDU_TYPE_UNDOSET: case AX_PDU_TYPE_CLEANUPSET: - appl_agentx_forceclose(&(session->sess_backend), - APPL_CLOSE_REASONPROTOCOLERROR); - if (!TAILQ_EMPTY(&(conn->conn_sessions))) - goto fail; - - ax_pdu_free(pdu); - appl_agentx_free(conn); - return; + log_warnx("%s: %s: Not an adminsitrative message", name, + ax_pdutype2string(pdu->ap_header.aph_type)); + error = APPL_ERROR_PARSEERROR; + goto fail; case AX_PDU_TYPE_NOTIFY: - log_warnx("%s: not supported", + log_warnx("%s: %s: not supported", name, ax_pdutype2string(pdu->ap_header.aph_type)); /* * RFC 2741 section 7.1.10: @@ -375,7 +415,7 @@ appl_agentx_recv(int fd, short event, void *cookie) break; case AX_PDU_TYPE_INDEXALLOCATE: case AX_PDU_TYPE_INDEXDEALLOCATE: - log_warnx("%s: not supported", + log_warnx("%s: %s: not supported", name, ax_pdutype2string(pdu->ap_header.aph_type)); ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid, pdu->ap_header.aph_transactionid, @@ -387,22 +427,32 @@ appl_agentx_recv(int fd, short event, void *cookie) break; case AX_PDU_TYPE_ADDAGENTCAPS: case AX_PDU_TYPE_REMOVEAGENTCAPS: - log_warnx("%s: not supported", + log_warnx("%s: %s: not supported", name, ax_pdutype2string(pdu->ap_header.aph_type)); - ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid, - pdu->ap_header.aph_transactionid, - pdu->ap_header.aph_packetid, smi_getticks(), - APPL_ERROR_PROCESSINGERROR, 1, - NULL, 0); - appl_agentx_send(-1, EV_WRITE, conn); - break; + error = APPL_ERROR_PROCESSINGERROR; + goto fail; case AX_PDU_TYPE_RESPONSE: appl_agentx_response(session, pdu); break; } + ax_pdu_free(pdu); + return; fail: + 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); ax_pdu_free(pdu); + + if (session == NULL || error != APPL_ERROR_PARSEERROR) + return; + + appl_agentx_forceclose(&(session->sess_backend), + APPL_CLOSE_REASONPARSEERROR); + if (TAILQ_EMPTY(&(conn->conn_sessions))) + appl_agentx_free(conn); } void -- 2.20.1