Download raw body.
snmpd(8): Be stricter with varbind value ranges/encodings
While reading RFC2578 I noticed some restrictions that are placed on
returned values that are currently not checked for. I don't think we
should blindly rely on all clients doing the right thing outside the
standardised ranges.
Restrictions according to section 7 are:
- Integer: -2147483648 to 2147483647 decimal
- octet string: max 65535 octets
- object identifier: at most 128 sub-identifiers. Further, each sub-
identifier must not exceed the value 2^32-1 (4294967295 decimal).
- ipaddress: an OCTET STRING of length 4
- counter32: non-negative integer until it reaches a maximum value of
2^32-1 (4294967295 decimal)
- gauge32: non-negative integer The maximum value can not be greater
than 2^32-1 4294967295 decimal
- timeticks: non-negative integer modulo 2^32 (4294967296 decimal)
- opaque: A value is encoded using the ASN.1 Basic Encoding Rules into a
string of octets
- counter64: non-negative integer until it reaches a maximum value of
2^64-1 (18446744073709551615 decimal)
AgentX has the numeric values already restricted in its binary format
and are thus unlikely to be hit (unless I overlooked something in the
internal/conf backend). However, this restriction might not be there
if another backend would ever be added.
Object identifier is already restricted by the ber.c types.
Octet string, ipaddress, and opaque can be violated via AgentX.
The opaque section states: The Opaque type is provided solely for
backward-compatibility, and shall not be used for newly-defined object
types. Hence I don't expect this one to be used outside of the
regression tests and I don't see it as a problem to take the brute
force approach with this one.
While here move the NULL value check all the way up and make the errstr
returns a bit more precise.
OK?
martijn@
diff /usr/src/usr.sbin/snmpd
commit - 96477c772b855c1b46d6d291c45854e18ee5bf5b
path + /usr/src/usr.sbin/snmpd
blob - f6bd60964497ffc539575056ec0ba8e96a04bb89
file + application.c
--- application.c
+++ application.c
@@ -153,6 +153,7 @@ void appl_request_upstream_reply(struct appl_request_u
int appl_varbind_valid(struct appl_varbind *, struct appl_varbind_internal *,
int, int, int, const char **);
int appl_error_valid(enum appl_error, enum snmp_pdutype);
+unsigned int appl_ber_any(struct ber_element *);
int appl_varbind_backend(struct appl_varbind_internal *);
void appl_varbind_error(struct appl_varbind_internal *, enum appl_error);
void appl_pdu_log(struct appl_backend *, enum snmp_pdutype, int32_t, uint16_t,
@@ -1427,6 +1428,11 @@ appl_varbind_valid(struct appl_varbind *varbind,
{
int cmp;
int eomv = 0;
+ long long intval;
+ void *buf;
+ size_t len;
+ struct ber ber;
+ struct ber_element *elm;
if (null)
next = 0;
@@ -1438,36 +1444,111 @@ appl_varbind_valid(struct appl_varbind *varbind,
}
return 1;
}
+
+ if (null) {
+ if (varbind->av_value->be_class != BER_CLASS_UNIVERSAL ||
+ varbind->av_value->be_type != BER_TYPE_NULL) {
+ *errstr = "expecting null value";
+ return 0;
+ }
+ } else {
+ if (varbind->av_value->be_class == BER_CLASS_UNIVERSAL &&
+ varbind->av_value->be_type == BER_TYPE_NULL) {
+ *errstr = "unexpected null value";
+ return 0;
+ }
+ }
+
if (varbind->av_value->be_class == BER_CLASS_UNIVERSAL) {
switch (varbind->av_value->be_type) {
case BER_TYPE_NULL:
- if (null)
- break;
- *errstr = "not expecting null value";
- return 0;
+ /* Checked above */
+ break;
case BER_TYPE_INTEGER:
+ (void)ober_get_integer(varbind->av_value, &intval);
+ if (intval < SMI_INTEGER_MIN) {
+ *errstr = "INTEGER value too small";
+ return 0;
+ }
+ if (intval > SMI_INTEGER_MAX) {
+ *errstr = "INTEGER value too large";
+ return 0;
+ }
+ break;
case BER_TYPE_OCTETSTRING:
+ (void)ober_get_nstring(varbind->av_value, NULL, &len);
+ if (len > SMI_OCTETSTRING_MAX) {
+ *errstr = "OCTET STRING too long";
+ return 0;
+ }
+ break;
case BER_TYPE_OBJECT:
- if (!null)
- break;
- /* FALLTHROUGH */
+ /* validated by ber.c */
+ break;
default:
- *errstr = "invalid value";
+ *errstr = "invalid value encoding";
return 0;
}
} else if (varbind->av_value->be_class == BER_CLASS_APPLICATION) {
switch (varbind->av_value->be_type) {
case SNMP_T_IPADDR:
+ (void)ober_get_nstring(varbind->av_value, NULL, &len);
+ if (len != SMI_IPADDRESS_MAX) {
+ *errstr = "invalid IpAddress size";
+ return 0;
+ }
+ break;
case SNMP_T_COUNTER32:
+ (void)ober_get_integer(varbind->av_value, &intval);
+ if (intval < SMI_COUNTER32_MIN) {
+ *errstr = "Counter32 value too small";
+ return 0;
+ }
+ if (intval > SMI_COUNTER32_MAX) {
+ *errstr = "Counter32 value too large";
+ return 0;
+ }
+ break;
case SNMP_T_GAUGE32:
+ (void)ober_get_integer(varbind->av_value, &intval);
+ if (intval < SMI_GAUGE32_MIN) {
+ *errstr = "Gauge32 value too small";
+ return 0;
+ }
+ if (intval > SMI_GAUGE32_MAX) {
+ *errstr = "Gauge32 value too large";
+ return 0;
+ }
+ break;
case SNMP_T_TIMETICKS:
+ (void)ober_get_integer(varbind->av_value, &intval);
+ if (intval < SMI_TIMETICKS_MIN) {
+ *errstr = "TimeTicks value too small";
+ return 0;
+ }
+ if (intval > SMI_TIMETICKS_MAX) {
+ *errstr = "TimeTicks value too large";
+ return 0;
+ }
+ break;
case SNMP_T_OPAQUE:
+ (void)ober_get_nstring(varbind->av_value, &buf, &len);
+ memset(&ber, 0, sizeof(ber));
+ ober_set_application(&ber, appl_ber_any);
+ ober_set_readbuf(&ber, buf, len);
+ elm = ober_read_elements(&ber, NULL);
+ if (elm == NULL || ober_calc_len(elm) != len) {
+ ober_free_elements(elm);
+ *errstr = "Opaque not valid ber encoded value";
+ return 0;
+ }
+ ober_free_elements(elm);
+ break;
case SNMP_T_COUNTER64:
- if (!null)
- break;
- /* FALLTHROUGH */
+ /* Maximum value supported by ber.c */
+ break;
default:
- *errstr = "expecting null value";
+ *errstr = "invalid value encoding";
return 0;
}
} else if (varbind->av_value->be_class == BER_CLASS_CONTEXT) {
@@ -1477,22 +1558,14 @@ appl_varbind_valid(struct appl_varbind *varbind,
*errstr = "Unexpected noSuchObject";
return 0;
}
- /* FALLTHROUGH */
+ break;
case APPL_EXC_NOSUCHINSTANCE:
- if (null) {
- *errstr = "expecting null value";
- return 0;
- }
if (next && request != NULL) {
*errstr = "Unexpected noSuchInstance";
return 0;
}
break;
case APPL_EXC_ENDOFMIBVIEW:
- if (null) {
- *errstr = "expecting null value";
- return 0;
- }
if (!next && request != NULL) {
*errstr = "Unexpected endOfMibView";
return 0;
@@ -1500,11 +1573,11 @@ appl_varbind_valid(struct appl_varbind *varbind,
eomv = 1;
break;
default:
- *errstr = "invalid exception";
+ *errstr = "invalid value encoding";
return 0;
}
} else {
- *errstr = "invalid value";
+ *errstr = "invalid value encoding";
return 0;
}
@@ -1546,6 +1619,12 @@ appl_varbind_valid(struct appl_varbind *varbind,
return 1;
}
+unsigned int
+appl_ber_any(struct ber_element *elm)
+{
+ return BER_TYPE_OCTETSTRING;
+}
+
int
appl_error_valid(enum appl_error error, enum snmp_pdutype type)
{
blob - c0a9a0cf4ca4796f56052ed09f0035637803c55a
file + snmp.h
--- snmp.h
+++ snmp.h
@@ -157,4 +157,17 @@ enum snmp_security_model {
#define SNMP_MAX_TIMEWINDOW 150 /* RFC3414 */
+/* RFC2578 */
+#define SMI_INTEGER_MIN -2147483648
+#define SMI_INTEGER_MAX 2147483647
+#define SMI_OCTETSTRING_MAX 65535
+#define SMI_IPADDRESS_MAX 4
+#define SMI_COUNTER32_MIN 0
+#define SMI_COUNTER32_MAX 4294967295
+#define SMI_GAUGE32_MIN 0
+#define SMI_GAUGE32_MAX 4294967295
+#define SMI_TIMETICKS_MIN 0
+#define SMI_TIMETICKS_MAX 4294967295
+
+
#endif /* SNMPD_SNMP_H */
snmpd(8): Be stricter with varbind value ranges/encodings