Re: ber.c: Don't continue on nonexistent ber
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);
Re: ber.c: Don't continue on nonexistent ber
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 13:26:09 -
> @@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b
>
> va_start(ap, fmt);
> while (*fmt) {
> + if (ber == NULL)
> + 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);
>
--
:wq Claudio
ber.c: Don't continue on nonexistent ber
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?
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 13:26:09 -
@@ -684,6 +684,8 @@ ber_scanf_elements(struct ber_element *b
va_start(ap, fmt);
while (*fmt) {
+ if (ber == NULL)
+ 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);
