Make usm discovery work with snmpd

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

Make usm discovery work with snmpd

Martijn van Duren-8
Hello,

When trying to make p5-Net-SNMP connect to snmpd with seclevel enc it
fails to do so. This is because NET::SNMP verifies agains
usmStatsUnknownEngineIDs, while we return usmStatsUnsupportedSecLevels.

According to RFC3414 chapter 4 we should return usmStatsUnknownEngineIDs
when: Request message with a securityLevel of noAuthNoPriv, a
msgUserName of zero-length, a msgAuthoritativeEngineID value of zero
length, and the varBindList left empty

The diff below doesn't do the full check (which might be a bit
excessive) but does do the usm_decode before the securelevel, so we
trigger the OIDVAL_usmErrEngineId first.

Found via check_snmp_load.pl.
Note that this doesn't make check_snmp_load work yet, it still errors
on the digest check, but gets us at least one step closer to a working
situation with securelevel enc.

OK?

martijn@

Index: snmpe.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.57
diff -u -p -r1.57 snmpe.c
--- snmpe.c 29 Apr 2019 16:04:05 -0000 1.57
+++ snmpe.c 7 May 2019 12:51:21 -0000
@@ -254,6 +254,9 @@ snmpe_parse(struct snmp_message *msg)
  goto parsefail;
 
  msg->sm_flags = *flagstr;
+ if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
+ goto parsefail;
+
  if (MSG_SECLEVEL(msg) < env->sc_min_seclevel ||
     msg->sm_secmodel != SNMP_SEC_USM) {
  /* XXX currently only USM supported */
@@ -262,9 +265,6 @@ snmpe_parse(struct snmp_message *msg)
  msg->sm_usmerr = OIDVAL_usmErrSecLevel;
  goto parsefail;
  }
