Re: snmpd(8): fix use of uninitialized pointer

[Jan Klemkow]

On Tue, Mar 10, 2020 at 06:25:10PM +0100, Martijn van Duren wrote:
> So I did some last minute testing of my own and apparently I misread.
> A varbindlist means a new sequence of varbinds, while the code assumes
> a list of varbinds.
> 
> Code below actually works and also gives the additional varbinds to
> the trap handle "command".
> 
> Now actually asking for OKs. :-)

OK jan@

> On 3/10/20 1:43 PM, Jan Klemkow wrote:
> > On Tue, Mar 10, 2020 at 01:13:46PM +0100, Martijn van Duren wrote:
> >> On 3/10/20 10:21 AM, Jan Klemkow wrote:
> >>> On Tue, Mar 10, 2020 at 12:27:20AM +0100, Martijn van Duren wrote:
> >>>> Looking at RFC1157 section 4.1.6, an snmpv1 trap should also contain a
> >>>> varbindlist.
> >>>>
> >>>> Could you test the diff below?
> >>>
> >>> Is also OK for me and the current call path seems to be clean.
> >>> But, shouldn't we set iter to NULL anyway?
> >>
> >> If we state that traphandler_parse must initialize *vbinds, then not
> >> setting it allows us to differentiate between undefined behaviour
> >> (initialized to garbage) and no varbindlist. The first one will only
> >> show its head if someone screws up a refactor.
> >> But if you feel strongly about it we can initialize it.
> > 
> > I don't feel strong about it.  Commit your change and I fine with it.
> > 
> > OK jan@
> > 
> >>>> On 3/9/20 11:38 PM, Jan Klemkow wrote:
> >>>>> Hi,
> >>>>>
> >>>>> The following diff fixes the use of the uninitialized pointer iter
> >>>>> in trapcmd_exec().
> >>>>>
> >>>>> iter is just initialized in traphandler_parse() if vers is SNMP_V2.  In
> >>>>> all other cases iter stays uninitialized and may dereferenced in
> >>>>> trapcmd_exec().
> >>>>>
> >>>>> OK?
> >>>>>
> >>>>> bye,
> >>>>> Jan
> 
> Index: traphandler.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 traphandler.c
> --- traphandler.c     24 Oct 2019 12:39:27 -0000      1.15
> +++ traphandler.c     10 Mar 2020 17:23:51 -0000
> @@ -232,10 +232,13 @@ traphandler_parse(char *buf, size_t n, s
>  
>       switch (vers) {
>       case SNMP_V1:
> -             if (ober_scanf_elements(elm, "{oSddd",
> -                 trapoid, &gtype, &etype, uptime) == -1)
> +             if (ober_scanf_elements(elm, "{oSddde",
> +                 trapoid, &gtype, &etype, uptime, &elm) == -1)
>                       goto done;
>               traphandler_v1translate(trapoid, gtype, etype);
> +             if (elm->be_type != BER_TYPE_SEQUENCE)
> +                     goto done;
> +             *vbinds = elm->be_sub;
>               break;
>  
>       case SNMP_V2:
>