From ab12ca856cc7acfe7ccaa0849919181f53a3dc32 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Fri, 3 Jan 2020 21:18:49 -0500 Subject: [PATCH 1/3] zebra: fix multiple bfd buffer issues Whatever this BFD re-transmission function is had a few problems. 1. Used memcpy instead of the (more concise) stream APIs, which include bounds checking. 2. Did not sufficiently check packet sizes. Actually, 2) is mitigated but is still a problem, because the BFD header is 2 bytes larger than the "normal" ZAPI header, while the overall message size remains the same. So if the source message being duplicated is actually right up against the ZAPI_MAX_PACKET_SIZ, you still can't fit the whole message into your duplicated message. I have no idea what the intent was here but at least there's a warning if it happens now. Signed-off-by: Quentin Young --- zebra/zebra_ptm.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/zebra/zebra_ptm.c b/zebra/zebra_ptm.c index 46f1385520..cefbae5458 100644 --- a/zebra/zebra_ptm.c +++ b/zebra/zebra_ptm.c @@ -1402,37 +1402,25 @@ static void _zebra_ptm_reroute(struct zserv *zs, struct zebra_vrf *zvrf, struct stream *msg, uint32_t command) { struct stream *msgc; - size_t zmsglen, zhdrlen; + char buf[ZEBRA_MAX_PACKET_SIZ]; pid_t ppid; - /* - * Don't modify message in the zebra API. In order to do that we - * need to allocate a new message stream and copy the message - * provided by zebra. - */ + /* Create BFD header */ msgc = stream_new(ZEBRA_MAX_PACKET_SIZ); - if (msgc == NULL) { - zlog_debug("%s: not enough memory", __func__); - return; - } - - /* Calculate our header size plus the message contents. */ - zhdrlen = ZEBRA_HEADER_SIZE + sizeof(uint32_t); - zmsglen = msg->endp - msg->getp; - memcpy(msgc->data + zhdrlen, msg->data + msg->getp, zmsglen); - - /* - * The message type will be BFD_DEST_REPLY so we can use only - * one callback at the `bfdd` side, however the real command - * number will be included right after the zebra header. - */ zclient_create_header(msgc, ZEBRA_BFD_DEST_REPLAY, zvrf->vrf->vrf_id); stream_putl(msgc, command); - /* Update the data pointers. */ - msgc->getp = 0; - msgc->endp = zhdrlen + zmsglen; - stream_putw_at(msgc, 0, stream_get_endp(msgc)); + if (STREAM_READABLE(msg) > STREAM_WRITEABLE(msgc)) { + zlog_warn("Cannot fit extended BFD header plus original message contents into ZAPI packet; dropping message"); + goto stream_failure; + } + + /* Copy original message, excluding header, into new message */ + stream_get_from(buf, msg, stream_get_getp(msg), STREAM_READABLE(msg)); + stream_put(msgc, buf, STREAM_READABLE(msg)); + + /* Update length field */ + stream_putw_at(msgc, 0, STREAM_READABLE(msgc)); zebra_ptm_send_bfdd(msgc); From aa8cb96489df39b3fd7c7197df13f139b85e05e9 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Fri, 3 Jan 2020 21:22:44 -0500 Subject: [PATCH 2/3] zebra: reject ingress packets that are too large There may be logic to prevent this ever happening earlier in the network read path, but it doesn't hurt to double check it here, because clearly deeper paths rely on this being the case. Signed-off-by: Quentin Young --- zebra/zapi_msg.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 1dbe41f462..9d108f305b 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -2597,6 +2597,14 @@ void zserv_handle_commands(struct zserv *client, struct stream *msg) struct zmsghdr hdr; struct zebra_vrf *zvrf; + if (STREAM_READABLE(msg) > ZEBRA_MAX_PACKET_SIZ) { + if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV) + zlog_debug( + "ZAPI message is %zu bytes long but the maximum packet size is %u; dropping", + STREAM_READABLE(msg), ZEBRA_MAX_PACKET_SIZ); + return; + } + zapi_parse_header(msg, &hdr); if (IS_ZEBRA_DEBUG_PACKET && IS_ZEBRA_DEBUG_RECV) From 6d097bf15c1bcc6cb71b01f980dfe9a2bdd93ce9 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Mon, 6 Jan 2020 12:09:23 -0500 Subject: [PATCH 3/3] zebra: free ptm message on error Signed-off-by: Quentin Young --- zebra/zebra_ptm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra/zebra_ptm.c b/zebra/zebra_ptm.c index cefbae5458..55cb28ffc0 100644 --- a/zebra/zebra_ptm.c +++ b/zebra/zebra_ptm.c @@ -1431,6 +1431,7 @@ static void _zebra_ptm_reroute(struct zserv *zs, struct zebra_vrf *zvrf, return; stream_failure: + stream_free(msgc); zlog_err("%s:%d failed to registrate client pid", __FILE__, __LINE__); }