-
- if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
- goto parsefail;
 
  if (ber_scanf_elements(a, "{xxe",
     &msg->sm_ctxengineid, &msg->sm_ctxengineid_len,

Reply | Threaded
Open this post in threaded view
|

Re: Make usm discovery work with snmpd - part 2: time discovery

Martijn van Duren-8
Hello,

Part 2 of making NET::SNMP work with snmpd(8) in seclevel enc mode.

RFC3414 Chapter 4 states:
If authenticated communication is required, then the discovery
process should also establish time synchronization with the
authoritative SNMP engine.  This may be accomplished by sending an
authenticated Request message with the value of
msgAuthoritativeEngineID set to the newly learned snmpEngineID and
with the values of msgAuthoritativeEngineBoots and
msgAuthoritativeEngineTime set to zero.  For an authenticated Request
message, a valid userName must be used in the msgUserName field.  The
response to this authenticated message will be a Report message
containing the up to date values of the authoritative SNMP engine's
snmpEngineBoots and snmpEngineTime as the value of the
msgAuthoritativeEngineBoots and msgAuthoritativeEngineTime fields
respectively.  It also contains the usmStatsNotInTimeWindows counter
in the varBindList of the Report PDU.  The time synchronization then
happens automatically as part of the procedures in section 3.2 step
7b.  See also section 2.3.

And Chapter 11.4 states:
The use of unsecure reports (i.e., sending them with a securityLevel
of noAuthNoPriv) potentially exposes a non-authoritative SNMP engine
to some form of attacks.  Some people consider these denial of
service attacks, others don't.  An installation should evaluate the
risk involved before deploying unsecure Report PDUs.

Right now we return nothing if engine_boots and engine_time are 0,
so this is incorrect behaviour.

Also, we currently always return reports in plaintext, which is
perfectly fine by the RFC, but NET::SNMP expects the timesync report
to be secure. So I made the diff return the report secure if we
have evaluated the credentials.
With the diff below (on top of the diff send out earlier today) I can
use check_snmp_load.pl (and most likely friends) from the
manubulon-snmp package:

# cat /etc/snmpd.conf
seclevel enc

user test authkey aaaaaaaa auth hmac-sha1 enckey bbbbbbbb enc aes
# snmpd
$ martijn$ ./check_snmp_load.pl -H 127.0.0.1 -l test -x aaaaaaaa -X bbbbbbb  -L sha,aes -w 4 -c 10
ERROR opening session: Received usmStatsUnsupportedSecLevels.0 Report-PDU with value 1 during discovery.
# ./obj/snmpd
martijn$ ./check_snmp_load.pl -H 127.0.0.1 -l test -x aaaaaaaa -X bbbbbbb  -L sha,aes -w 4 -c 10
4 CPU, average load 5.8% > 4% : WARNING

OK?

martijn@

Index: usm.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/usm.c,v
retrieving revision 1.13
diff -u -p -r1.13 usm.c
--- usm.c 12 Aug 2018 22:04:09 -0000 1.13
+++ usm.c 7 May 2019 20:17:25 -0000
@@ -226,6 +226,7 @@ usm_decode(struct snmp_message *msg, str
 
  if (ber_get_nstring(elm, (void *)&usmparams, &len) < 0) {
  *errp = "cannot decode security params";
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -233,6 +234,7 @@ usm_decode(struct snmp_message *msg, str
  usm = ber_read_elements(&ber, NULL);
  if (usm == NULL) {
  *errp = "cannot decode security params";
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -245,6 +247,7 @@ usm_decode(struct snmp_message *msg, str
     &engine_boots, &engine_time, &user, &userlen, &offs2,
     &digest, &digestlen, &salt, &saltlen) != 0) {
  *errp = "cannot decode USM params";
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -257,6 +260,7 @@ usm_decode(struct snmp_message *msg, str
     (digestlen != (MSG_HAS_AUTH(msg) ? SNMP_USM_DIGESTLEN : 0)) ||
     (saltlen != (MSG_HAS_PRIV(msg) ? SNMP_USM_SALTLEN : 0))) {
  *errp = "bad field length";
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -265,21 +269,10 @@ usm_decode(struct snmp_message *msg, str
  *errp = "unknown engine id";
  msg->sm_usmerr = OIDVAL_usmErrEngineId;
  stats->snmp_usmnosuchengine++;
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
- if (engine_boots != 0LL && engine_time != 0LL) {
- now = snmpd_engine_time();
- if (engine_boots != snmpd_env->sc_engine_boots ||
-    engine_time < (long long)(now - SNMP_MAX_TIMEWINDOW) ||
-    engine_time > (long long)(now + SNMP_MAX_TIMEWINDOW)) {
- *errp = "out of time window";
- msg->sm_usmerr = OIDVAL_usmErrTimeWindow;
- stats->snmp_usmtimewindow++;
- goto done;
- }
- }
-
  msg->sm_engine_boots = (u_int32_t)engine_boots;
  msg->sm_engine_time = (u_int32_t)engine_time;
 
@@ -290,12 +283,14 @@ usm_decode(struct snmp_message *msg, str
  *errp = "no such user";
  msg->sm_usmerr = OIDVAL_usmErrUserName;
  stats->snmp_usmnosuchuser++;
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
  if (MSG_SECLEVEL(msg) > msg->sm_user->uu_seclevel) {
  *errp = "unsupported security model";
  msg->sm_usmerr = OIDVAL_usmErrSecLevel;
  stats->snmp_usmbadseclevel++;
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -307,6 +302,7 @@ usm_decode(struct snmp_message *msg, str
  *errp = "bad msg digest";
  msg->sm_usmerr = OIDVAL_usmErrDigest;
  stats->snmp_usmwrongdigest++;
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
 
@@ -316,10 +312,22 @@ usm_decode(struct snmp_message *msg, str
  *errp = "cannot decrypt msg";
  msg->sm_usmerr = OIDVAL_usmErrDecrypt;
  stats->snmp_usmdecrypterr++;
+ msg->sm_flags &= SNMP_MSGFLAG_REPORT;
  goto done;
  }
  ber_replace_elements(elm, decr);
  }
+
+ now = snmpd_engine_time();
+ if (engine_boots != snmpd_env->sc_engine_boots ||
+    engine_time < (long long)(now - SNMP_MAX_TIMEWINDOW) ||
+    engine_time > (long long)(now + SNMP_MAX_TIMEWINDOW)) {
+ *errp = "out of time window";
+ msg->sm_usmerr = OIDVAL_usmErrTimeWindow;
+ stats->snmp_usmtimewindow++;
+ goto done;
+ }
+
  next = elm->be_next;
 
 done:
@@ -477,10 +485,7 @@ usm_make_report(struct snmp_message *msg
 {
  struct ber_oid usmstat = OID(MIB_usmStats, 0, 0);
 
- /* Always send report in clear-text */
- msg->sm_flags = 0;
  msg->sm_context = SNMP_C_REPORT;
- msg->sm_username[0] = '\0';
  usmstat.bo_id[OIDIDX_usmStats] = msg->sm_usmerr;
  usmstat.bo_n = OIDIDX_usmStats + 2;
  if (msg->sm_varbindresp != NULL)

Reply | Threaded
Open this post in threaded view
|

Re: Make usm discovery work with snmpd

Theo Buehler-4
In reply to this post by Martijn van Duren-8
On Tue, May 07, 2019 at 03:02:24PM +0200, Martijn van Duren wrote:

> Hello,
>
> When trying to make p5-Net-SNMP connect to snmpd with seclevel enc it
> fails to do so. This is because NET::SNMP verifies agains
> usmStatsUnknownEngineIDs, while we return usmStatsUnsupportedSecLevels.
>
> According to RFC3414 chapter 4 we should return usmStatsUnknownEngineIDs
> when: Request message with a securityLevel of noAuthNoPriv, a
> msgUserName of zero-length, a msgAuthoritativeEngineID value of zero
> length, and the varBindList left empty
>
> The diff below doesn't do the full check (which might be a bit
> excessive) but does do the usm_decode before the securelevel, so we
> trigger the OIDVAL_usmErrEngineId first.
>
> Found via check_snmp_load.pl.
> Note that this doesn't make check_snmp_load work yet, it still errors
> on the digest check, but gets us at least one step closer to a working
> situation with securelevel enc.
>
> OK?

Not really my area, but this patch is ok tb - fwiw.

>
> martijn@
>
> Index: snmpe.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 snmpe.c
> --- snmpe.c 29 Apr 2019 16:04:05 -0000 1.57
> +++ snmpe.c 7 May 2019 12:51:21 -0000
> @@ -254,6 +254,9 @@ snmpe_parse(struct snmp_message *msg)
>   goto parsefail;
>  
>   msg->sm_flags = *flagstr;
> + if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
> + goto parsefail;
> +
>   if (MSG_SECLEVEL(msg) < env->sc_min_seclevel ||
>      msg->sm_secmodel != SNMP_SEC_USM) {
>   /* XXX currently only USM supported */
> @@ -262,9 +265,6 @@ snmpe_parse(struct snmp_message *msg)
>   msg->sm_usmerr = OIDVAL_usmErrSecLevel;
>   goto parsefail;
>   }
> -
> - if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
> - goto parsefail;
>  
>   if (ber_scanf_elements(a, "{xxe",
>      &msg->sm_ctxengineid, &msg->sm_ctxengineid_len,
>

Reply | Threaded
Open this post in threaded view
|

Re: Make usm discovery work with snmpd

Martijn van Duren-8
I'll probably commit this somewhere today or tomorrow based on tb's OK.
Is there someone else who would like to give their OK?

Also, is there someone who could give their OK on the second patch, so
we can get manubulon-snmp/NET::SNMP to work with seclevel enc?

martijn@

On 5/10/19 11:08 AM, Theo Buehler wrote:

> On Tue, May 07, 2019 at 03:02:24PM +0200, Martijn van Duren wrote:
>> Hello,
>>
>> When trying to make p5-Net-SNMP connect to snmpd with seclevel enc it
>> fails to do so. This is because NET::SNMP verifies agains
>> usmStatsUnknownEngineIDs, while we return usmStatsUnsupportedSecLevels.
>>
>> According to RFC3414 chapter 4 we should return usmStatsUnknownEngineIDs
>> when: Request message with a securityLevel of noAuthNoPriv, a
>> msgUserName of zero-length, a msgAuthoritativeEngineID value of zero
>> length, and the varBindList left empty
>>
>> The diff below doesn't do the full check (which might be a bit
>> excessive) but does do the usm_decode before the securelevel, so we
>> trigger the OIDVAL_usmErrEngineId first.
>>
>> Found via check_snmp_load.pl.
>> Note that this doesn't make check_snmp_load work yet, it still errors
>> on the digest check, but gets us at least one step closer to a working
>> situation with securelevel enc.
>>
>> OK?
>
> Not really my area, but this patch is ok tb - fwiw.
>
>>
>> martijn@
>>
>> Index: snmpe.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
>> retrieving revision 1.57
>> diff -u -p -r1.57 snmpe.c
>> --- snmpe.c 29 Apr 2019 16:04:05 -0000 1.57
>> +++ snmpe.c 7 May 2019 12:51:21 -0000
>> @@ -254,6 +254,9 @@ snmpe_parse(struct snmp_message *msg)
>>   goto parsefail;
>>  
>>   msg->sm_flags = *flagstr;
>> + if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
>> + goto parsefail;
>> +
>>   if (MSG_SECLEVEL(msg) < env->sc_min_seclevel ||
>>      msg->sm_secmodel != SNMP_SEC_USM) {
>>   /* XXX currently only USM supported */
>> @@ -262,9 +265,6 @@ snmpe_parse(struct snmp_message *msg)
>>   msg->sm_usmerr = OIDVAL_usmErrSecLevel;
>>   goto parsefail;
>>   }
>> -
>> - if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
>> - goto parsefail;
>>  
>>   if (ber_scanf_elements(a, "{xxe",
>>      &msg->sm_ctxengineid, &msg->sm_ctxengineid_len,
>>