Shameless ping!
On Tue, Mar 28, 2017 at 12:45:20PM +0200, alf wrote:
> Further investigation shows that p5-Net-SNMP sends a first packet
> unauthentified and with an empty request. Here snmpd(8) violates
> RFC 3414. It should send OIDVAL_usmErrEngineId but instead sends
> OIDVAL_usmErrSecLevel.
> After that is fixed p5-Net-SNMP sends another empty request to
> sync its time however authentificated this time. This is unexpected
> for snmpd(8) so it doesn't handle this correctly.
> Below is a patch to fix this of the works-for-me fashion, needs some
> checking by someone with better understanding of snmp though.
>
>
> Explanations inline, complete diff at end:
>
> We asume that having the REPORT flag set indicates a discovery so
> set OIDVAL_usmErrEngineid and answer back.
>
> diff -ur snmpd/snmpe.c snmpd.modified/snmpe.c
> --- snmpd/snmpe.c Tue Mar 28 12:19:03 2017
> +++ snmpd.modified/snmpe.c Tue Mar 28 09:33:37 2017
> @@ -220,11 +220,18 @@
> msg->sm_flags = *flagstr;
> if (MSG_SECLEVEL(msg) < env->sc_min_seclevel ||
> msg->sm_secmodel != SNMP_SEC_USM) {
> - /* XXX currently only USM supported */
> - msg->sm_errstr = "unsupported security model";
> - stats->snmp_usmbadseclevel++;
> - msg->sm_usmerr = OIDVAL_usmErrSecLevel;
> - goto parsefail;
> + if (MSG_REPORT(msg)) {
> + msg->sm_errstr = "no such engineid";
> + stats->snmp_usmnosuchengine++;
> + msg->sm_usmerr = OIDVAL_usmErrEngineId;
> + goto parsefail;
> + } else {
> + /* XXX currently only USM supported */
> + msg->sm_errstr = "unsupported security model";
> + stats->snmp_usmbadseclevel++;
> + msg->sm_usmerr = OIDVAL_usmErrSecLevel;
> + goto parsefail;
> + }
> }
>
> if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
> @@ -324,11 +331,12 @@
> msg->sm_errstr = "invalid PDU";
> goto fail;
> }
>
> This check gets in the way because p5-Net-SNMP sends after the first
> unauthentificated packet a second one with an empty request to sync its time.
> There should be a better way however, as is it is XXX.
>
> - if (class != BER_CLASS_UNIVERSAL || type != BER_TYPE_SEQUENCE) {
> - stats->snmp_silentdrops++;
> - msg->sm_errstr = "invalid varbind";
> - goto fail;
> - }
> + /* XXX disabled for now */
> + /* if (class != BER_CLASS_UNIVERSAL || type != BER_TYPE_SEQUENCE) { */
> + /* stats->snmp_silentdrops++; */
> + /* msg->sm_errstr = "invalid varbind"; */
> + /* goto fail; */
> + /* } */
>
> msg->sm_request = req;
> msg->sm_error = errval;
>
> Because of the second empty request in an otherwise valid packet first check
> for MSG_REPORT and check if snmpe_parse has failed later.
> @@ -485,7 +493,8 @@
> struct snmp_stats *stats = &env->sc_stats;
> ssize_t len;
> struct snmp_message *msg;
> -
> + int ret;
> +
> if ((msg = calloc(1, sizeof(*msg))) == NULL)
> return;
>
> @@ -517,17 +526,19 @@
> smi_debug_elements(msg->sm_req);
> #endif
>
> - if (snmpe_parse(msg) == -1) {
> - if (msg->sm_usmerr != 0 && MSG_REPORT(msg)) {
> - usm_make_report(msg);
> - snmpe_response(fd, msg);
> - return;
> - } else {
> - snmp_msgfree(msg);
> - return;
> - }
> + ret = snmpe_parse(msg);
> +
> + if (msg->sm_usmerr != 0 && MSG_REPORT(msg)) {
> + usm_make_report(msg);
> + snmpe_response(fd, msg);
> + return;
> }
>
> + if (ret == -1) {
> + snmp_msgfree(msg);
> + return;
> + }
> +
> snmpe_dispatchmsg(msg, fd);
> }
>
> This check must be done differently so OIDVAL_usmErrTimeWindow gets set and
> reported back to the client.
> diff -ur snmpd/usm.c snmpd.modified/usm.c
> --- snmpd/usm.c Tue Mar 28 12:19:03 2017
> +++ snmpd.modified/usm.c Tue Mar 28 09:33:37 2017
> @@ -269,21 +269,20 @@
> 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++;
> + 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++;
> + if (engine_boots != 0LL && engine_time != 0LL)
> goto done;
> - }
> + } else {
> + msg->sm_engine_boots = (u_int32_t)engine_boots;
> + msg->sm_engine_time = (u_int32_t)engine_time;
> }
> -
> - msg->sm_engine_boots = (u_int32_t)engine_boots;
> - msg->sm_engine_time = (u_int32_t)engine_time;
> -
> +
> memcpy(msg->sm_username, user, userlen);
> msg->sm_username[userlen] = '\0';
> msg->sm_user = usm_finduser(msg->sm_username);
>
> Because the second request is already authentificated we must avoid clearing
> sm_flags and sm_username in the answer otherwise p5-Net-SNMP will not accept
> this packet.
>
> @@ -481,9 +480,9 @@
> struct ber_oid usmstat = OID(MIB_usmStats, 0, 0);
>
> /* Always send report in clear-text */
> - msg->sm_flags = 0;
> + /* msg->sm_flags = 0; */
> msg->sm_context = SNMP_C_REPORT;
> - msg->sm_username[0] = '\0';
> + /* msg->sm_username[0] = '\0'; */
> usmstat.bo_id[OIDIDX_usmStats] = msg->sm_usmerr;
> usmstat.bo_n = OIDIDX_usmStats + 2;
> if (msg->sm_varbindresp != NULL)
>
>
> ------------------------------------------------------------------------------
> Complete diff:
>
>
> diff -ur snmpd/snmpe.c snmpd.modified/snmpe.c
> --- snmpd/snmpe.c Tue Mar 28 12:19:03 2017
> +++ snmpd.modified/snmpe.c Tue Mar 28 09:33:37 2017
> @@ -220,11 +220,18 @@
> msg->sm_flags = *flagstr;
> if (MSG_SECLEVEL(msg) < env->sc_min_seclevel ||
> msg->sm_secmodel != SNMP_SEC_USM) {
> - /* XXX currently only USM supported */
> - msg->sm_errstr = "unsupported security model";
> - stats->snmp_usmbadseclevel++;
> - msg->sm_usmerr = OIDVAL_usmErrSecLevel;
> - goto parsefail;
> + if (MSG_REPORT(msg)) {
> + msg->sm_errstr = "no such engineid";
> + stats->snmp_usmnosuchengine++;
> + msg->sm_usmerr = OIDVAL_usmErrEngineId;
> + goto parsefail;
> + } else {
> + /* XXX currently only USM supported */
> + msg->sm_errstr = "unsupported security model";
> + stats->snmp_usmbadseclevel++;
> + msg->sm_usmerr = OIDVAL_usmErrSecLevel;
> + goto parsefail;
> + }
> }
>
> if ((a = usm_decode(msg, a, &msg->sm_errstr)) == NULL)
> @@ -324,11 +331,12 @@
> msg->sm_errstr = "invalid PDU";
> goto fail;
> }
> - if (class != BER_CLASS_UNIVERSAL || type != BER_TYPE_SEQUENCE) {
> - stats->snmp_silentdrops++;
> - msg->sm_errstr = "invalid varbind";
> - goto fail;
> - }
> + /* XXX disabled for now */
> + /* if (class != BER_CLASS_UNIVERSAL || type != BER_TYPE_SEQUENCE) { */
> + /* stats->snmp_silentdrops++; */
> + /* msg->sm_errstr = "invalid varbind"; */
> + /* goto fail; */
> + /* } */
>
> msg->sm_request = req;
> msg->sm_error = errval;
> @@ -485,7 +493,8 @@
> struct snmp_stats *stats = &env->sc_stats;
> ssize_t len;
> struct snmp_message *msg;
> -
> + int ret;
> +
> if ((msg = calloc(1, sizeof(*msg))) == NULL)
> return;
>
> @@ -517,17 +526,19 @@
> smi_debug_elements(msg->sm_req);
> #endif
>
> - if (snmpe_parse(msg) == -1) {
> - if (msg->sm_usmerr != 0 && MSG_REPORT(msg)) {
> - usm_make_report(msg);
> - snmpe_response(fd, msg);
> - return;
> - } else {
> - snmp_msgfree(msg);
> - return;
> - }
> + ret = snmpe_parse(msg);
> +
> + if (msg->sm_usmerr != 0 && MSG_REPORT(msg)) {
> + usm_make_report(msg);
> + snmpe_response(fd, msg);
> + return;
> }
>
> + if (ret == -1) {
> + snmp_msgfree(msg);
> + return;
> + }
> +
> snmpe_dispatchmsg(msg, fd);
> }
>
> diff -ur snmpd/usm.c snmpd.modified/usm.c
> --- snmpd/usm.c Tue Mar 28 12:19:03 2017
> +++ snmpd.modified/usm.c Tue Mar 28 09:33:37 2017
> @@ -269,21 +269,20 @@
> 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++;
> + 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++;
> + if (engine_boots != 0LL && engine_time != 0LL)
> goto done;
> - }
> + } else {
> + msg->sm_engine_boots = (u_int32_t)engine_boots;
> + msg->sm_engine_time = (u_int32_t)engine_time;
> }
> -
> - msg->sm_engine_boots = (u_int32_t)engine_boots;
> - msg->sm_engine_time = (u_int32_t)engine_time;
> -
> +
> memcpy(msg->sm_username, user, userlen);
> msg->sm_username[userlen] = '\0';
> msg->sm_user = usm_finduser(msg->sm_username);
> @@ -481,9 +480,9 @@
> struct ber_oid usmstat = OID(MIB_usmStats, 0, 0);
>
> /* Always send report in clear-text */
> - msg->sm_flags = 0;
> + /* msg->sm_flags = 0; */
> msg->sm_context = SNMP_C_REPORT;
> - msg->sm_username[0] = '\0';
> + /* msg->sm_username[0] = '\0'; */
> usmstat.bo_id[OIDIDX_usmStats] = msg->sm_usmerr;
> usmstat.bo_n = OIDIDX_usmStats + 2;
> if (msg->sm_varbindresp != NULL)
>
