This is an automated email from the ASF dual-hosted git repository. zwoop pushed a commit to branch 9.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 58b6e32895f6a0342201e509149d0a3c7af1c10c Author: Randall Meyer <[email protected]> AuthorDate: Tue Feb 2 18:42:25 2021 -0800 drop use of BIO_f_base64 and EVP_PKEY_new_mac_key (#7106) BIO_f_base64 is not available in BoringSSL plus we have our own Base64 methods. With this change, the estimation values change, but shouldn't effect actual runtime. And BoringSSL suggests using the HMAC* functions in place of EVP_PKEY_new_mac_key --- plugins/experimental/access_control/Makefile.inc | 4 +- .../experimental/access_control/access_control.cc | 6 +- plugins/experimental/access_control/headers.cc | 4 +- .../access_control/unit_tests/test_utils.cc | 48 +++--- plugins/experimental/access_control/utils.cc | 172 ++++++--------------- plugins/experimental/access_control/utils.h | 2 - 6 files changed, 83 insertions(+), 153 deletions(-) diff --git a/plugins/experimental/access_control/Makefile.inc b/plugins/experimental/access_control/Makefile.inc index 3fefd4f..92d799f 100644 --- a/plugins/experimental/access_control/Makefile.inc +++ b/plugins/experimental/access_control/Makefile.inc @@ -28,11 +28,11 @@ experimental_access_control_access_control_la_SOURCES = \ check_PROGRAMS += experimental/access_control/test_access_control experimental_access_control_test_access_control_CPPFLAGS = $(AM_CPPFLAGS) -I$(abs_top_srcdir)/tests/include -DACCESS_CONTROL_UNIT_TEST -experimental_access_control_test_access_control_LDADD = $(OPENSSL_LIBS) +experimental_access_control_test_access_control_LDADD = $(OPENSSL_LIBS) $(top_builddir)/src/tscore/libtscore.la + experimental_access_control_test_access_control_SOURCES = \ experimental/access_control/unit_tests/test_access_control.cc \ experimental/access_control/unit_tests/test_utils.cc \ experimental/access_control/access_control.cc \ experimental/access_control/common.cc \ experimental/access_control/utils.cc - diff --git a/plugins/experimental/access_control/access_control.cc b/plugins/experimental/access_control/access_control.cc index 3f71c0f..9cb9495 100644 --- a/plugins/experimental/access_control/access_control.cc +++ b/plugins/experimental/access_control/access_control.cc @@ -358,7 +358,7 @@ static const std::map<String, String> _digestAlgosMap = createStaticDigestAlgoMa * @param hf Hash Function (HF) [optional] * @param secret secret * @param message input message - * @param messageLen input message lenght + * @param messageLen input message length * @param buffer output buffer for storing the message digest * @param len output buffer length * @return number of characters actually written to the output buffer. @@ -437,7 +437,7 @@ accessTokenStatusToString(const AccessTokenStatus &state) s = "INVALID_FIELD_VALUE"; break; case INVALID_VERSION: - s = "UNSUPORTED_VERSION"; + s = "UNSUPPORTED_VERSION"; break; case INVALID_SECRET: s = "NO_SECRET_SPECIFIED"; @@ -461,7 +461,7 @@ accessTokenStatusToString(const AccessTokenStatus &state) s = "INVALID_KEYID"; break; case INVALID_HASH_FUNCTION: - s = "UNSUPORTED_HASH_FUNCTION"; + s = "UNSUPPORTED_HASH_FUNCTION"; break; default: s = ""; diff --git a/plugins/experimental/access_control/headers.cc b/plugins/experimental/access_control/headers.cc index ddc64e4..a22e0ff 100644 --- a/plugins/experimental/access_control/headers.cc +++ b/plugins/experimental/access_control/headers.cc @@ -80,7 +80,7 @@ headerExist(TSMBuffer bufp, TSMLoc hdrLoc, const char *header, int headerlen) * @param header header name * @param headerlen header name length * @param value buffer for the value - * @param valuelen lenght of the buffer for the value + * @param valuelen length of the buffer for the value * @return pointer to the string with the value. */ char * @@ -129,7 +129,7 @@ getHeader(TSMBuffer bufp, TSMLoc hdrLoc, const char *header, int headerlen, char * @param header header name * @param headerlen header name len * @param value the new value - * @param valuelen lenght of the value + * @param valuelen length of the value * @return true - OK, false - failed */ bool diff --git a/plugins/experimental/access_control/unit_tests/test_utils.cc b/plugins/experimental/access_control/unit_tests/test_utils.cc index 9cc69aa..bbad959 100644 --- a/plugins/experimental/access_control/unit_tests/test_utils.cc +++ b/plugins/experimental/access_control/unit_tests/test_utils.cc @@ -26,34 +26,33 @@ #include "../common.h" /********************************************************************************************************************* - * Base64 related test - * @note the purpose of these test is not to test Base64 itself since it is provided by openssl library, - * the idea is to test the usage and some corner cases. + * Base64 related tests + * @note the purpose of these tests is to test the usage and some corner cases. ********************************************************************************************************************/ TEST_CASE("Base64: estimate buffer size needed to encode a message", "[Base64][access_control][utility]") { size_t encodedLen; - /* Test with a zero decoded message lenght */ + /* Test with a zero decoded message length */ encodedLen = cryptoBase64EncodedSize(0); - CHECK(0 == encodedLen); + CHECK(4 == encodedLen); /* Test with a random non-zero decoded message length */ encodedLen = cryptoBase64EncodedSize(64); - CHECK(88 == encodedLen); + CHECK(89 == encodedLen); /* Test the space for padding. Size of encoding that would result in 2 x "=" padding */ encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3abc")); - CHECK(88 == encodedLen); + CHECK(92 == encodedLen); /* Test the space for padding. Size of encoding that would result in 1 x "=" padding */ encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3ab")); - CHECK(88 == encodedLen); + CHECK(90 == encodedLen); /* Test the space for padding. Size of encoding that would result in no padding */ encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3a")); - CHECK(88 == encodedLen); + CHECK(89 == encodedLen); } TEST_CASE("Base64: estimate buffer size needed to decode a message", "[Base64][access_control][utility]") @@ -64,24 +63,24 @@ TEST_CASE("Base64: estimate buffer size needed to decode a message", "[Base64][a /* Padding with 2 x '=' */ encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYQ=="; encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded)); - CHECK(64 == encodedLen); + CHECK(66 == encodedLen); /* Padding with 1 x '=' */ encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYWI="; encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded)); - CHECK(65 == encodedLen); + CHECK(66 == encodedLen); /* Padding with 0 x "=" */ encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYWJj"; encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded)); CHECK(66 == encodedLen); - /* Test empty encoded message caclulation */ + /* Test empty encoded message calculation */ encoded = ""; encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded)); CHECK(0 == encodedLen); - /* Test empty encoded message caclulation */ + /* Test empty encoded message calculation */ encoded = nullptr; encodedLen = cryptoBase64DecodeSize(encoded, 0); CHECK(0 == encodedLen); @@ -94,16 +93,17 @@ TEST_CASE("Base64: quick encode / decode", "[Base64][access_control][utility]") CHECK(64 == messageLen); size_t encodedMessageEstimatedLen = cryptoBase64EncodedSize(messageLen); - CHECK(88 == encodedMessageEstimatedLen); + CHECK(89 == encodedMessageEstimatedLen); char encodedMessage[encodedMessageEstimatedLen]; + // now encode message into encodedMessage size_t encodedMessageLen = cryptoBase64Encode(message, messageLen, encodedMessage, encodedMessageEstimatedLen); CHECK(88 == encodedMessageLen); CHECK(0 == strncmp(encodedMessage, "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYQ==", encodedMessageLen)); size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen); - CHECK(64 == decodedMessageEstimatedLen); + CHECK(66 == decodedMessageEstimatedLen); char decodedMessage[encodedMessageEstimatedLen]; size_t decodedMessageLen = cryptoBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, encodedMessageLen); @@ -180,8 +180,9 @@ TEST_CASE("Base64: quick encode / decode with '+', '/' and various paddings", "[ CHECK(0 == strncmp(encodedMessage, encoded[i], encodedMessageLen)); /* Decode */ - size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen); - CHECK(strlen(decoded[i]) == decodedMessageEstimatedLen); + // Keep test around incase our implementation's estimation gets better + // size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen); + // CHECK(strlen(decoded[i]) == decodedMessageEstimatedLen); char decodedMessage[encodedMessageEstimatedLen]; size_t decodedMessageLen = cryptoBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, encodedMessageLen); CHECK(strlen(decoded[i]) == decodedMessageLen); @@ -191,7 +192,6 @@ TEST_CASE("Base64: quick encode / decode with '+', '/' and various paddings", "[ /********************************************************************************************************************* * Modified Base64 related test - * @note since Modified Base64 function use Base64 which use openssl the idea is to test the modification itself * (for more info see the comment in the implementation) + some corner cases. ********************************************************************************************************************/ @@ -215,6 +215,9 @@ TEST_CASE("Base64: modified encode / decode with '+', '/' and various paddings", char decodedMessage[encodedMessageEstimatedLen]; size_t decodedMessageLen = cryptoModifiedBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, decodedMessageEstimatedLen); + CAPTURE(i); + CAPTURE(decoded[i]); + CAPTURE(std::string(decodedMessage)); CHECK(strlen(decoded[i]) == decodedMessageLen); CHECK(0 == strncmp(decodedMessage, message, messageLen)); } @@ -222,7 +225,6 @@ TEST_CASE("Base64: modified encode / decode with '+', '/' and various paddings", /********************************************************************************************************************* * Digest calculation related test - * @note since Modified Base64 function use Base64 which use openssl the idea is to test the modification itself * (for more info see the comment in the implementation) + some corner cases. ********************************************************************************************************************/ @@ -236,10 +238,9 @@ TEST_CASE("HMAC Digest: test various supported/unsupported types", "[MAC][access char out[MAX_MSGDIGEST_BUFFER_SIZE]; char hexOut[MAX_MSGDIGEST_BUFFER_SIZE]; - StringList types = {"MD4", "MD5", "RIPEMD160", "SHA1", "SHA224", "SHA256", "SHA384", "SHA512"}; + StringList types = {"MD4", "MD5", "SHA1", "SHA224", "SHA256", "SHA384", "SHA512"}; StringList digests = {"6b3057137a6e17613883ac25a628b1b3", "820117c62fa161804efb3743cc838b81", - "ccf3230972bcf229fb3b16741495c74a72bbdd14", "0e3dfdfb04a3dfcd4d195cb1a5e4186feab2e0c1", "00a6f43962e2b35cb2491f81d59ef2268309c8cde744891188c9b855", "149333e1db61f9a18a91a13aca0370b89cec4c546360b85530ae2da97b7b1cb9", @@ -247,6 +248,11 @@ TEST_CASE("HMAC Digest: test various supported/unsupported types", "[MAC][access "e075c8b0637bc4fb82cdca66a2b72e3c1734f4f78c803e5db7ca879f85f16b2e057fa62bdd09eef5bbea562990d52a671927033056" "314c19092263f753ecd019"}; +#ifndef OPENSSL_IS_BORINGSSL // RIPEMD160 is not available on BoringSSL? + types.push_back("RIPEMD160"); + digests.push_back("ccf3230972bcf229fb3b16741495c74a72bbdd14"); +#endif + StringList::iterator digestIter = digests.begin(); for (String digestType : types) { size_t outLen = cryptoMessageDigestGet(digestType.c_str(), data.c_str(), data.length(), key.c_str(), key.length(), out, diff --git a/plugins/experimental/access_control/utils.cc b/plugins/experimental/access_control/utils.cc index 6476d56..cd212d1 100644 --- a/plugins/experimental/access_control/utils.cc +++ b/plugins/experimental/access_control/utils.cc @@ -22,42 +22,15 @@ * @see utils.h */ -#include <cerrno> /* errno */ -#include <climits> /* LONG_MIN, LONG_MAX */ -#include <openssl/bio.h> /* BIO I/O abstraction */ -#include <openssl/buffer.h> /* buf_mem_st */ -#include <openssl/err.h> /* ERR_get_error() and ERR_error_string_n() */ +#include <cerrno> /* errno */ +#include <openssl/err.h> /* ERR_get_error() and ERR_error_string_n() */ +#include <openssl/hmac.h> #include <openssl/crypto.h> #include "common.h" #include "utils.h" - -/** - * @brief Parse a counted string containing a long integer - * - * @param s ptr to the counted string - * @param lenght character count - * @param val where the long integer to be stored - * @return true - success, false - failed to parse - */ -bool -parseStrLong(const char *s, size_t length, long &val) -{ - // Make an extra copy since strtol expects NULL-terminated strings. - char str[length + 1]; - strncpy(str, s, length); - str[length] = 0; - - errno = 0; - char *temp; - val = strtol(str, &temp, 0); - - if (temp == str || *temp != '\0' || ((val == LONG_MIN || val == LONG_MAX) && errno == ERANGE)) { - AccessControlError("Could not convert '%s' to a long integer and leftover string is: '%s'", str, temp); - return false; - } - return true; -} +#include "tscore/ink_base64.h" +#include "ink_autoconf.h" /* ******* Encoding/Decoding functions ******* */ @@ -247,54 +220,48 @@ size_t cryptoMessageDigestGet(const char *digestType, const char *data, size_t dataLen, const char *key, size_t keyLen, char *out, size_t outLen) { - EVP_MD_CTX *ctx = nullptr; +#ifndef HAVE_HMAC_CTX_NEW + HMAC_CTX ctx[1]; +#else + HMAC_CTX *ctx; +#endif + const EVP_MD *md = nullptr; - EVP_PKEY *pkey = nullptr; - size_t len = outLen; + unsigned int len = 0; char buffer[256]; - size_t result = 0; - if (!(ctx = EVP_MD_CTX_create())) { - AccessControlError("failed to create EVP message digest context: %s", cryptoErrStr(buffer, sizeof(buffer))); - } else { -#ifndef OPENSSL_IS_BORINGSSL - if (!(pkey = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, nullptr, reinterpret_cast<const unsigned char *>(key), keyLen))) { + if (!(md = EVP_get_digestbyname(digestType))) { + AccessControlError("unknown digest name '%s'", digestType); + return 0; + } + +#ifndef HAVE_HMAC_CTX_NEW + HMAC_CTX_init(ctx); #else - if (false) { + ctx = HMAC_CTX_new(); #endif - AccessControlError("failed to create EVP private key. %s", cryptoErrStr(buffer, sizeof(buffer))); - EVP_MD_CTX_destroy(ctx); - } else { - do { - if (!(md = EVP_get_digestbyname(digestType))) { - AccessControlError("failed to get digest by name %s. %s", digestType, cryptoErrStr(buffer, sizeof(buffer))); - break; - } - - if (1 != EVP_DigestSignInit(ctx, nullptr, md, nullptr, pkey)) { - AccessControlError("failed to set up signing context. %s", cryptoErrStr(buffer, sizeof(buffer))); - break; - } - - if (1 != EVP_DigestSignUpdate(ctx, data, dataLen)) { - AccessControlError("failed to update the signing hash. %s", cryptoErrStr(buffer, sizeof(buffer))); - break; - } - - if (1 != EVP_DigestSignFinal(ctx, reinterpret_cast<unsigned char *>(out), &len)) { - AccessControlError("failed to finalize the signing hash. %s", cryptoErrStr(buffer, sizeof(buffer))); - } - - /* success */ - result = len; - } while (false); - - EVP_PKEY_free(pkey); - EVP_MD_CTX_destroy(ctx); - } + if (!HMAC_Init_ex(ctx, key, keyLen, md, NULL)) { + AccessControlError("failed to create EVP message digest context: %s", cryptoErrStr(buffer, sizeof(buffer))); + goto err; } - return result; + if (!HMAC_Update(ctx, reinterpret_cast<const unsigned char *>(data), dataLen)) { + AccessControlError("failed to update the signing hash: %s", cryptoErrStr(buffer, sizeof(buffer))); + goto err; + } + + if (!HMAC_Final(ctx, reinterpret_cast<unsigned char *>(out), &len)) { + AccessControlError("failed to finalize the signing hash: %s", cryptoErrStr(buffer, sizeof(buffer))); + goto err; + } + +err: +#ifndef HAVE_HMAC_CTX_NEW + HMAC_CTX_cleanup(ctx); +#else + HMAC_CTX_free(ctx); +#endif + return len; } /** @@ -329,7 +296,7 @@ cryptoMessageDigestEqual(const char *md1, size_t md1Len, const char *md2, size_t size_t cryptoBase64EncodedSize(size_t decodedSize) { - return (((4 * decodedSize) / 3) + 3) & ~3; + return ATS_BASE64_ENCODE_DSTLEN(decodedSize); } /** @@ -341,22 +308,7 @@ cryptoBase64EncodedSize(size_t decodedSize) size_t cryptoBase64DecodeSize(const char *encoded, size_t encodedLen) { - if (nullptr == encoded || 0 == encodedLen) { - return 0; - } - - size_t padding = 0; - const char *end = encoded + encodedLen; - - if ('=' == *(--end)) { - padding++; - } - - if ('=' == *(--end)) { - padding++; - } - - return (3 * encodedLen) / 4 - padding; + return ATS_BASE64_DECODE_DSTLEN(encodedLen); } /** @@ -375,27 +327,11 @@ cryptoBase64Encode(const char *in, size_t inLen, char *out, size_t outLen) return 0; } - BIO *head, *b64, *bmem; - BUF_MEM *bptr; - size_t len = 0; - - head = b64 = BIO_new(BIO_f_base64()); - if (nullptr != b64) { - BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL); - bmem = BIO_new(BIO_s_mem()); - if (nullptr != bmem) { - head = BIO_push(b64, bmem); - - BIO_write(head, in, inLen); - (void)BIO_flush(head); - - BIO_get_mem_ptr(head, &bptr); - len = bptr->length < outLen ? bptr->length : outLen; - strncpy(out, bptr->data, len); - } - BIO_free_all(head); + if (!ats_base64_encode(in, inLen, out, outLen, &outLen)) { + return 0; } - return len; + + return outLen; } /** @@ -414,21 +350,11 @@ cryptoBase64Decode(const char *in, size_t inLen, char *out, size_t outLen) return 0; } - BIO *head, *bmem, *b64; - size_t len = 0; - - head = b64 = BIO_new(BIO_f_base64()); - if (nullptr != b64) { - BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL); - bmem = BIO_new_mem_buf((void *)in, inLen); - if (nullptr != bmem) { - head = BIO_push(b64, bmem); - len = BIO_read(head, out, outLen); - } - BIO_free_all(head); + if (!ats_base64_decode(in, inLen, reinterpret_cast<unsigned char *>(out), outLen, &outLen)) { + return 0; } - return len; + return outLen; } /** diff --git a/plugins/experimental/access_control/utils.h b/plugins/experimental/access_control/utils.h index 6c821df..ebdb5f2 100644 --- a/plugins/experimental/access_control/utils.h +++ b/plugins/experimental/access_control/utils.h @@ -30,8 +30,6 @@ #define MAX_MSGDIGEST_BUFFER_SIZE EVP_MAX_MD_SIZE -bool parseStrLong(const char *s, size_t len, long &val); - /* ******* Encoding/Decoding functions ******* */ size_t hexEncode(const char *in, size_t inLen, char *out, size_t outLen);
