Download raw body.
snmpd/agentx compatibility issue
Moving to tech@
On Sun, 2024-02-11 at 11:17 +0100, Steffen Christgau wrote:
> Hi,
>
> I use snmpd on OpenBSD 7.4 and try to get it work with PyAgentX3 [1] in
> order to provide additional data to the SNMP daemon. To test it, I use
> the minimal agent as provided in the PyAgentX3 readme (with the "print"
> function adjusted to Python 3). The respective config line for snmpd
> looks as simple as it could be:
>
> agentx
>
> Connection management and registration between the two processes work.
> Unfortunately, no data is returned when issuing a GET command using both
> OpenBSD's "snmp get" on the local machine and NET-SNMP's "snmpget" on a
> remote Linux machine. Instead the connection to the subagent is aborted.
> Same applies to a "walk".
>
> The reason for that appears to be a check of the packetID in the AgentX
> PDU header by snmpd. From what I understand from RFC 2257, it is
> expected that the packetID of a response must the same as in the
> corresponding request. PyAgentX3 correctly does that. However, snmpd
> drops the response PDU during the handling of AX_PDU_TYPE_RESPONSE in
> ax_recv (ax.c) because it does not know the packet id, i.e. it's not in
> ax_packetids.
>
> From my point of view, this is due to appl_agentx_get calling ax_get
> with the request_id externally provided from the client (?). In ax_get,
> ax_pdu_header is called with that request_id. In turn, ax_pdu_header
> only asks for new IDs in case it is non-zero:
>
> if (packetid == 0)
> packetid = ax_packetid(ax);
> /* possible fix
> else if (type == AX_PDU_TYPE_GET /*NEXT ?! */)
> ax_record_packetid(packetid)
> */
>
> So essentially, the request ID is passed through to the agentx subagent
> but not recorded in the array of known IDs causing the refusal of the
> subagent's response and finally connection close of the agentx socket.
>
> Since I am not an agentx/snmp(d) expert, I'm not sure if that's a bug in
> snmpd or expected behavior. I would be happy if somebody could confirm
> either way.
>
> That being said, my current fix was to disable the check for known ids
> in the handling of AX_PDU_TYPE_RESPONSEs, but that's apparently a bad
> idea. With that, however, both walking tree and getting the respective
> OIDs work as intended. Maybe the pseudo code above would be a better fix.
>
> Regards, Steffen
>
> [1] https://github.com/rprinz08/pyagentx3
>
Thanks for the report and analysis. I'm also quite confident this
explains the two elusive bug reports I've received.
First of, your analysis is correct, but I'd prefer to remove the code
instead of creating an ax_record_packetid(). The reason this code was
originally added to ax.c was that I wanted to lift some burden out of
libagentx's agentx.c into ax.c to deduplicate a bit of code when it
came to administrative packets. When I copied this over to snmpd I
kept it, but the code was effectively never called, because ax_get,
and ax_getnext get their packetid from application.c's requestid.
ax_response simply copies the packetid, and other than that only
ax_close can be called, which closes the connection after that anyway.
So there's no reason to keep it around from that regard and ax.c is
supposed to be a translation layer between network and C format, so
this code also doesn't belong here from that perspective.
The reason that this triggers here and why it made the other bugs so
elusive is because ax_response()'s call to ax_pdu_header() also called
ax_packetid() unconditionally, which meant that an incoming packetid of
0 (what pyagentx3 does) would generated an (invalid) new outgoing
packetid and would enable the `if (ax->ax_packetids != NULL)` test for
incoming responses, making it reproducible. So how does this relate to
the earlier reported crashes? If once in a blue moon a requestid of 0 is
generated this code path would be enabled, resulting in EPROTO, which in
turn puts disables the backend. Where disabling a libexec backend for
snmpd results in snmpd shutting down.
This means that it should also be removed from libagentx, but ax's
usage there is a bit more involved, so I need a little more time to
build a diff for there.
OK?
martijn@
diff /usr/src/usr.sbin/snmpd
commit - 2f88df9020fe17d24fc9604ac3c283ef43597e3b
path + /usr/src/usr.sbin/snmpd
blob - 4a2509a2f7887ed9ba583a99f146041d313e1556
file + ax.c
--- ax.c
+++ ax.c
@@ -36,7 +36,6 @@ static int ax_pdu_need(struct ax *, size_t);
static int ax_pdu_header(struct ax *,
enum ax_pdu_type, uint8_t, uint32_t, uint32_t, uint32_t,
struct ax_ostring *);
-static uint32_t ax_packetid(struct ax *);
static uint32_t ax_pdu_queue(struct ax *);
static int ax_pdu_add_uint16(struct ax *, uint16_t);
static int ax_pdu_add_uint32(struct ax *, uint32_t);
@@ -89,7 +88,6 @@ ax_free(struct ax *ax)
close(ax->ax_fd);
free(ax->ax_rbuf);
free(ax->ax_wbuf);
- free(ax->ax_packetids);
free(ax);
}
@@ -394,24 +392,6 @@ ax_recv(struct ax *ax)
}
break;
case AX_PDU_TYPE_RESPONSE:
- if (ax->ax_packetids != NULL) {
- found = 0;
- for (i = 0; ax->ax_packetids[i] != 0; i++) {
- if (ax->ax_packetids[i] ==
- pdu->ap_header.aph_packetid) {
- packetidx = i;
- found = 1;
- }
- }
- if (found) {
- ax->ax_packetids[packetidx] =
- ax->ax_packetids[i - 1];
- ax->ax_packetids[i - 1] = 0;
- } else {
- errno = EPROTO;
- goto fail;
- }
- }
if (rawlen < 8) {
errno = EPROTO;
goto fail;
@@ -543,7 +523,7 @@ uint32_t
ax_close(struct ax *ax, uint32_t sessionid,
enum ax_close_reason reason)
{
- if (ax_pdu_header(ax, AX_PDU_TYPE_CLOSE, 0, sessionid, 0, 0,
+ if (ax_pdu_header(ax, AX_PDU_TYPE_CLOSE, 0, sessionid, arc4random(), 0,
NULL) == -1)
return 0;
@@ -1163,8 +1143,6 @@ ax_pdu_header(struct ax *ax, enum ax_pdu_type type, ui
flags |= AX_PDU_FLAG_NETWORK_BYTE_ORDER;
ax->ax_wbuf[ax->ax_wbtlen++] = flags;
ax->ax_wbuf[ax->ax_wbtlen++] = 0;
- if (packetid == 0)
- packetid = ax_packetid(ax);
if (ax_pdu_add_uint32(ax, sessionid) == -1 ||
ax_pdu_add_uint32(ax, transactionid) == -1 ||
ax_pdu_add_uint32(ax, packetid) == -1 ||
@@ -1179,40 +1157,6 @@ ax_pdu_header(struct ax *ax, enum ax_pdu_type type, ui
return 0;
}
-static uint32_t
-ax_packetid(struct ax *ax)
-{
- uint32_t packetid, *packetids;
- size_t npackets = 0, i;
- int found;
-
- if (ax->ax_packetids != NULL) {
- for (npackets = 0; ax->ax_packetids[npackets] != 0; npackets++)
- continue;
- }
- if (ax->ax_packetidsize == 0 || npackets == ax->ax_packetidsize - 1) {
- packetids = recallocarray(ax->ax_packetids, ax->ax_packetidsize,
- ax->ax_packetidsize + 25, sizeof(*packetids));
- if (packetids == NULL)
- return 0;
- ax->ax_packetidsize += 25;
- ax->ax_packetids = packetids;
- }
- do {
- found = 0;
- packetid = arc4random();
- for (i = 0; ax->ax_packetids[i] != 0; i++) {
- if (ax->ax_packetids[i] == packetid) {
- found = 1;
- break;
- }
- }
- } while (packetid == 0 || found);
- ax->ax_packetids[npackets] = packetid;
-
- return packetid;
-}
-
static int
ax_pdu_add_uint16(struct ax *ax, uint16_t value)
{
blob - 36006221ab07fda06ee3aeaa5365f9170ea1d78a
file + ax.h
--- ax.h
+++ ax.h
@@ -125,8 +125,6 @@ struct ax {
size_t ax_wblen;
size_t ax_wbtlen;
size_t ax_wbsize;
- uint32_t *ax_packetids;
- size_t ax_packetidsize;
};
#ifndef AX_PRIMITIVE
snmpd/agentx compatibility issue