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=156570803230850=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 - 1.11
+++ ber.c 13 Aug 2019 15:00:36 -
@@ -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);
