Fix NULL dereference in snmpd/snmpctl

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix NULL dereference in snmpd/snmpctl

Jan Klemkow
Hello,

The function ber_free_elements() sets the variable ber to NULL in most
error cases and calls ber_free_elements(ber).  This causes a NULL
dereference in ber_free_elements.  This patch fix that problem.

bye,
Jan

Index: ber.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
retrieving revision 1.31
diff -u -p -r1.31 ber.c
--- ber.c 5 Mar 2016 03:31:36 -0000 1.31
+++ ber.c 9 May 2016 19:20:54 -0000
@@ -847,6 +847,8 @@ ber_getpos(struct ber_element *elm)
 void
 ber_free_elements(struct ber_element *root)
 {
+ if (root == NULL)
+ return;
  if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
     root->be_encoding == BER_TYPE_SET))
  ber_free_elements(root->be_sub);

Reply | Threaded
Open this post in threaded view
|

Re: Fix NULL dereference in snmpd/snmpctl

Jonathan Matthew-4
On Mon, May 09, 2016 at 09:37:08PM +0200, Jan Klemkow wrote:
> Hello,
>
> The function ber_free_elements() sets the variable ber to NULL in most
> error cases and calls ber_free_elements(ber).  This causes a NULL
> dereference in ber_free_elements.  This patch fix that problem.

It took me a little while to realise you were talking about
ber_printf_elements here.  I'm not overly familiar with this code, but my
reading of it is that callers of ber_free_elements are expected to check for
NULL themselves, and that ber_printf_elements shouldn't try to free anything.

As you've noticed, when ber_printf_elements jumps to the fail: label, 'ber'
is mostly NULL, because an allocation failed.  In the cases where it isn't,
'ber' points to a previously allocated element that has already been attached
to the element that was passed in as the first argument, so freeing it will
result in use after free later on.  It has no way of knowing if the element
passed in is attached to some other element, so freeing that is similarly bad.

trivial diff will follow when I've tried it out a bit.

Reply | Threaded
Open this post in threaded view
|

Re: Fix NULL dereference in snmpd/snmpctl

Jan Klemkow
Sorry, you are right.  I messed up the description of my diff.  I just
noticed that snmpctl core dumped me.  I debugged it to that point and
thought it were an easy fix.  I didn't investigate further problems with
that code.

Thanks,
Jan

On Sun, May 22, 2016 at 10:04:43PM +1000, Jonathan Matthew wrote:

> On Mon, May 09, 2016 at 09:37:08PM +0200, Jan Klemkow wrote:
> > Hello,
> >
> > The function ber_free_elements() sets the variable ber to NULL in most
> > error cases and calls ber_free_elements(ber).  This causes a NULL
> > dereference in ber_free_elements.  This patch fix that problem.
>
> It took me a little while to realise you were talking about
> ber_printf_elements here.  I'm not overly familiar with this code, but my
> reading of it is that callers of ber_free_elements are expected to check for
> NULL themselves, and that ber_printf_elements shouldn't try to free anything.
>
> As you've noticed, when ber_printf_elements jumps to the fail: label, 'ber'
> is mostly NULL, because an allocation failed.  In the cases where it isn't,
> 'ber' points to a previously allocated element that has already been attached
> to the element that was passed in as the first argument, so freeing it will
> result in use after free later on.  It has no way of knowing if the element
> passed in is attached to some other element, so freeing that is similarly bad.
>
> trivial diff will follow when I've tried it out a bit.