From aef20fe1a79526d56e1c389ca6ee37599e573b9e Mon Sep 17 00:00:00 2001 From: Vojtech Vilimek Date: Wed, 25 Oct 2023 12:56:23 +0200 Subject: [PATCH] SNMP: Better handling of errors --- proto/snmp/snmp.c | 33 +++++++++++++++++----- proto/snmp/snmp.h | 2 ++ proto/snmp/subagent.c | 66 +++++++++++++++++++++++++++++++++---------- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/proto/snmp/snmp.c b/proto/snmp/snmp.c index 0b123f04..ef041201 100644 --- a/proto/snmp/snmp.c +++ b/proto/snmp/snmp.c @@ -54,6 +54,15 @@ * +-----------------+ * * + * +-----------------+ + * | SNMP_RESET | waiting to transmit response to malformed packet + * +-----------------+ + * | + * | response was send, reseting the session (with socket) + * | + * \--> SNMP_LOCKED + * + * * Erroneous transitions: * SNMP is UP in states SNMP_CONN and also in SNMP_REGISTER because the * session is establised and the GetNext request should be responsed @@ -196,15 +205,15 @@ snmp_reconnect(timer *tm) snmp_connected(p->sock); } -static void -snmp_sock_err(sock *sk, int UNUSED err) +/* this function is internal and shouldn't be used outside the snmp module */ +void +snmp_sock_disconnect(struct snmp_proto *p, int reconnect) { - snmp_log("socket error '%s' (errno: %d)", strerror(err), err); - struct snmp_proto *p = sk->data; - p->errs++; - tm_stop(p->ping_timer); + if (!reconnect) + return snmp_down(p); + proto_notify_state(&p->p, PS_START); rfree(p->sock); p->sock = NULL; @@ -212,10 +221,20 @@ snmp_sock_err(sock *sk, int UNUSED err) snmp_log("changing state to LOCKED"); p->state = SNMP_LOCKED; + /* We try to reconnect after a short delay */ p->startup_timer->hook = snmp_startup_timeout; - tm_start(p->startup_timer, 4 S); // TODO make me configurable + tm_start(p->startup_timer, 4 S); // TODO make me configurable } +static void +snmp_sock_err(sock *sk, int UNUSED err) +{ + snmp_log("socket error '%s' (errno: %d)", strerror(err), err); + struct snmp_proto *p = sk->data; + p->errs++; + + snmp_sock_disconnect(p, 1); +} static void snmp_start_locked(struct object_lock *lock) diff --git a/proto/snmp/snmp.h b/proto/snmp/snmp.h index 180a2030..ebdd9b5f 100644 --- a/proto/snmp/snmp.h +++ b/proto/snmp/snmp.h @@ -37,6 +37,7 @@ enum snmp_proto_state { SNMP_CONN, SNMP_STOP, SNMP_DOWN, + SNMP_RESET, }; /* hash table macros */ @@ -169,6 +170,7 @@ void snmp_startup(struct snmp_proto *p); void snmp_connected(sock *sk); void snmp_startup_timeout(timer *tm); void snmp_reconnect(timer *tm); +void snmp_sock_disconnect(struct snmp_proto *p, int reconnect); #endif diff --git a/proto/snmp/subagent.c b/proto/snmp/subagent.c index f65b9047..3404c568 100644 --- a/proto/snmp/subagent.c +++ b/proto/snmp/subagent.c @@ -35,6 +35,7 @@ static struct agentx_response *prepare_response(struct snmp_proto *p, struct snm static void response_err_ind(struct agentx_response *res, uint err, uint ind); static uint update_packet_size(struct snmp_proto *p, const byte *start, byte *end); static struct oid *search_mib(struct snmp_proto *p, const struct oid *o_start, const struct oid *o_end, struct oid *o_curr, struct snmp_pdu *c, enum snmp_search_res *result); +static void snmp_tx(sock *sk); u32 snmp_internet[] = { SNMP_ISO, SNMP_ORG, SNMP_DOD, SNMP_INTERNET }; @@ -76,6 +77,22 @@ static const char * const snmp_pkt_type[] UNUSED = { [AGENTX_RESPONSE_PDU] = "Response-PDU", }; +static int +snmp_rx_skip(sock UNUSED *sk, uint UNUSED size) +{ + return 1; +} + +static inline void +snmp_error(struct snmp_proto *p) +{ + snmp_log("changing state to RESET"); + p->state = SNMP_RESET; + + p->sock->rx_hook = snmp_rx_skip; + p->sock->tx_hook = snmp_tx; +} + static void snmp_simple_response(struct snmp_proto *p, enum agentx_response_err error, u16 index) { @@ -313,27 +330,28 @@ close_pdu(struct snmp_proto *p, u8 reason) #undef REASON_SIZE } -static uint UNUSED -parse_close_pdu(struct snmp_proto UNUSED *p, byte UNUSED *req, uint UNUSED size) +static uint +parse_close_pdu(struct snmp_proto *p, byte * const pkt_start, uint size) { -#if 0 - //byte *pkt = req; - //sock *sk = p->sock; + byte *pkt = pkt_start; + struct agentx_header *h = (void *) pkt; + ADVANCE(pkt, size, AGENTX_HEADER_SIZE); + int byte_ord = h->flags & AGENTX_NETWORK_BYTE_ORDER; + uint pkt_size = LOAD_U32(h->payload, byte_ord); - if (size < sizeof(struct agentx_header)) + if (pkt_size != 0) { - return 0; + snmp_simple_response(p, AGENTX_RES_GEN_ERROR, 0); + // TODO: best solution for possibly malicious pkt_size + //return AGENTX_HEADER_SIZE + MIN(size, pkt_size); + return AGENTX_HEADER_SIZE; } - //struct agentx_header *h = (void *) req; - ADVANCE(req, size, AGENTX_HEADER_SIZE); + /* The agentx-Close-PDU must not have non-default context */ - p->state = SNMP_ERR; - - /* or snmp_cleanup(); // ??! */ - proto_notify_state(&p->p, PS_DOWN); -#endif - return 0; + snmp_simple_response(p, AGENTX_RES_NO_ERROR, 0); + snmp_sock_disconnect(p, 1); // TODO: should we try to reconnect (2nd arg) ?? + return AGENTX_HEADER_SIZE; } /* MUCH better signature would be @@ -541,7 +559,10 @@ error: s = update_packet_size(p, sk->tpos, c.buffer); if (c.error != AGENTX_RES_NO_ERROR) + { response_err_ind(res, c.error, c.index + 1); + snmp_error(p); + } else if (all_possible) response_err_ind(res, AGENTX_RES_NO_ERROR, 0); else @@ -604,6 +625,11 @@ parse_set_pdu(struct snmp_proto *p, byte * const pkt_start, uint size, uint err) error:; response_err_ind(r, c.error, 0); sk_send(p->sock, AGENTX_HEADER_SIZE); + + /* Reset the connection on unrecoverable error */ + if (c.error != AGENTX_RES_NO_ERROR && c.error != err) + snmp_error(p); + return pkt - pkt_start; } @@ -1374,6 +1400,16 @@ snmp_rx(sock *sk, uint size) return 1; } +/* snmp_tx - used to reset the connection when the + * agentx-Response-PDU was sent + */ +static void +snmp_tx(sock *sk) +{ + struct snmp_proto *p = sk->data; + snmp_sock_disconnect(p, 1); +} + /* Ping-PDU */ void snmp_ping(struct snmp_proto *p)