[erlang-patches] SNMP: verify RowIndex in table set requests

[Stefan Zegenhagen]

Dear all,

I've not heard any comment on the patch below. Anyone interested???

Kind regards,

Stefan.

> Dear all,
> 
> during intensive testing it was found that it's possible to create table
> rows in snmpTargetAddrTable where the row index (snmpTargetAddrName ::
> SnmpAdminString(1..32)) is the empty string. This patch solves the issue
> generically by checking for all set-requests that the row index is of
> acceptable length before calling the table instrumentation function.
> 
> The same is not done for get requests because such invalid row indices
> should not be present if it's impossible to create them.
> 
> The same is not done for get-next requests because the semantic of the
> operation requires it to be able to cope with incomplete/overlong row
> indices.
> 
> Kind regards,
> 
> 
> --- snip ---
> 
> From be617c829a89dd9b73768181810776e50fb36c42 Mon Sep 17 00:00:00 2001
> From: Stefan Zegenhagen <stefan.zegenhagen@REDACTED>
> Date: Wed, 4 Jul 2012 14:34:46 +0200
> Subject: [PATCH 2/2] verify table row index constraints in SNMP set requests
> 
> When a set-request is received for table columns, check that the varbind
> OIDs are syntactically correct. This includes checking that Table Entry
> OID as well as the Column OID are present, and that the row index
> contains an acceptable number of subidentifiers.
> 
> This check was not done carefully enough before, giving the following
> test result from an erlang-snmp agent:
> 
>  Response: Version = SNMPv3, MsgId = 173, MaxSize = 2048
>  Flags = Auth/Priv, SecurityModel = USM
>  EngineId = [...], EngineBoots = 16, EngineTime = 81244, UserName = ...
>  ContextEngineId = [...], ContextName = [...]
>  Request Id = 173, Error Status = No Error , Error Index = 0
>  Oid1 = snmpTargetAddrRowStatus , Type = Integer, Value = 4
>  Oid2 = snmpTargetAddrTAddress , Type = OctetString, Value = ...
>  Oid3 = snmpTargetAddrTimeout , Type = Integer, Value = 100
>  Oid4 = snmpTargetAddrRetryCount , Type = Integer, Value = 3
>  Oid5 = snmpTargetAddrTagList , Type = OctetString, Value = abc
>  Oid6 = snmpTargetAddrParams , Type = OctetString, Value = abc
>  Oid7 = snmpTargetAddrStorageType , Type = Integer, Value = 2
>  Oid8 = snmpTargetAddrTDomain , Type = ObjectID, Value = 1.3.6.1.6.1.1
> 
> Please observe that a table row in snmptargetAddrParams is created
> without errors, although an empty RowIndex was specified for all
> columnar objects in the request. The index to that table is defined as
> IMPLIED SnmpAdminString(1..32).
> ---
>  lib/snmp/src/agent/snmpa_set_lib.erl |   65 ++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/snmp/src/agent/snmpa_set_lib.erl b/lib/snmp/src/agent/snmpa_set_lib.erl
> index f5218d5..ef9e20e 100644
> --- a/lib/snmp/src/agent/snmpa_set_lib.erl
> +++ b/lib/snmp/src/agent/snmpa_set_lib.erl
> @@ -70,8 +70,19 @@
>  %% Purpose: Call is_varbind_ok for each varbind
>  %% Returns: {noError, 0} | {ErrorStatus, ErrorIndex}
>  %%-----------------------------------------------------------------
> -is_varbinds_ok([{_TableOid, TableVbs} | IVarbinds]) -> 
> -    is_varbinds_ok(lists:append(TableVbs, IVarbinds));
> +is_varbinds_ok([{TableOid, TableVbs} | IVarbinds]) ->
> +    % TableOid points to the table entry, not the table itself.
> +    % for table row index checking, we need the table entry, though.
> +    % Hence, strip the last OID.
> +    NTableOid = lists:reverse(tl(lists:reverse(TableOid))),
> +    case is_table_varbinds_ok(NTableOid, TableVbs) of
> +        true ->
> +            is_varbinds_ok(lists:append(TableVbs, IVarbinds));
> +        {ErrorStatus, Varbind} ->
> +            ?vtrace("varbinds erroneous: ~p -> ~p",
> +                [Varbind#varbind.org_index,ErrorStatus]),
> +            {ErrorStatus, Varbind#varbind.org_index}
> +    end;
>  is_varbinds_ok([IVarbind | IVarbinds]) -> 
>      case catch is_varbind_ok(IVarbind) of
>  	true -> is_varbinds_ok(IVarbinds);
> @@ -85,6 +96,56 @@ is_varbinds_ok([]) ->
>      ?vtrace("varbinds ok",[]),
>      {noError, 0}.
>  
> +
> +%%-----------------------------------------------------------------
> +%% Func: is_table_varbinds_ok/2
> +%% Purpose: Perform basic checks on the RowIndex of varbinds against
> +%%          the MIB. Here we don't call any instrumentation functions.
> +%% Returns: true |
> +%% Fails:   with an <error-atom>.
> +%%-----------------------------------------------------------------
> +is_table_varbinds_ok(_TableOid, []) ->
> +    true;
> +is_table_varbinds_ok(TableOid, [Hd | _] = Varbinds) ->
> +    ErrResult = {noCreation, Hd#ivarbind.varbind},
> +    % resolve table information, it's required for the checks we do
> +    case snmpa_symbolic_store:oid_to_aliasname(TableOid) of
> +        {value, TableName} ->
> +            case snmpa_symbolic_store:table_info(TableName) of
> +                {value, #table_info{} = TableInfo} ->
> +                    is_table_varbinds_ok(TableOid, TableInfo, Varbinds);
> +                false ->
> +                    snmpa_error:user_err("Failed in table_info(~w)", [TableName]),
> +                    ErrResult
> +            end;
> +        false ->
> +            snmpa_error:user_err("Failed in oid_to_aliasname(~w)", [TableOid]),
> +            ErrResult
> +    end.
> +
> +is_table_varbinds_ok(TableOid, TableInfo, [#ivarbind{varbind = Varbind} | Varbinds]) ->
> +    Indices = TableInfo#table_info.index_types,
> +    Oid = Varbind#varbind.oid,
> +    case snmp_misc:diff(Oid, TableOid) of
> +        [_TblEntryId, _TblColumnId | RowIndex] ->
> +            case (catch snmp_generic:split_index_to_keys(Indices, RowIndex)) of
> +                Keys when is_list(Keys) ->
> +                    % RowIndex seems to be OK
> +                    is_table_varbinds_ok(TableOid, TableInfo, Varbinds);
> +                Error ->
> +                    % RowIndex is syntactically wrong...
> +                    ?vtrace("table varbind RowIndex erroneous: ~p", [Error]),
> +                    {noCreation, Varbind}
> +            end;
> +        _Else ->
> +            % OID is syntactically wrong...
> +            ?vtrace("table varbind OID erroneous: ~p", [Oid]),
> +            {noCreation, Varbind}
> +    end;
> +is_table_varbinds_ok(_TableOid, _TableInfo, []) ->
> +    true.
> +
> +
>  %%-----------------------------------------------------------------
>  %% Func: is_varbind_ok/1
>  %% Purpose: Check everything we can check about the varbind against
> -- 
> 1.7.9.5
> 
> 

-- 
Dr. Stefan Zegenhagen

arcutronix GmbH
Garbsener Landstr. 10
30419 Hannover
Germany

Tel:   +49 511 277-2734
Fax:   +49 511 277-2709
Email: stefan.zegenhagen@REDACTED
Web:   www.arcutronix.com

*Synchronize the Ethernet*

General Managers: Dipl. Ing. Juergen Schroeder, Dr. Josef Gfrerer -
Legal Form: GmbH, Registered office: Hannover, HRB 202442, Amtsgericht
Hannover; Ust-Id: DE257551767.

Please consider the environment before printing this message.