snmp(1): Better error reporting on malformed packets

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

snmp(1): Better error reporting on malformed packets

Martijn van Duren-5
Right now if we receive a malformed reply (apart from potentially
crashing[0]) we return a rather unsightly and uninformative error
message:
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 snmp getnext -v2c -cpublic 127.0.0.1 ifInDiscards.0      
snmp: getnext: Undefined error: 0

This diff always sets errno to EPROTO if we can't parse the packet.
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 ./obj/snmp getnext -v2c -cpublic 127.0.0.1 ifInDiscards.0
snmp: getnext: Protocol error

(note that I malformed snmpd to return "{o}" for this case).

This diff also retries the packet if a malformed reply is received.
This is similar to what netsnmp does.

OK?

martijn@

[0] https://marc.info/?l=openbsd-tech&m=156570856731073&w=2

Index: snmp.c
===================================================================
RCS file: /cvs/src/usr.bin/snmp/snmp.c,v
retrieving revision 1.1
diff -u -p -r1.1 snmp.c
--- snmp.c 9 Aug 2019 06:17:59 -0000 1.1
+++ snmp.c 13 Aug 2019 17:25:28 -0000
@@ -254,26 +254,51 @@ snmp_resolve(struct snmp_agent *agent, s
  if (ret <= 0)
  goto fail;
  ber_set_readbuf(&ber, buf, ret);
- if ((message = ber_read_elements(&ber, NULL)) == NULL)
- goto fail;
+ if ((message = ber_read_elements(&ber, NULL)) == NULL) {
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
  if (ber_scanf_elements(message, "{ise", &version, &community,
-    &pdu) != 0)
- goto fail;
+    &pdu) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
  /* Skip invalid packets; should not happen */
  if (version != agent->version ||
-    strcmp(community, agent->community) != 0)
+    strcmp(community, agent->community) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
  continue;
+ }
  /* Validate pdu format and check request id */
  if (ber_scanf_elements(pdu, "{iSSe", &rreqid, &varbind) != 0 ||
-    varbind->be_encoding != BER_TYPE_SEQUENCE)
- goto fail;
- if (rreqid != reqid)
+    varbind->be_encoding != BER_TYPE_SEQUENCE) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
  continue;
+ }
+ if (rreqid != reqid) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ continue;
+ }
  for (varbind = varbind->be_sub; varbind != NULL;
     varbind = varbind->be_next) {
- if (ber_scanf_elements(varbind, "{oS}", &oid) != 0)
- goto fail;
+ if (ber_scanf_elements(varbind, "{oS}", &oid) != 0) {
+ errno = EPROTO;
+ direction = POLLOUT;
+ tries--;
+ break;
+ }
  }
+ if (varbind != NULL)
+ continue;
 
  ber_unlink_elements(message->be_sub->be_next);
  ber_free_elements(message);