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

[Martijn van Duren]

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. :-)

martijn@

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: