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", , ,
> > > + if (ober_scanf_elements(usm, "{xiixpxx$", , ,
>
> 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", , >sm_pdu) != 0)
+ if (ober_scanf_elements(a, "seS$", , >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",
>sm_msgid, >sm_max_msg_size, ,
>sm_secmodel, ) != 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$}$",
>sm_ctxengineid, >sm_ctxengineid_len,
, , >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}$",
, , , >sm_pduend,
>sm_varbind, , ) != 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}", , ) == -1) {
+ if (ober_scanf_elements(varbind, "{oeS$}", , ) == -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 = _env->sc_ps;
struct snmp_stats *stats = _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}", ) == -1) {
+
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", , ,
> > + if (ober_scanf_elements(usm, "{xiixpxx$", , ,
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", , >sm_pdu) != 0)
> > + if (ober_scanf_elements(a, "seS$", , >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", , >sm_pdu) != 0)
> > + if (ober_scanf_elements(a, "seS$", , >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",
> > >sm_msgid, >sm_max_msg_size, ,
> > >sm_secmodel, ) != 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$}$",
> > >sm_ctxengineid, >sm_ctxengineid_len,
> > , , >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$}$",
> > , , , >sm_pduend,
> > >sm_varbind, , ) != 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}", , ) == -1)
> > {
> > + if (ober_scanf_elements(varbind, "{oeS$}", , ) ==
> > -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 = _env->sc_ps;
> > struct snmp_stats *stats = _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,
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", , >sm_pdu) != 0)
> + if (ober_scanf_elements(a, "seS$", , >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", , >sm_pdu) != 0)
> + if (ober_scanf_elements(a, "seS$", , >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",
> >sm_msgid, >sm_max_msg_size, ,
> >sm_secmodel, ) != 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$}$",
> >sm_ctxengineid, >sm_ctxengineid_len,
> , , >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$}$",
> , , , >sm_pduend,
> >sm_varbind, , ) != 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}", , ) == -1) {
> + if (ober_scanf_elements(varbind, "{oeS$}", , ) == -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 = _env->sc_ps;
> struct snmp_stats *stats = _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}", ) == -1) {
> + if (ober_scanf_elements(msg->sm_pdu, "{SSe}$", ) == -1) {
> stats->snmp_inasnparseerrs++;
> goto done;
> }
> @@ -98,7 +98,7 @@ traphandler_parse(struct snmp_message *m
>
> (void)ober_string2oid("1.3.6.1.2.1.1.3.0", );
> (void)ober_string2oid("1.3.6.1.6.3.1.1.4.1.0", );
> - if
snmpd: Add end of sequence tests
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", , >sm_pdu) != 0)
+ if (ober_scanf_elements(a, "seS$", , >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", , >sm_pdu) != 0)
+ if (ober_scanf_elements(a, "seS$", , >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",
>sm_msgid, >sm_max_msg_size, ,
>sm_secmodel, ) != 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$}$",
>sm_ctxengineid, >sm_ctxengineid_len,
, , >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$}$",
, , , >sm_pduend,
>sm_varbind, , ) != 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}", , ) == -1) {
+ if (ober_scanf_elements(varbind, "{oeS$}", , ) == -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 = _env->sc_ps;
struct snmp_stats *stats = _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}", ) == -1) {
+ if (ober_scanf_elements(msg->sm_pdu, "{SSe}$", ) == -1) {
stats->snmp_inasnparseerrs++;
goto done;
}
@@ -98,7 +98,7 @@ traphandler_parse(struct snmp_message *m
(void)ober_string2oid("1.3.6.1.2.1.1.3.0", );
(void)ober_string2oid("1.3.6.1.6.3.1.1.4.1.0", );
- if (ober_scanf_elements(vblist, "{{od}{oo}", , , ,
+ if (ober_scanf_elements(vblist, "{{od$}{oo$}", , , ,
) == -1 ||
ober_oid_cmp(, ) != 0 ||
ober_oid_cmp(, ) != 0) {
@@ -107,8 +107,7 @@ traphandler_parse(struct snmp_message *m
}
