From 324fe29346e16228d5dca349ea509254c9cd4ea5 Mon Sep 17 00:00:00 2001 From: Andrew Yourtchenko Date: Wed, 17 May 2017 23:43:59 +0200 Subject: [PATCH] acl-plugin: reject the too-short variable-length messages from clients (VPP-839) Prior to commit bfd9227e6da567e0e19e026afe94cd4c0b65f725, there was no clean way to check the lower-level message length as supplied by the client, so there was no option but to trust that the client does the right thing and allocates memory correctly. The absence of checks makes it hard for a misbehaving client to spot the problem - because everything "appears" to work correctly for the specific erroneous message exchange. This commit ensures the message received is at least as big as we expect, and complains loudly if it is not. Change-Id: I806eaac7c7f1ab3c64cb2bfa6939ce27da9a2b44 Signed-off-by: Andrew Yourtchenko --- src/plugins/acl/acl.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c index 4f3c54e842e..6b79411e771 100644 --- a/src/plugins/acl/acl.c +++ b/src/plugins/acl/acl.c @@ -1079,6 +1079,34 @@ macip_acl_interface_add_del_acl (u32 sw_if_index, u8 is_add, return rv; } +/* + * If the client does not allocate enough memory for a variable-length + * message, and then proceed to use it as if the full memory allocated, + * absent the check we happily consume that on the VPP side, and go + * along as if nothing happened. However, the resulting + * effects range from just garbage in the API decode + * (because the decoder snoops too far), to potential memory + * corruptions. + * + * This verifies that the actual length of the message is + * at least expected_len, and complains loudly if it is not. + * + * A failing check here is 100% a software bug on the API user side, + * so we might as well yell. + * + */ +static int verify_message_len(void *mp, u32 expected_len, char *where) +{ + u32 supplied_len = vl_msg_api_get_msg_length (mp); + if (supplied_len < expected_len) { + clib_warning("%s: Supplied message length %d is less than expected %d", + where, supplied_len, expected_len); + return 0; + } else { + return 1; + } +} + /* API message handler */ static void vl_api_acl_add_replace_t_handler (vl_api_acl_add_replace_t * mp) @@ -1087,8 +1115,14 @@ vl_api_acl_add_replace_t_handler (vl_api_acl_add_replace_t * mp) acl_main_t *am = &acl_main; int rv; u32 acl_list_index = ntohl (mp->acl_index); + u32 acl_count = ntohl (mp->count); + u32 expected_len = sizeof(*mp) + acl_count*sizeof(mp->r[0]); - rv = acl_add_list (ntohl (mp->count), mp->r, &acl_list_index, mp->tag); + if (verify_message_len(mp, expected_len, "acl_add_replace")) { + rv = acl_add_list (acl_count, mp->r, &acl_list_index, mp->tag); + } else { + rv = VNET_API_ERROR_INVALID_VALUE; + } /* *INDENT-OFF* */ REPLY_MACRO2(VL_API_ACL_ADD_REPLACE_REPLY, @@ -1344,9 +1378,14 @@ vl_api_macip_acl_add_t_handler (vl_api_macip_acl_add_t * mp) acl_main_t *am = &acl_main; int rv; u32 acl_list_index = ~0; + u32 acl_count = ntohl (mp->count); + u32 expected_len = sizeof(*mp) + acl_count*sizeof(mp->r[0]); - rv = - macip_acl_add_list (ntohl (mp->count), mp->r, &acl_list_index, mp->tag); + if (verify_message_len(mp, expected_len, "macip_acl_add")) { + rv = macip_acl_add_list (acl_count, mp->r, &acl_list_index, mp->tag); + } else { + rv = VNET_API_ERROR_INVALID_VALUE; + } /* *INDENT-OFF* */ REPLY_MACRO2(VL_API_MACIP_ACL_ADD_REPLY, -- 2.16.6