Re: libagentx: Don't allow OIDs < 2
On Sun, Oct 24, 2021 at 06:39:42PM +0100, Martijn van Duren wrote:
> libagentx currently allows OIDs with a length of 0.
> This isn't wrong from an agentx protocol point of view, but ber encoding
> can't handle OIDs with less then 2 elements, which makes it unable to
> map the values back to SNMP. netsnmp maps a null-oid to 0.0, but I don't
> think we should do that.
>
> This diff also adds the check to a couple of other places where there
> was no active length checking prior. This is not an issue with current
> code using it (relayd(8)), because all OIDs are static, so are not
> susceptible for manipulation.
>
> regress still passes.
>
> OK?
Agreed that this is the right approach. agentx protocol spec was sloppy
in this regard. NULL OIDs only make sense for some messages but not for
others.
Diff looks OK.
> Index: agentx.h
> ===
> RCS file: /cvs/src/lib/libagentx/agentx.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 agentx.h
> --- agentx.h 27 Oct 2020 18:24:01 - 1.5
> +++ agentx.h 24 Oct 2021 17:39:16 -
> @@ -36,6 +36,7 @@ enum agentx_request_type {
> };
>
> #define AGENTX_MASTER_PATH "/var/agentx/master"
> +#define AGENTX_OID_MIN_LEN 2
> #define AGENTX_OID_MAX_LEN 128
> #define AGENTX_OID_INDEX_MAX_LEN 10
> #define AGENTX_MIB2 1, 3, 6, 1, 2, 1
> Index: agentx.c
> ===
> RCS file: /cvs/src/lib/libagentx/agentx.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 agentx.c
> --- agentx.c 23 Oct 2021 17:13:50 - 1.13
> +++ agentx.c 24 Oct 2021 17:39:17 -
> @@ -189,6 +189,8 @@ static int agentx_request(struct agentx
> static int agentx_request_cmp(struct agentx_request *,
> struct agentx_request *);
> static int agentx_strcat(char **, const char *);
> +static int agentx_oidfill(struct ax_oid *, const uint32_t[], size_t,
> +const char **);
>
> RB_PROTOTYPE_STATIC(ax_requests, agentx_request, axr_ax_requests,
> agentx_request_cmp)
> @@ -362,25 +364,26 @@ agentx_session(struct agentx *ax, uint32
> size_t oidlen, const char *descr, uint8_t timeout)
> {
> struct agentx_session *axs;
> - size_t i;
> + const char *errstr;
>
> - if (oidlen > AGENTX_OID_MAX_LEN) {
> -#ifdef AX_DEBUG
> - agentx_log_ax_fatalx(ax, "%s: oidlen > %d", __func__,
> - AGENTX_OID_MAX_LEN);
> -#else
> - errno = EINVAL;
> - return NULL;
> -#endif
> - }
> if ((axs = calloc(1, sizeof(*axs))) == NULL)
> return NULL;
>
> axs->axs_ax = ax;
> axs->axs_timeout = timeout;
> - for (i = 0; i < oidlen; i++)
> - axs->axs_oid.aoi_id[i] = oid[i];
> - axs->axs_oid.aoi_idlen = oidlen;
> + /* RFC 2741 section 6.2.1: may send a null Object Identifier */
> + if (oidlen == 0)
> + axs->axs_oid.aoi_idlen = oidlen;
> + else {
> + if (agentx_oidfill((&axs->axs_oid), oid, oidlen,
> + &errstr) == -1) {
> +#ifdef AX_DEBUG
> + agentx_log_ax_fatalx(ax, "%s: %s", __func__, errstr);
> +#else
> + return NULL;
> +#endif
> + }
> + }
> axs->axs_descr.aos_string = (unsigned char *)strdup(descr);
> if (axs->axs_descr.aos_string == NULL) {
> free(axs);
> @@ -670,11 +673,21 @@ agentx_context_object_find(struct agentx
> const uint32_t oid[], size_t oidlen, int active, int instance)
> {
> struct agentx_object *axo, axo_search;
> - size_t i;
> + const char *errstr;
>
> - for (i = 0; i < oidlen; i++)
> - axo_search.axo_oid.aoi_id[i] = oid[i];
> - axo_search.axo_oid.aoi_idlen = oidlen;
> + if (agentx_oidfill(&(axo_search.axo_oid), oid, oidlen, &errstr) == -1) {
> + if (oidlen > AGENTX_OID_MIN_LEN) {
> +#ifdef AX_DEBUG
> + agentx_log_axc_fatalx(axc, "%s: %s", __func__, errstr);
> +#else
> + agentx_log_axc_warnx(axc, "%s: %s", __func__, errstr);
> + return NULL;
> + }
> +#endif
> + if (oidlen == 1)
> + axo_search.axo_oid.aoi_id[0] = oid[0];
> + axo_search.axo_oid.aoi_idlen = oidlen;
> + }
>
> axo = RB_FIND(axc_objects, &(axc->axc_objects), &axo_search);
> while (axo == NULL && !instance && axo_search.axo_oid.aoi_idlen > 0) {
> @@ -691,11 +704,21 @@ agentx_context_object_nfind(struct agent
> const uint32_t oid[], size_t oidlen, int active, int inclusive)
> {
> struct agentx_object *axo, axo_search;
> - size_t i;
> + const char *errstr;
>
> - for (i = 0; i < oidlen; i++)
> - axo_search.axo_oid.aoi_id[i] = oid[i];
> - axo_search.axo_oid.aoi_idlen = oidlen;
> + if (agentx_oidfill(&(axo_search.axo_oid), oid, oidlen, &errstr) == -1) {
> + if (oidlen > AGENTX_OID_MIN_LEN) {
> +#ifdef AX_DEBUG
> +
libagentx: Don't allow OIDs < 2
libagentx currently allows OIDs with a length of 0.
This isn't wrong from an agentx protocol point of view, but ber encoding
can't handle OIDs with less then 2 elements, which makes it unable to
map the values back to SNMP. netsnmp maps a null-oid to 0.0, but I don't
think we should do that.
This diff also adds the check to a couple of other places where there
was no active length checking prior. This is not an issue with current
code using it (relayd(8)), because all OIDs are static, so are not
susceptible for manipulation.
regress still passes.
OK?
Index: agentx.h
===
RCS file: /cvs/src/lib/libagentx/agentx.h,v
retrieving revision 1.5
diff -u -p -r1.5 agentx.h
--- agentx.h27 Oct 2020 18:24:01 - 1.5
+++ agentx.h24 Oct 2021 17:39:16 -
@@ -36,6 +36,7 @@ enum agentx_request_type {
};
#define AGENTX_MASTER_PATH "/var/agentx/master"
+#define AGENTX_OID_MIN_LEN 2
#define AGENTX_OID_MAX_LEN 128
#define AGENTX_OID_INDEX_MAX_LEN 10
#define AGENTX_MIB2 1, 3, 6, 1, 2, 1
Index: agentx.c
===
RCS file: /cvs/src/lib/libagentx/agentx.c,v
retrieving revision 1.13
diff -u -p -r1.13 agentx.c
--- agentx.c23 Oct 2021 17:13:50 - 1.13
+++ agentx.c24 Oct 2021 17:39:17 -
@@ -189,6 +189,8 @@ static int agentx_request(struct agentx
static int agentx_request_cmp(struct agentx_request *,
struct agentx_request *);
static int agentx_strcat(char **, const char *);
+static int agentx_oidfill(struct ax_oid *, const uint32_t[], size_t,
+const char **);
RB_PROTOTYPE_STATIC(ax_requests, agentx_request, axr_ax_requests,
agentx_request_cmp)
@@ -362,25 +364,26 @@ agentx_session(struct agentx *ax, uint32
size_t oidlen, const char *descr, uint8_t timeout)
{
struct agentx_session *axs;
- size_t i;
+ const char *errstr;
- if (oidlen > AGENTX_OID_MAX_LEN) {
-#ifdef AX_DEBUG
- agentx_log_ax_fatalx(ax, "%s: oidlen > %d", __func__,
- AGENTX_OID_MAX_LEN);
-#else
- errno = EINVAL;
- return NULL;
-#endif
- }
if ((axs = calloc(1, sizeof(*axs))) == NULL)
return NULL;
axs->axs_ax = ax;
axs->axs_timeout = timeout;
- for (i = 0; i < oidlen; i++)
- axs->axs_oid.aoi_id[i] = oid[i];
- axs->axs_oid.aoi_idlen = oidlen;
+ /* RFC 2741 section 6.2.1: may send a null Object Identifier */
+ if (oidlen == 0)
+ axs->axs_oid.aoi_idlen = oidlen;
+ else {
+ if (agentx_oidfill((&axs->axs_oid), oid, oidlen,
+ &errstr) == -1) {
+#ifdef AX_DEBUG
+ agentx_log_ax_fatalx(ax, "%s: %s", __func__, errstr);
+#else
+ return NULL;
+#endif
+ }
+ }
axs->axs_descr.aos_string = (unsigned char *)strdup(descr);
if (axs->axs_descr.aos_string == NULL) {
free(axs);
@@ -670,11 +673,21 @@ agentx_context_object_find(struct agentx
const uint32_t oid[], size_t oidlen, int active, int instance)
{
struct agentx_object *axo, axo_search;
- size_t i;
+ const char *errstr;
- for (i = 0; i < oidlen; i++)
- axo_search.axo_oid.aoi_id[i] = oid[i];
- axo_search.axo_oid.aoi_idlen = oidlen;
+ if (agentx_oidfill(&(axo_search.axo_oid), oid, oidlen, &errstr) == -1) {
+ if (oidlen > AGENTX_OID_MIN_LEN) {
+#ifdef AX_DEBUG
+ agentx_log_axc_fatalx(axc, "%s: %s", __func__, errstr);
+#else
+ agentx_log_axc_warnx(axc, "%s: %s", __func__, errstr);
+ return NULL;
+ }
+#endif
+ if (oidlen == 1)
+ axo_search.axo_oid.aoi_id[0] = oid[0];
+ axo_search.axo_oid.aoi_idlen = oidlen;
+ }
axo = RB_FIND(axc_objects, &(axc->axc_objects), &axo_search);
while (axo == NULL && !instance && axo_search.axo_oid.aoi_idlen > 0) {
@@ -691,11 +704,21 @@ agentx_context_object_nfind(struct agent
const uint32_t oid[], size_t oidlen, int active, int inclusive)
{
struct agentx_object *axo, axo_search;
- size_t i;
+ const char *errstr;
- for (i = 0; i < oidlen; i++)
- axo_search.axo_oid.aoi_id[i] = oid[i];
- axo_search.axo_oid.aoi_idlen = oidlen;
+ if (agentx_oidfill(&(axo_search.axo_oid), oid, oidlen, &errstr) == -1) {
+ if (oidlen > AGENTX_OID_MIN_LEN) {
+#ifdef AX_DEBUG
+ agentx_log_axc_fatalx(axc, "%s: %s", __func__, errstr);
+#else
+ agentx_log_axc_warnx(axc, "%s: %s", __func__, errstr);
+ return NULL;
+#endif
+ }
+ if (oidlen == 1)
+ axo_search.axo_oid.aoi_id[0] = oid[0];
+ axo_search.axo_oid.aoi_idlen = oi
