From 9a0da36b71c7c95fcb692cb556c89802717c2d80 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 8 Dec 2021 02:26:52 +0000 Subject: [PATCH] TSUserArg: add value type checking A plugin may have multiple user argument types. For instance, it may have both TS_USER_ARGS_SSN and TS_USER_ARGS_TXN plugin data. Due to a coding error, it is possible that it may accidentally use the wrong index type while retrieving one or the other user data types. For instance, if it uses its transaction index to retrieve its session data, this may result in them getting the transaction data for another plugin. In these situations, the plugin will either read what looks like garbage data to it (because it is casting incorrect memory to its data type), or it may write to that data and thus corrupt the other plugin's data, or both. This change updates the core to segment off the plugin user data ids by category. This allows the core to verify that the plugin passes the correct id type per the data type (session, transaction, etc.) and fails an assertion if there is a mismatch. --- include/tscore/PluginUserArgs.h | 39 ++++++++++++++ src/traffic_server/InkAPI.cc | 18 ++++--- src/tscore/Makefile.am | 1 + src/tscore/unit_tests/test_PluginUserArgs.cc | 56 ++++++++++++++++++++ 4 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 src/tscore/unit_tests/test_PluginUserArgs.cc diff --git a/include/tscore/PluginUserArgs.h b/include/tscore/PluginUserArgs.h index 057e37c1b3a..4dd637b2158 100644 --- a/include/tscore/PluginUserArgs.h +++ b/include/tscore/PluginUserArgs.h @@ -35,6 +35,41 @@ static constexpr std::array MAX_USER_ARGS = {{ 128 /* max number of user arguments for GLB */ }}; +/** Stagger each user argument value so we can detect mismatched + * indices. + * + * For example, say a plugin associates data with both sessions and + * transactions and that its session index is 2 and its transaction index is 4. + * In this case, we'll hand back to the plugin 2002 for its session index and + * 1004 for its transaction index. If it then accidentally uses its session + * index to reference its transaction index, it will pass back 2002 instead of + * 1004, which we will identify as belonging to the wrong user argument type + * because it is in the 2000 session block rather than the expected 1000 + * transaction block. + * + * Note that these higher value 1000 block indices are only used when + * interfacing with the plugin. Internally the lower valued index is used. + * That is, for a transaction, expect internally a value of 3 instead of 1003. + */ +static constexpr size_t +get_user_arg_offset(TSUserArgType type) +{ + // TS_USER_ARGS_TXN indices begin at 1000, TS_USER_ARGS_SSN begin at 2000, + // etc. + return (static_cast(type) + 1) * 1000; +} + +/** Verify that the user passed in an index whose value corresponds with the + * type. See the comment above the declaration of get_user_arg_offset for the + * intention behind this. + */ +static constexpr inline bool +SanityCheckUserIndex(TSUserArgType type, int idx) +{ + int const block_start = get_user_arg_offset(type); + return idx >= block_start && idx < block_start + 1000; +} + /** This is a mixin class (sort of), implementing the appropriate APIs and data storage for a particular user arg table. Used by VConn / Ssn / Txn user arg data. @@ -53,6 +88,8 @@ template class PluginUserArgs : public virtual PluginUserArgsM void * get_user_arg(size_t ix) const { + ink_release_assert(SanityCheckUserIndex(I, ix)); + ix -= get_user_arg_offset(I); ink_release_assert(ix < user_args.size()); return this->user_args[ix]; }; @@ -60,6 +97,8 @@ template class PluginUserArgs : public virtual PluginUserArgsM void set_user_arg(size_t ix, void *arg) { + ink_release_assert(SanityCheckUserIndex(I, ix)); + ix -= get_user_arg_offset(I); ink_release_assert(ix < user_args.size()); user_args[ix] = arg; }; diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index f43796cad4f..0235ac54379 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -6312,6 +6312,9 @@ TSUserArgIndexReserve(TSUserArgType type, const char *name, const char *descript if (TS_SUCCESS == TSUserArgIndexNameLookup(type, name, &idx, &desc)) { // Found existing index. + + // No need to add get_user_arg_offset(type) here since + // TSUserArgIndexNameLookup already does so. *ptr_idx = idx; return TS_SUCCESS; } @@ -6325,7 +6328,7 @@ TSUserArgIndexReserve(TSUserArgType type, const char *name, const char *descript if (description) { arg.description = description; } - *ptr_idx = idx; + *ptr_idx = idx + get_user_arg_offset(type); return TS_SUCCESS; } @@ -6336,6 +6339,8 @@ TSReturnCode TSUserArgIndexLookup(TSUserArgType type, int idx, const char **name, const char **description) { sdk_assert(0 <= type && type < TS_USER_ARGS_COUNT); + sdk_assert(SanityCheckUserIndex(type, idx)); + idx -= get_user_arg_offset(type); if (sdk_sanity_check_null_ptr(name) == TS_SUCCESS) { if (idx < UserArgIdx[type]) { UserArg &arg(UserArgTable[type][idx]); @@ -6363,7 +6368,7 @@ TSUserArgIndexNameLookup(TSUserArgType type, const char *name, int *arg_idx, con if (description) { *description = arg->description.c_str(); } - *arg_idx = arg - UserArgTable[type]; + *arg_idx = arg - UserArgTable[type] + get_user_arg_offset(type); return TS_SUCCESS; } } @@ -6398,6 +6403,7 @@ TSUserArgGet(void *data, int arg_idx) } // ------------- +/* These are deprecated as of v9.0.0, and will be removed in v10.0.0 */ TSReturnCode TSHttpTxnArgIndexReserve(const char *name, const char *description, int *arg_idx) { @@ -6456,7 +6462,6 @@ void TSHttpTxnArgSet(TSHttpTxn txnp, int arg_idx, void *arg) { sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_TXN]); HttpSM *sm = reinterpret_cast(txnp); @@ -6467,7 +6472,6 @@ void * TSHttpTxnArgGet(TSHttpTxn txnp, int arg_idx) { sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_TXN]); HttpSM *sm = reinterpret_cast(txnp); return sm->get_user_arg(arg_idx); @@ -6477,7 +6481,6 @@ void TSHttpSsnArgSet(TSHttpSsn ssnp, int arg_idx, void *arg) { sdk_assert(sdk_sanity_check_http_ssn(ssnp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_SSN]); ProxySession *cs = reinterpret_cast(ssnp); @@ -6488,7 +6491,6 @@ void * TSHttpSsnArgGet(TSHttpSsn ssnp, int arg_idx) { sdk_assert(sdk_sanity_check_http_ssn(ssnp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_SSN]); ProxySession *cs = reinterpret_cast(ssnp); return cs->get_user_arg(arg_idx); @@ -6498,7 +6500,6 @@ void TSVConnArgSet(TSVConn connp, int arg_idx, void *arg) { sdk_assert(sdk_sanity_check_iocore_structure(connp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_VCONN]); PluginUserArgsMixin *user_args = dynamic_cast(reinterpret_cast(connp)); sdk_assert(user_args); @@ -6509,13 +6510,14 @@ void * TSVConnArgGet(TSVConn connp, int arg_idx) { sdk_assert(sdk_sanity_check_iocore_structure(connp) == TS_SUCCESS); - sdk_assert(arg_idx >= 0 && static_cast(arg_idx) < MAX_USER_ARGS[TS_USER_ARGS_VCONN]); PluginUserArgsMixin *user_args = dynamic_cast(reinterpret_cast(connp)); sdk_assert(user_args); return user_args->get_user_arg(arg_idx); } +/* End deprecated Arg functions. */ + void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status) { diff --git a/src/tscore/Makefile.am b/src/tscore/Makefile.am index c0ca76c670e..8861b86afbd 100644 --- a/src/tscore/Makefile.am +++ b/src/tscore/Makefile.am @@ -183,6 +183,7 @@ test_tscore_SOURCES = \ unit_tests/test_MemArena.cc \ unit_tests/test_MT_hashtable.cc \ unit_tests/test_ParseRules.cc \ + unit_tests/test_PluginUserArgs.cc \ unit_tests/test_PriorityQueue.cc \ unit_tests/test_Ptr.cc \ unit_tests/test_Regex.cc \ diff --git a/src/tscore/unit_tests/test_PluginUserArgs.cc b/src/tscore/unit_tests/test_PluginUserArgs.cc new file mode 100644 index 00000000000..72dc7eec711 --- /dev/null +++ b/src/tscore/unit_tests/test_PluginUserArgs.cc @@ -0,0 +1,56 @@ +/** + @file Test for Regex.cc + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#include "tscore/PluginUserArgs.h" +#include "catch.hpp" + +TEST_CASE("get_user_arg_offset", "[libts][PluginUserArgs]") +{ + CHECK(get_user_arg_offset(TS_USER_ARGS_TXN) == 1000); + CHECK(get_user_arg_offset(TS_USER_ARGS_SSN) == 2000); + CHECK(get_user_arg_offset(TS_USER_ARGS_VCONN) == 3000); + CHECK(get_user_arg_offset(TS_USER_ARGS_GLB) == 4000); +} + +TEST_CASE("SanityCheckUserIndex", "[libts][PluginUserArgs]") +{ + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_TXN, 0)); + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_TXN, 1)); + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_TXN, 999)); + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_TXN, 2000)); + + CHECK(SanityCheckUserIndex(TS_USER_ARGS_TXN, 1000)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_TXN, 1001)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_TXN, 1999)); + + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_SSN, 1000)); + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_SSN, 3000)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_SSN, 2000)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_SSN, 2001)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_SSN, 2999)); + + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_VCONN, 2000)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_VCONN, 3000)); + + CHECK_FALSE(SanityCheckUserIndex(TS_USER_ARGS_GLB, 3000)); + CHECK(SanityCheckUserIndex(TS_USER_ARGS_GLB, 4000)); +}