any takers?

On Fri, 2021-06-04 at 22:11 +0200, Martijn van Duren wrote:
> ping
> 
> On Fri, 2021-05-28 at 08:19 +0200, Martijn van Duren wrote:
> > As the original comment said:
> > /*
> >  * This should probably go into parsevarbinds, but that's for a
> >  * next refactor
> >  */
> > 
> > This moves the pdu parsing of traps into parsevarbinds.
> > Added bonus, we can now see the packets in debugging mode:
> > startup
> > snmpe: listening on udp 127.0.0.1:162
> > ktable_new: new ktable for rtableid 0
> > snmpe_parse: 127.0.0.1:162: SNMPv2 'public' pdutype SNMPv2-Trap request 
> > 1329591927
> > snmpe_parse: 127.0.0.1:162: SNMPv1 'public' pdutype Trap request 0
> > 
> > OK?
> > 
> > martijn@
> > 
> > Index: snmpd.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
> > retrieving revision 1.95
> > diff -u -p -r1.95 snmpd.h
> > --- snmpd.h     20 May 2021 08:53:12 -0000      1.95
> > +++ snmpd.h     28 May 2021 06:17:16 -0000
> > @@ -776,6 +776,7 @@ int  proc_flush_imsg(struct privsep *, e
> >  
> >  /* traphandler.c */
> >  int     traphandler_parse(struct snmp_message *);
> > +int     traphandler_v1translate(struct snmp_message *, int);
> >  int     traphandler_priv_recvmsg(struct privsep_proc *, struct imsg *);
> >  void    trapcmd_free(struct trapcmd *);
> >  int     trapcmd_add(struct trapcmd *);
> > Index: snmpe.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> > retrieving revision 1.71
> > diff -u -p -r1.71 snmpe.c
> > --- snmpe.c     20 May 2021 08:53:12 -0000      1.71
> > +++ snmpe.c     28 May 2021 06:17:16 -0000
> > @@ -368,6 +368,10 @@ snmpe_parse(struct snmp_message *msg)
> >                         msg->sm_errstr = "trapv1 request on !SNMPv1 
> > message";
> >                         goto parsefail;
> >                 }
> > +               /* Internally we work with SNMPv2-Traps */
> > +               if (traphandler_v1translate(msg, 0) == -1)
> > +                       goto fail;
> > +               a = msg->sm_pdu->be_sub;
> >         case SNMP_C_TRAPV2:
> >                 if (msg->sm_pdutype == SNMP_C_TRAPV2 &&
> >                     !(msg->sm_version == SNMP_V2 ||
> > @@ -390,16 +394,9 @@ snmpe_parse(struct snmp_message *msg)
> >                         goto fail;
> >                 }
> >                 stats->snmp_intraps++;
> > -               /*
> > -                * This should probably go into parsevarbinds, but that's 
> > for a
> > -                * next refactor
> > -                */
> > -               if (traphandler_parse(msg) == -1)
> > -                       goto fail;
> > -               /* Shortcircuit */
> > -               return 0;
> > +               break;
> >         default:
> > -               msg->sm_errstr = "invalid context";
> > +               msg->sm_errstr = "invalid pdutype";
> >                 goto parsefail;
> >         }
> >  
> > @@ -450,7 +447,14 @@ snmpe_parsevarbinds(struct snmp_message 
> >         struct ber_element      *pvarbind = NULL, *end;
> >         char                     buf[BUFSIZ];
> >         struct ber_oid           o;
> > -       int                      i;
> > +       int                      i = 1;
> > +
> > +       if (msg->sm_pdutype == SNMP_C_TRAP ||
> > +           msg->sm_pdutype == SNMP_C_TRAPV2) {
> > +               if (traphandler_parse(msg) == -1)
> > +                       goto varfail;
> > +               return 0;
> > +       }
> >  
> >         msg->sm_errstr = "invalid varbind element";
> >  
> > @@ -458,7 +462,7 @@ snmpe_parsevarbinds(struct snmp_message 
> >         msg->sm_varbindresp = NULL;
> >         end = NULL;
> >  
> > -       for (i = 1; varbind != NULL && i < SNMPD_MAXVARBIND;
> > +       for (; varbind != NULL && i < SNMPD_MAXVARBIND;
> >             varbind = varbind->be_next, i++) {
> >                 if (ober_scanf_elements(varbind, "{oeS$}", &o, &value) == 
> > -1) {
> >                         stats->snmp_inasnparseerrs++;
> > @@ -543,7 +547,7 @@ snmpe_parsevarbinds(struct snmp_message 
> >         msg->sm_errorindex = 0;
> >  
> >         return 0;
> > - varfail:
> > +varfail:
> >         log_debug("%s: %s:%hd: %s, error index %d", __func__,
> >             msg->sm_host, msg->sm_port, msg->sm_errstr, i);
> >         if (msg->sm_error == 0)
> > @@ -790,18 +794,18 @@ snmpe_recvmsg(int fd, short sig, void *a
> >  void
> >  snmpe_dispatchmsg(struct snmp_message *msg)
> >  {
> > -       if (msg->sm_pdutype == SNMP_C_TRAP ||
> > -           msg->sm_pdutype == SNMP_C_TRAPV2) {
> > -               snmp_msgfree(msg);
> > -               return;
> > -       }
> >         /* dispatched to subagent */
> >         /* XXX Do proper error handling */
> >         (void) snmpe_parsevarbinds(msg);
> >  
> > -       /* respond directly */
> > -       msg->sm_pdutype = SNMP_C_GETRESP;
> > -       snmpe_response(msg);
> > +       if (msg->sm_pdutype == SNMP_C_TRAP ||
> > +           msg->sm_pdutype == SNMP_C_TRAPV2)
> > +               snmp_msgfree(msg);
> > +       else {
> > +               /* respond directly */
> > +               msg->sm_pdutype = SNMP_C_GETRESP;
> > +               snmpe_response(msg);
> > +       }
> >  }
> >  
> >  void
> > Index: traphandler.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v
> > retrieving revision 1.21
> > diff -u -p -r1.21 traphandler.c
> > --- traphandler.c       22 Feb 2021 11:31:09 -0000      1.21
> > +++ traphandler.c       28 May 2021 06:17:16 -0000
> > @@ -45,8 +45,6 @@
> >  
> >  int     traphandler_priv_recvmsg(struct privsep_proc *, struct imsg *);
> >  int     traphandler_fork_handler(struct privsep_proc *, struct imsg *);
> > -struct ber_element *
> > -        traphandler_v1translate(struct snmp_message *, int);
> >  int     trapcmd_cmp(struct trapcmd *, struct trapcmd *);
> >  void    trapcmd_exec(struct trapcmd *, struct sockaddr *,
> >             struct ber_element *);
> > @@ -76,25 +74,17 @@ traphandler_parse(struct snmp_message *m
> >         ssize_t                  buflen;
> >         int                      ret = -1;
> >  
> > -       switch (msg->sm_pdu->be_type) {
> > -       case SNMP_C_TRAP:
> > -               if ((vblist = traphandler_v1translate(msg, 0)) == NULL)
> > -                       goto done;
> > -               break;
> > -       case SNMP_C_TRAPV2:
> > -               if (ober_scanf_elements(msg->sm_pdu, "{SSe}$", &elm) == -1) 
> > {
> > -                       stats->snmp_inasnparseerrs++;
> > -                       goto done;
> > -               }
> > -               if (elm->be_type != BER_TYPE_INTEGER) {
> > -                       stats->snmp_inasnparseerrs++;
> > -                       goto done;
> > -               }
> > -               vblist = ober_unlink_elements(elm);
> > -               break;
> > -       default:
> > -               fatalx("%s called without proper context", __func__);
> > +       if (ober_scanf_elements(msg->sm_pdu, "{SSe}$", &elm) == -1) {
> > +               stats->snmp_inasnparseerrs++;
> > +               msg->sm_errstr = "invalid pdu";
> > +               goto done;
> > +       }
> > +       if (elm->be_type != BER_TYPE_INTEGER) {
> > +               stats->snmp_inasnparseerrs++;
> > +               msg->sm_errstr = "invalid pdu";
> > +               goto done;
> >         }
> > +       vblist = ober_unlink_elements(elm);
> >  
> >         (void)ober_string2oid("1.3.6.1.2.1.1.3.0", &sysUpTimeOID);
> >         (void)ober_string2oid("1.3.6.1.6.3.1.1.4.1.0", &snmpTrapOIDOID);
> > @@ -103,11 +93,13 @@ traphandler_parse(struct snmp_message *m
> >             ober_oid_cmp(&o1, &sysUpTimeOID) != 0 ||
> >             ober_oid_cmp(&o2, &snmpTrapOIDOID) != 0) {
> >                 stats->snmp_inasnparseerrs++;
> > +               msg->sm_errstr = "invalid pdu";
> >                 goto done;
> >         }
> >         (void)ober_scanf_elements(vblist, "{Se", &elm);
> >         for (elm = elm->be_next; elm != NULL; elm = elm->be_next) {
> >                 if (ober_scanf_elements(elm, "{oS$}", &o1) == -1) {
> > +                       msg->sm_errstr = "invalid pdu";
> >                         stats->snmp_inasnparseerrs++;
> >                         goto done;
> >                 }
> > @@ -140,7 +132,7 @@ done:
> >         return ret;
> >  }
> >  
> > -struct ber_element *
> > +int
> >  traphandler_v1translate(struct snmp_message *msg, int proxy)
> >  {
> >         struct snmp_stats       *stats = &snmpd_env->sc_stats;
> > @@ -158,7 +150,8 @@ traphandler_v1translate(struct snmp_mess
> >             agent_addrlen != 4 ||
> >             vblist->be_type != BER_TYPE_SEQUENCE) {
> >                 stats->snmp_inasnparseerrs++;
> > -               return NULL;
> > +               msg->sm_errstr = "invalid pdu";
> > +               return -1;
> >         }
> >         switch (generic_trap) {
> >         case 0:
> > @@ -184,33 +177,33 @@ traphandler_v1translate(struct snmp_mess
> >                 /* Officially this should be 128, but BER_MAX_OID_LEN is 64 
> > */
> >                 if (trapoid.bo_n + 2 > BER_MAX_OID_LEN) {
> >                         stats->snmp_inasnparseerrs++;
> > -                       return NULL;
> > +                       msg->sm_errstr = "enterprise too large";
> > +                       return -1;
> >                 }
> >                 trapoid.bo_id[trapoid.bo_n++] = 0;
> >                 trapoid.bo_id[trapoid.bo_n++] = specific_trap;
> >                 break;
> >         default:
> >                 stats->snmp_inasnparseerrs++;
> > -               return NULL;
> > +               msg->sm_errstr = "invalid generic-trap";
> > +               return -1;
> >         }
> >  
> >         /* work aronud net-snmp's snmptrap: It adds an EOC element in 
> > vblist */
> >         if (vblist->be_len != 0)
> >                 vb0 = ober_unlink_elements(vblist);
> > +       vblist = ober_unlink_elements(msg->sm_pdu);
> > +       ober_free_elements(vblist);
> >  
> > -       if ((vblist = ober_add_sequence(NULL)) == NULL) {
> > -               msg->sm_errstr = strerror(errno);
> > -               if (vb0 != NULL)
> > -                       ober_free_elements(vb0);
> > -               return NULL;
> > -       }
> > -       if (ober_printf_elements(vblist, "{od}{oO}e", "1.3.6.1.2.1.1.3.0",
> > -           time_stamp, "1.3.6.1.6.3.1.1.4.1.0", &trapoid, vb0) == NULL) {
> > +       if (ober_printf_elements(msg->sm_pdu, "ddd{{od}{oO}e}", 0, 0, 0,
> > +           "1.3.6.1.2.1.1.3.0", time_stamp, "1.3.6.1.6.3.1.1.4.1.0", 
> > &trapoid,
> > +           vb0) == NULL) {
> >                 msg->sm_errstr = strerror(errno);
> >                 if (vb0 != 0)
> >                         ober_free_elements(vb0);
> >                 ober_free_elements(vblist);
> > -               return NULL;
> > +               msg->sm_errstr = strerror(errno);
> > +               return -1;
> >         }
> >  
> >         if (proxy) {
> > @@ -220,11 +213,12 @@ traphandler_v1translate(struct snmp_mess
> >                     &snmpTrapCommunityOid);
> >                 (void)ober_string2oid("1.3.6.1.6.3.1.1.4.3.0",
> >                     &snmpTrapEnterpriseOid);
> > -               for (elm = vblist->be_sub; elm != NULL; elm = elm->be_next) 
> > {
> > +               ober_scanf_elements(msg->sm_pdu, "{SSS{e", &elm);
> > +               for (; elm != NULL; elm = elm->be_next) {
> >                         if (ober_get_oid(elm->be_sub, &oid) == -1) {
> >                                 msg->sm_errstr = "failed to read oid";
> >                                 ober_free_elements(vblist);
> > -                               return NULL;
> > +                               return -1;
> >                         }
> >                         if (ober_oid_cmp(&oid, &snmpTrapAddressOid) == 0)
> >                                 hasaddress = 1;
> > @@ -243,11 +237,11 @@ traphandler_v1translate(struct snmp_mess
> >                             &snmpTrapEnterpriseOid, &enterprise) == NULL) {
> >                                 msg->sm_errstr = strerror(errno);
> >                                 ober_free_elements(vblist);
> > -                               return NULL;
> > +                               return -1;
> >                         }
> >                 }
> >         }
> > -       return vblist;
> > +       return 0;
> >  }
> >  
> >  int
> > 
> 


Reply via email to