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

[Stefan Zegenhagen]

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.