Re: snmpd: Add end of sequence tests
On Fri, 2021-02-12 at 11:02 +0100, Claudio Jeker wrote: > On Fri, Feb 12, 2021 at 10:03:21AM +0100, Martijn van Duren wrote: > > ping > > > > On Sun, 2021-01-31 at 11:57 +0100, Martijn van Duren wrote: > > > Now that ober_scanf_elements supports '$' lets use it. > > > > > > Here's a first stab by adding it to snmpd. > > > Passing regress and a few manual checks. > > > > > > 'e' still doesn't consume the element, but I've talked it over with > > > rob@, who said that shouldn't get in the way of using this new feature. > > > > > > OK? > > Looks reasonable and I guess you verified the layout of all those ASN.1 > messages to ensure the $ is at the right place. To the best of my knowledge in both reading and testing. If I somehow did screw it up, it should be easy enough to fix and loud enough to notice. > > Side note: I wonder why does } not imply the $? At least now with S it > would be possible to enforce this. } does nothing more then dropping down from the current sequence level. This comes in handy if for instance when parsing a PDU. The varbindlist is of undefined length, so I wouldn't know how many Ss I'd need. See line snmpe.c:380. The parsing and verifying of the varbindlist happens on snmpe.c:439. > I like that you closed a lot of open { format strings. What about these: > > > > - if (ober_scanf_elements(usm, "{xiixpxx", &engineid, &enginelen, > > > + if (ober_scanf_elements(usm, "{xiixpxx$", &engineid, &enginelen, > > Wouldn't it be better to use "{xiixpxx$}" here? > I treat } as dropping down from the current sequence level. In that case it doesn't matter. It might be more estatically pleasing. I can add it if you want, but doesn't add anything. Updated diff, since I just now noticed how mangled that thing was. Index: snmpe.c === RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v retrieving revision 1.69 diff -u -p -r1.69 snmpe.c --- snmpe.c 5 Feb 2021 10:30:45 - 1.69 +++ snmpe.c 12 Feb 2021 10:34:32 - @@ -227,7 +227,7 @@ snmpe_parse(struct snmp_message *msg) case SNMP_V2: if (env->sc_min_seclevel != 0) goto badversion; - if (ober_scanf_elements(a, "se", &comn, &msg->sm_pdu) != 0) + if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != 0) goto parsefail; if (strlcpy(msg->sm_community, comn, sizeof(msg->sm_community)) >= sizeof(msg->sm_community)) { @@ -237,7 +237,7 @@ snmpe_parse(struct snmp_message *msg) } break; case SNMP_V3: - if (ober_scanf_elements(a, "{iisi}e", + if (ober_scanf_elements(a, "{iisi$}e", &msg->sm_msgid, &msg->sm_max_msg_size, &flagstr, &msg->sm_secmodel, &a) != 0) goto parsefail; @@ -255,7 +255,7 @@ snmpe_parse(struct snmp_message *msg) goto parsefail; } - if (ober_scanf_elements(a, "{xxe", + if (ober_scanf_elements(a, "{xxeS$}$", &msg->sm_ctxengineid, &msg->sm_ctxengineid_len, &ctxname, &len, &msg->sm_pdu) != 0) goto parsefail; @@ -377,7 +377,7 @@ snmpe_parse(struct snmp_message *msg) } /* SNMP PDU */ - if (ober_scanf_elements(a, "iiie{et", + if (ober_scanf_elements(a, "iiie{et}$", &req, &errval, &erridx, &msg->sm_pduend, &msg->sm_varbind, &class, &type) != 0) { stats->snmp_silentdrops++; @@ -436,7 +436,7 @@ snmpe_parsevarbinds(struct snmp_message for (i = 1; varbind != NULL && i < SNMPD_MAXVARBIND; varbind = varbind->be_next, i++) { - if (ober_scanf_elements(varbind, "{oe}", &o, &value) == -1) { + if (ober_scanf_elements(varbind, "{oeS$}", &o, &value) == -1) { stats->snmp_inasnparseerrs++; msg->sm_errstr = "invalid varbind"; goto varfail; Index: traphandler.c === RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v retrieving revision 1.20 diff -u -p -r1.20 traphandler.c --- traphandler.c 22 Jan 2021 06:33:27 - 1.20 +++ traphandler.c 12 Feb 2021 10:34:32 - @@ -67,7 +67,7 @@ traphandler_parse(struct snmp_message *m struct privsep *ps = &snmpd_env->sc_ps; struct snmp_stats *stats = &snmpd_env->sc_stats; struct ber ber = {0}; - struct ber_element *vblist = NULL, *elm, *elm2; + struct ber_element *vblist = NULL, *elm; struct ber_oid o1, o2, snmpTrapOIDOID; struct ber_oid snmpTrapOID, sysUpTimeOID; int sysUpTime; @@ -82,7 +82,7 @@ traphandler_parse(struct snmp_message
Re: snmpd: Add end of sequence tests
On Fri, Feb 12, 2021 at 10:03:21AM +0100, Martijn van Duren wrote: > ping > > On Sun, 2021-01-31 at 11:57 +0100, Martijn van Duren wrote: > > Now that ober_scanf_elements supports '$' lets use it. > > > > Here's a first stab by adding it to snmpd. > > Passing regress and a few manual checks. > > > > 'e' still doesn't consume the element, but I've talked it over with > > rob@, who said that shouldn't get in the way of using this new feature. > > > > OK? Looks reasonable and I guess you verified the layout of all those ASN.1 messages to ensure the $ is at the right place. Side note: I wonder why does } not imply the $? At least now with S it would be possible to enforce this. I like that you closed a lot of open { format strings. What about these: > > - if (ober_scanf_elements(usm, "{xiixpxx", &engineid, &enginelen, > > + if (ober_scanf_elements(usm, "{xiixpxx$", &engineid, &enginelen, Wouldn't it be better to use "{xiixpxx$}" here? > > Index: snmpe.c > > === > > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v > > retrieving revision 1.68 > > diff -u -p -r1.68 snmpe.c > > --- snmpe.c 22 Jan 2021 06:33:27 - 1.68 > > +++ snmpe.c 31 Jan 2021 10:55:49 - > > @@ -220,7 +220,7 @@ snmpe_parse(struct snmp_message *msg) > > case SNMP_V2: > > if (env->sc_min_seclevel != 0) > > goto badversion; > > - if (ober_scanf_elements(a, "se", &comn, &msg->sm_pdu) != 0) > > + if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != > > 0) > > goto parsefail; > > if (strlcpy(msg->sm_community, comn, > > sizeof(msg->sm_community)) >= > > sizeof(msg->sm_community)) { > > @@ -230,7 +230,7 @@ snmpe_parse(st? tm_udp.c > > Index: snmpe.c > > === > > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v > > retrieving revision 1.68 > > diff -u -p -r1.68 snmpe.c > > --- snmpe.c 22 Jan 2021 06:33:27 - 1.68 > > +++ snmpe.c 31 Jan 2021 10:55:49 - > > @@ -220,7 +220,7 @@ snmpe_parse(struct snmp_message *msg) > > case SNMP_V2: > > if (env->sc_min_seclevel != 0) > > goto badversion; > > - if (ober_scanf_elements(a, "se", &comn, &msg->sm_pdu) != 0) > > + if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != > > 0) > > goto parsefail; > > if (strlcpy(msg->sm_community, comn, > > sizeof(msg->sm_community)) >= > > sizeof(msg->sm_community)) { > > @@ -230,7 +230,7 @@ snmpe_parse(struct snmp_message *msg) > > } > > break; > > case SNMP_V3: > > - if (ober_scanf_elements(a, "{iisi}e", > > + if (ober_scanf_elements(a, "{iisi$}e", > > &msg->sm_msgid, &msg->sm_max_msg_size, &flagstr, > > &msg->sm_secmodel, &a) != 0) > > goto parsefail; > > @@ -248,7 +248,7 @@ snmpe_parse(struct snmp_message *msg) > > goto parsefail; > > } > > > > - if (ober_scanf_elements(a, "{xxe", > > + if (ober_scanf_elements(a, "{xxeS$}$", > > &msg->sm_ctxengineid, &msg->sm_ctxengineid_len, > > &ctxname, &len, &msg->sm_pdu) != 0) > > goto parsefail; > > @@ -370,7 +370,7 @@ snmpe_parse(struct snmp_message *msg) > > } > > > > /* SNMP PDU */ > > - if (ober_scanf_elements(a, "iiie{et", > > + if (ober_scanf_elements(a, "iiie{etS$}$", > > &req, &errval, &erridx, &msg->sm_pduend, > > &msg->sm_varbind, &class, &type) != 0) { > > stats->snmp_silentdrops++; > > @@ -429,7 +429,7 @@ snmpe_parsevarbinds(struct snmp_message > > > > for (i = 1; varbind != NULL && i < SNMPD_MAXVARBIND; > > varbind = varbind->be_next, i++) { > > - if (ober_scanf_elements(varbind, "{oe}", &o, &value) == -1) > > { > > + if (ober_scanf_elements(varbind, "{oeS$}", &o, &value) == > > -1) { > > stats->snmp_inasnparseerrs++; > > msg->sm_errstr = "invalid varbind"; > > goto varfail; > > Index: traphandler.c > > === > > RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v > > retrieving revision 1.20 > > diff -u -p -r1.20 traphandler.c > > --- traphandler.c 22 Jan 2021 06:33:27 - 1.20 > > +++ traphandler.c 31 Jan 2021 10:55:49 - > > @@ -67,7 +67,7 @@ traphandler_parse(struct snmp_message *m > > struct privsep *ps = &snmpd_env->sc_ps; > > struct snmp_stats *stats = &snmpd_env->sc_stats; > > struct ber
Re: snmpd: Add end of sequence tests
ping On Sun, 2021-01-31 at 11:57 +0100, Martijn van Duren wrote: > Now that ober_scanf_elements supports '$' lets use it. > > Here's a first stab by adding it to snmpd. > Passing regress and a few manual checks. > > 'e' still doesn't consume the element, but I've talked it over with > rob@, who said that shouldn't get in the way of using this new feature. > > OK? > > martijn@ > > Index: snmpe.c > === > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v > retrieving revision 1.68 > diff -u -p -r1.68 snmpe.c > --- snmpe.c 22 Jan 2021 06:33:27 - 1.68 > +++ snmpe.c 31 Jan 2021 10:55:49 - > @@ -220,7 +220,7 @@ snmpe_parse(struct snmp_message *msg) > case SNMP_V2: > if (env->sc_min_seclevel != 0) > goto badversion; > - if (ober_scanf_elements(a, "se", &comn, &msg->sm_pdu) != 0) > + if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != 0) > goto parsefail; > if (strlcpy(msg->sm_community, comn, > sizeof(msg->sm_community)) >= sizeof(msg->sm_community)) { > @@ -230,7 +230,7 @@ snmpe_parse(st? tm_udp.c > Index: snmpe.c > === > RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v > retrieving revision 1.68 > diff -u -p -r1.68 snmpe.c > --- snmpe.c 22 Jan 2021 06:33:27 - 1.68 > +++ snmpe.c 31 Jan 2021 10:55:49 - > @@ -220,7 +220,7 @@ snmpe_parse(struct snmp_message *msg) > case SNMP_V2: > if (env->sc_min_seclevel != 0) > goto badversion; > - if (ober_scanf_elements(a, "se", &comn, &msg->sm_pdu) != 0) > + if (ober_scanf_elements(a, "seS$", &comn, &msg->sm_pdu) != 0) > goto parsefail; > if (strlcpy(msg->sm_community, comn, > sizeof(msg->sm_community)) >= sizeof(msg->sm_community)) { > @@ -230,7 +230,7 @@ snmpe_parse(struct snmp_message *msg) > } > break; > case SNMP_V3: > - if (ober_scanf_elements(a, "{iisi}e", > + if (ober_scanf_elements(a, "{iisi$}e", > &msg->sm_msgid, &msg->sm_max_msg_size, &flagstr, > &msg->sm_secmodel, &a) != 0) > goto parsefail; > @@ -248,7 +248,7 @@ snmpe_parse(struct snmp_message *msg) > goto parsefail; > } > > - if (ober_scanf_elements(a, "{xxe", > + if (ober_scanf_elements(a, "{xxeS$}$", > &msg->sm_ctxengineid, &msg->sm_ctxengineid_len, > &ctxname, &len, &msg->sm_pdu) != 0) > goto parsefail; > @@ -370,7 +370,7 @@ snmpe_parse(struct snmp_message *msg) > } > > /* SNMP PDU */ > - if (ober_scanf_elements(a, "iiie{et", > + if (ober_scanf_elements(a, "iiie{etS$}$", > &req, &errval, &erridx, &msg->sm_pduend, > &msg->sm_varbind, &class, &type) != 0) { > stats->snmp_silentdrops++; > @@ -429,7 +429,7 @@ snmpe_parsevarbinds(struct snmp_message > > for (i = 1; varbind != NULL && i < SNMPD_MAXVARBIND; > varbind = varbind->be_next, i++) { > - if (ober_scanf_elements(varbind, "{oe}", &o, &value) == -1) { > + if (ober_scanf_elements(varbind, "{oeS$}", &o, &value) == -1) > { > stats->snmp_inasnparseerrs++; > msg->sm_errstr = "invalid varbind"; > goto varfail; > Index: traphandler.c > === > RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v > retrieving revision 1.20 > diff -u -p -r1.20 traphandler.c > --- traphandler.c 22 Jan 2021 06:33:27 - 1.20 > +++ traphandler.c 31 Jan 2021 10:55:49 - > @@ -67,7 +67,7 @@ traphandler_parse(struct snmp_message *m > struct privsep *ps = &snmpd_env->sc_ps; > struct snmp_stats *stats = &snmpd_env->sc_stats; > struct ber ber = {0}; > - struct ber_element *vblist = NULL, *elm, *elm2; > + struct ber_element *vblist = NULL, *elm; > struct ber_oid o1, o2, snmpTrapOIDOID; > struct ber_oid snmpTrapOID, sysUpTimeOID; > int sysUpTime; > @@ -82,7 +82,7 @@ traphandler_parse(struct snmp_message *m > goto done; > break; > case SNMP_C_TRAPV2: > - if (ober_scanf_elements(msg->sm_pdu, "{SSe}", &elm) == -1) { > + if (ober_scanf_elements(msg->sm_pdu, "{SSe}$", &elm) == -1) { > stats->snmp_inasnparseerrs++; > goto done; > } > @@ -98,7 +98,7 @@ traphandler