I found two issues related to this diff. 1) I posted a fix[0] for this one. 2) We can skip a NULL-ber on ')' and '}' since we replace it with a parent ber.
There's only regress tests for ldapd and snmpd, so those are all I tested. martijn@ [0] https://marc.info/?l=openbsd-tech&m=156570803230850&w=2 On 8/13/19 3:37 PM, Claudio Jeker wrote: > On Tue, Aug 13, 2019 at 03:27:17PM +0200, Martijn van Duren wrote: >> I managed to make snmp(1) crash, when I sent a malformed snmp packet. >> Specifically when I have a varbind with an oid, but no value. >> >> I test for this case via ber_scanf_elements("{oS}", which presumably >> would crap out if my skip doesn't have an element. Unfortunately reality >> is that the be_next is skipped and we try again with the same value. >> >> This can give us extremely weird results if we scan for two consecutive >> elements of the same type (e.g. "ss") where the second element is >> non-existent. This would result in the second element having the data >> of the first element. >> >> Diff below fixes this. >> >> OK? > > If the various regress tests still work with this then OK claudio@ > It for sure makes sense but I wouldn't be surprised if there is code > that expects this weird behaviour. > >> martijn@ >> Index: ber.c =================================================================== RCS file: /cvs/src/lib/libutil/ber.c,v retrieving revision 1.11 diff -u -p -r1.11 ber.c --- ber.c 5 Aug 2019 12:38:14 -0000 1.11 +++ ber.c 13 Aug 2019 15:00:36 -0000 @@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b va_start(ap, fmt); while (*fmt) { + if (ber == NULL && *fmt != '}' && *fmt != ')') + goto fail; switch (*fmt++) { case 'B': ptr = va_arg(ap, void **); @@ -788,8 +790,6 @@ ber_scanf_elements(struct ber_element *b goto fail; } - if (ber->be_next == NULL) - continue; ber = ber->be_next; } va_end(ap);
