From 83f8a12b8ecbc3ffb285a59b6ce0a86e3a0cfb8f Mon Sep 17 00:00:00 2001 From: Satheesh Kumar K Date: Thu, 10 Oct 2019 21:33:19 -0700 Subject: [PATCH] lib, pimd, zebra: Provide some insurance against reading bad stream data This patch does two things: 1) Ensure the decoding of stream data between pim <-> zebra is properly decoded and we don't read beyond the end of the stream. 2) In zebra when we are freeing memory alloced ensure that we actually have memory to delete before we do so. Ticket: CM-27055 Signed-off-by: Satheesh Kumar K Signed-off-by: Donald Sharp --- lib/mlag.c | 28 +++++++++++---- lib/mlag.h | 9 +++-- pimd/pim_mlag.c | 13 ++++--- zebra/zebra_mlag.c | 87 +++++++++++++++++++++++++++++++++------------- 4 files changed, 96 insertions(+), 41 deletions(-) diff --git a/lib/mlag.c b/lib/mlag.c index 1daf290725..733dd41ea8 100644 --- a/lib/mlag.c +++ b/lib/mlag.c @@ -81,22 +81,33 @@ char *mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf, size_t size) } -int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg) +int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg, + size_t *length) { - if (s == NULL || msg == NULL) +#define LIB_MLAG_HDR_LENGTH 8 + *length = stream_get_endp(s); + + if (s == NULL || msg == NULL || *length < LIB_MLAG_HDR_LENGTH) return -1; + *length -= LIB_MLAG_HDR_LENGTH; + STREAM_GETL(s, msg->msg_type); STREAM_GETW(s, msg->data_len); STREAM_GETW(s, msg->msg_cnt); + return 0; stream_failure: return -1; } -int mlag_lib_decode_mroute_add(struct stream *s, struct mlag_mroute_add *msg) +#define MLAG_MROUTE_ADD_LENGTH \ + (VRF_NAMSIZ + INTERFACE_NAMSIZ + 4 + 4 + 4 + 4 + 1 + 1 + 4) + +int mlag_lib_decode_mroute_add(struct stream *s, struct mlag_mroute_add *msg, + size_t *length) { - if (s == NULL || msg == NULL) + if (s == NULL || msg == NULL || *length < MLAG_MROUTE_ADD_LENGTH) return -1; STREAM_GET(msg->vrf_name, s, VRF_NAMSIZ); @@ -108,14 +119,18 @@ int mlag_lib_decode_mroute_add(struct stream *s, struct mlag_mroute_add *msg) STREAM_GETC(s, msg->am_i_dual_active); STREAM_GETL(s, msg->vrf_id); STREAM_GET(msg->intf_name, s, INTERFACE_NAMSIZ); + return 0; stream_failure: return -1; } -int mlag_lib_decode_mroute_del(struct stream *s, struct mlag_mroute_del *msg) +#define MLAG_MROUTE_DEL_LENGTH (VRF_NAMSIZ + INTERFACE_NAMSIZ + 4 + 4 + 4 + 4) + +int mlag_lib_decode_mroute_del(struct stream *s, struct mlag_mroute_del *msg, + size_t *length) { - if (s == NULL || msg == NULL) + if (s == NULL || msg == NULL || *length < MLAG_MROUTE_DEL_LENGTH) return -1; STREAM_GET(msg->vrf_name, s, VRF_NAMSIZ); @@ -124,6 +139,7 @@ int mlag_lib_decode_mroute_del(struct stream *s, struct mlag_mroute_del *msg) STREAM_GETL(s, msg->owner_id); STREAM_GETL(s, msg->vrf_id); STREAM_GET(msg->intf_name, s, INTERFACE_NAMSIZ); + return 0; stream_failure: return -1; diff --git a/lib/mlag.h b/lib/mlag.h index c531fb5b68..37bb3aa6db 100644 --- a/lib/mlag.h +++ b/lib/mlag.h @@ -125,11 +125,14 @@ struct mlag_msg { extern char *mlag_role2str(enum mlag_role role, char *buf, size_t size); extern char *mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf, size_t size); -extern int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg); +extern int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg, + size_t *length); extern int mlag_lib_decode_mroute_add(struct stream *s, - struct mlag_mroute_add *msg); + struct mlag_mroute_add *msg, + size_t *length); extern int mlag_lib_decode_mroute_del(struct stream *s, - struct mlag_mroute_del *msg); + struct mlag_mroute_del *msg, + size_t *length); extern int mlag_lib_decode_mlag_status(struct stream *s, struct mlag_status *msg); extern int mlag_lib_decode_vxlan_update(struct stream *s, diff --git a/pimd/pim_mlag.c b/pimd/pim_mlag.c index 321745590d..0a6ff41114 100644 --- a/pimd/pim_mlag.c +++ b/pimd/pim_mlag.c @@ -765,8 +765,9 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len) struct mlag_msg mlag_msg; char buf[ZLOG_FILTER_LENGTH_MAX]; int rc = 0; + size_t length; - rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg); + rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg, &length); if (rc) return (rc); @@ -805,7 +806,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len) case MLAG_MROUTE_ADD: { struct mlag_mroute_add msg; - rc = mlag_lib_decode_mroute_add(s, &msg); + rc = mlag_lib_decode_mroute_add(s, &msg, &length); if (rc) return (rc); pim_mlag_process_mroute_add(msg); @@ -813,7 +814,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len) case MLAG_MROUTE_DEL: { struct mlag_mroute_del msg; - rc = mlag_lib_decode_mroute_del(s, &msg); + rc = mlag_lib_decode_mroute_del(s, &msg, &length); if (rc) return (rc); pim_mlag_process_mroute_del(msg); @@ -823,8 +824,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len) int i; for (i = 0; i < mlag_msg.msg_cnt; i++) { - - rc = mlag_lib_decode_mroute_add(s, &msg); + rc = mlag_lib_decode_mroute_add(s, &msg, &length); if (rc) return (rc); pim_mlag_process_mroute_add(msg); @@ -835,8 +835,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len) int i; for (i = 0; i < mlag_msg.msg_cnt; i++) { - - rc = mlag_lib_decode_mroute_del(s, &msg); + rc = mlag_lib_decode_mroute_del(s, &msg, &length); if (rc) return (rc); pim_mlag_process_mroute_del(msg); diff --git a/zebra/zebra_mlag.c b/zebra/zebra_mlag.c index 5b721a8eac..edd71b9f77 100644 --- a/zebra/zebra_mlag.c +++ b/zebra/zebra_mlag.c @@ -667,14 +667,17 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) int n_len = 0; int rc = 0; char buf[ZLOG_FILTER_LENGTH_MAX]; + size_t length; if (IS_ZEBRA_DEBUG_MLAG) zlog_debug("%s: Entering..", __func__); - rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg); + rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg, &length); if (rc) return rc; + memset(tmp_buf, 0, ZEBRA_MLAG_BUF_LIMIT); + if (IS_ZEBRA_DEBUG_MLAG) zlog_debug("%s: Mlag ProtoBuf encoding of message:%s, len:%d", __func__, @@ -688,9 +691,10 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) ZebraMlagMrouteAdd pay_load = ZEBRA_MLAG_MROUTE_ADD__INIT; uint32_t vrf_name_len = 0; - rc = mlag_lib_decode_mroute_add(s, &msg); + rc = mlag_lib_decode_mroute_add(s, &msg, &length); if (rc) return rc; + vrf_name_len = strlen(msg.vrf_name) + 1; pay_load.vrf_name = XMALLOC(MTYPE_MLAG_PBUF, vrf_name_len); strlcpy(pay_load.vrf_name, msg.vrf_name, vrf_name_len); @@ -720,7 +724,7 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) ZebraMlagMrouteDel pay_load = ZEBRA_MLAG_MROUTE_DEL__INIT; uint32_t vrf_name_len = 0; - rc = mlag_lib_decode_mroute_del(s, &msg); + rc = mlag_lib_decode_mroute_del(s, &msg, &length); if (rc) return rc; vrf_name_len = strlen(msg.vrf_name) + 1; @@ -749,18 +753,18 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) ZebraMlagMrouteAddBulk Bulk_msg = ZEBRA_MLAG_MROUTE_ADD_BULK__INIT; ZebraMlagMrouteAdd **pay_load = NULL; - int i; bool cleanup = false; + uint32_t i, actual; Bulk_msg.n_mroute_add = mlag_msg.msg_cnt; pay_load = XMALLOC(MTYPE_MLAG_PBUF, sizeof(ZebraMlagMrouteAdd *) * mlag_msg.msg_cnt); - for (i = 0; i < mlag_msg.msg_cnt; i++) { + for (i = 0, actual = 0; i < mlag_msg.msg_cnt; i++, actual++) { uint32_t vrf_name_len = 0; - rc = mlag_lib_decode_mroute_add(s, &msg); + rc = mlag_lib_decode_mroute_add(s, &msg, &length); if (rc) { cleanup = true; break; @@ -796,8 +800,17 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) tmp_buf); } - for (i = 0; i < mlag_msg.msg_cnt; i++) { - XFREE(MTYPE_MLAG_PBUF, pay_load[i]->vrf_name); + for (i = 0; i < actual; i++) { + /* + * The mlag_lib_decode_mroute_add can + * fail to properly decode and cause nothing + * to be allocated. Prevent a crash + */ + if (!pay_load[i]) + continue; + + if (pay_load[i]->vrf_name) + XFREE(MTYPE_MLAG_PBUF, pay_load[i]->vrf_name); if (pay_load[i]->owner_id == MLAG_OWNER_INTERFACE && pay_load[i]->intf_name) XFREE(MTYPE_MLAG_PBUF, pay_load[i]->intf_name); @@ -812,18 +825,18 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) ZebraMlagMrouteDelBulk Bulk_msg = ZEBRA_MLAG_MROUTE_DEL_BULK__INIT; ZebraMlagMrouteDel **pay_load = NULL; - int i; bool cleanup = false; + uint32_t i, actual; Bulk_msg.n_mroute_del = mlag_msg.msg_cnt; pay_load = XMALLOC(MTYPE_MLAG_PBUF, sizeof(ZebraMlagMrouteDel *) * mlag_msg.msg_cnt); - for (i = 0; i < mlag_msg.msg_cnt; i++) { + for (i = 0, actual = 0; i < mlag_msg.msg_cnt; i++, actual++) { uint32_t vrf_name_len = 0; - rc = mlag_lib_decode_mroute_del(s, &msg); + rc = mlag_lib_decode_mroute_del(s, &msg, &length); if (rc) { cleanup = true; break; @@ -858,8 +871,17 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) tmp_buf); } - for (i = 0; i < mlag_msg.msg_cnt; i++) { - XFREE(MTYPE_MLAG_PBUF, pay_load[i]->vrf_name); + for (i = 0; i < actual; i++) { + /* + * The mlag_lib_decode_mroute_add can + * fail to properly decode and cause nothing + * to be allocated. Prevent a crash + */ + if (!pay_load[i]) + continue; + + if (pay_load[i]->vrf_name) + XFREE(MTYPE_MLAG_PBUF, pay_load[i]->vrf_name); if (pay_load[i]->owner_id == MLAG_OWNER_INTERFACE && pay_load[i]->intf_name) XFREE(MTYPE_MLAG_PBUF, pay_load[i]->intf_name); @@ -915,6 +937,15 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type) return len; } +static void zebra_fill_protobuf_msg(struct stream *s, char *name, int len) +{ + int str_len = strlen(name) + 1; + + stream_put(s, name, str_len); + /* Fill the rest with Null Character for aligning */ + stream_put(s, NULL, len - str_len); +} + int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, uint32_t len) { @@ -966,7 +997,8 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, /* No Batching */ stream_putw(s, MLAG_MSG_NO_BATCH); /* Actual Data */ - stream_put(s, msg->peerlink, INTERFACE_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->peerlink, + INTERFACE_NAMSIZ); stream_putl(s, msg->my_role); stream_putl(s, msg->peer_state); zebra_mlag_status_update__free_unpacked(msg, NULL); @@ -1003,7 +1035,7 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, /* No Batching */ stream_putw(s, MLAG_MSG_NO_BATCH); /* Actual Data */ - stream_put(s, msg->vrf_name, VRF_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->vrf_name, VRF_NAMSIZ); stream_putl(s, msg->source_ip); stream_putl(s, msg->group_ip); @@ -1013,7 +1045,8 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, stream_putc(s, msg->am_i_dual_active); stream_putl(s, msg->vrf_id); if (msg->owner_id == MLAG_OWNER_INTERFACE) - stream_put(s, msg->intf_name, INTERFACE_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->intf_name, + INTERFACE_NAMSIZ); else stream_put(s, NULL, INTERFACE_NAMSIZ); zebra_mlag_mroute_add__free_unpacked(msg, NULL); @@ -1032,15 +1065,15 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, /* No Batching */ stream_putw(s, MLAG_MSG_NO_BATCH); /* Actual Data */ - stream_put(s, msg->vrf_name, VRF_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->vrf_name, VRF_NAMSIZ); stream_putl(s, msg->source_ip); stream_putl(s, msg->group_ip); - stream_putl(s, msg->group_ip); stream_putl(s, msg->owner_id); stream_putl(s, msg->vrf_id); if (msg->owner_id == MLAG_OWNER_INTERFACE) - stream_put(s, msg->intf_name, INTERFACE_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->intf_name, + INTERFACE_NAMSIZ); else stream_put(s, NULL, INTERFACE_NAMSIZ); zebra_mlag_mroute_del__free_unpacked(msg, NULL); @@ -1067,7 +1100,8 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, msg = Bulk_msg->mroute_add[i]; - stream_put(s, msg->vrf_name, VRF_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->vrf_name, + VRF_NAMSIZ); stream_putl(s, msg->source_ip); stream_putl(s, msg->group_ip); stream_putl(s, msg->cost_to_rp); @@ -1076,8 +1110,9 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, stream_putc(s, msg->am_i_dual_active); stream_putl(s, msg->vrf_id); if (msg->owner_id == MLAG_OWNER_INTERFACE) - stream_put(s, msg->intf_name, - INTERFACE_NAMSIZ); + zebra_fill_protobuf_msg( + s, msg->intf_name, + INTERFACE_NAMSIZ); else stream_put(s, NULL, INTERFACE_NAMSIZ); } @@ -1106,14 +1141,16 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data, msg = Bulk_msg->mroute_del[i]; - stream_put(s, msg->vrf_name, VRF_NAMSIZ); + zebra_fill_protobuf_msg(s, msg->vrf_name, + VRF_NAMSIZ); stream_putl(s, msg->source_ip); stream_putl(s, msg->group_ip); stream_putl(s, msg->owner_id); stream_putl(s, msg->vrf_id); if (msg->owner_id == MLAG_OWNER_INTERFACE) - stream_put(s, msg->intf_name, - INTERFACE_NAMSIZ); + zebra_fill_protobuf_msg( + s, msg->intf_name, + INTERFACE_NAMSIZ); else stream_put(s, NULL, INTERFACE_NAMSIZ); } -- 2.39.5