This is an automated email from the ASF dual-hosted git repository. gancho pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push: new 953cde6 s3_auth:fixed uncaught exception on invalid input 953cde6 is described below commit 953cde65b015e0f914fabdc79153fb9d694b18aa Author: Gancho Tenev <gan...@apache.org> AuthorDate: Sat Feb 16 09:32:07 2019 -0800 s3_auth:fixed uncaught exception on invalid input Instead of fixing the URI decoding functionality to handle the exception (re)implemented a check for percent encoding which was what was needed. During signature calculation AWS avoids URI encoding of already encoded query parameters (rfc3986#section-2.4 says "implementations must not percent-encode or decode the same string more than once ...") --- plugins/s3_auth/aws_auth_v4.cc | 63 ++++++++++++-------- plugins/s3_auth/unit_tests/test_aws_auth_v4.cc | 79 +++++++++++++++++++------- plugins/s3_auth/unit_tests/test_aws_auth_v4.h | 2 +- 3 files changed, 101 insertions(+), 43 deletions(-) diff --git a/plugins/s3_auth/aws_auth_v4.cc b/plugins/s3_auth/aws_auth_v4.cc index 61f595d..1df1a61 100644 --- a/plugins/s3_auth/aws_auth_v4.cc +++ b/plugins/s3_auth/aws_auth_v4.cc @@ -103,32 +103,52 @@ uriEncode(const String &in, bool isObjectName) } /** - * @brief URI-decode a character string (AWS specific version, see spec) + * @brief checks if the string is URI-encoded (AWS specific encoding version, see spec) * * @see AWS spec: http://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html * - * @todo Consider reusing / converting to TSStringPercentDecode() - * Currently we don't build a library/archive so we could link with the unit-test binary. Also using - * different sets of encode/decode functions during runtime and unit-testing did not seem as a good idea. - * @param in string to be URI decoded - * @return encoded string. + * @note According to the following RFC if the string is encoded and contains '%' it should + * be followed by 2 hexadecimal symbols otherwise '%' should be encoded with %25: + * https://tools.ietf.org/html/rfc3986#section-2.1 + * + * @param in string to be URI checked + * @param isObjectName if true encoding didn't encode '/', kept it as it is. + * @return true if encoded, false not encoded. */ -String -uriDecode(const String &in) +bool +isUriEncoded(const String &in, bool isObjectName) { - std::string result; - result.reserve(in.length()); - size_t i = 0; - while (i < in.length()) { - if (in[i] == '%') { - result += static_cast<char>(std::stoi(in.substr(i + 1, 2), nullptr, 16)); - i += 3; - } else { - result += in[i]; - i++; + for (size_t pos = 0; pos < in.length(); pos++) { + char c = in[pos]; + + if (isalnum(c) || c == '-' || c == '_' || c == '.' || c == '~') { + /* found a unreserved character which should not have been be encoded regardless + * 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'. */ + continue; + } + + if (' ' == c) { + /* space should have been encoded with %20 if the string was encoded */ + return false; + } + + if ('/' == c && !isObjectName) { + /* if this is not an object name '/' should have been encoded */ + return false; + } + + if ('%' == c) { + if (pos + 2 < in.length() && std::isxdigit(in[pos + 1]) && std::isxdigit(in[pos + 2])) { + /* if string was encoded we should have exactly 2 hexadecimal chars following it */ + return true; + } else { + /* lonely '%' should have been encoded with %25 according to the RFC so likely not encoded */ + return false; + } } } - return result; + + return false; } /** @@ -290,10 +310,7 @@ getCanonicalRequestSha256Hash(TsInterface &api, bool signPayload, const StringSe paramNames.insert(encodedParam); - /* Look for '%' first trying to avoid as many uri-decode calls as possible. - * it is hard to estimate which is more likely use-case - (1) URIs with uri-encoded query parameter - * values or (2) with unencoded which defines the success of this optimization */ - if (nullptr == memchr(value.c_str(), '%', value.length()) || 0 == uriDecode(value).compare(value)) { + if (!isUriEncoded(value, /* isObjectName */ false)) { /* Not URI-encoded */ paramsMap[encodedParam] = uriEncode(value, /* isObjectName */ false); } else { diff --git a/plugins/s3_auth/unit_tests/test_aws_auth_v4.cc b/plugins/s3_auth/unit_tests/test_aws_auth_v4.cc index 7dd4f60..025cc2b 100644 --- a/plugins/s3_auth/unit_tests/test_aws_auth_v4.cc +++ b/plugins/s3_auth/unit_tests/test_aws_auth_v4.cc @@ -69,37 +69,78 @@ TEST_CASE("uriEncode(): encode reserved chars in an object name", "[AWS][auth][u CHECK_FALSE(encoded.compare("%20/%21%22%23%24%25%26%27%28%29%2A%2B%2C%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%5E%60%7B%7C%7D")); } -TEST_CASE("uriDecode(): decode empty input", "[AWS][auth][utility]") +TEST_CASE("isUriEncoded(): check an empty input", "[AWS][auth][utility]") { - String encoded(""); - String decoded = uriDecode(encoded); - CHECK(0 == decoded.length()); /* 0 encoded because of the invalid input */ + CHECK(false == isUriEncoded("")); } -TEST_CASE("uriDecode(): decode unreserved chars", "[AWS][auth][utility]") +TEST_CASE("isUriEncoded(): '%' and nothing else", "[AWS][auth][utility]") +{ + CHECK(false == isUriEncoded("%")); +} + +TEST_CASE("isUriEncoded(): '%' but no hex digits", "[AWS][auth][utility]") +{ + CHECK(false == isUriEncoded("XXX%XXX")); +} + +TEST_CASE("isUriEncoded(): '%' but only one hex digit", "[AWS][auth][utility]") +{ + CHECK(false == isUriEncoded("XXXXX%1XXXXXX")); + CHECK(false == isUriEncoded("XXX%1")); // test end of string case +} + +TEST_CASE("isUriEncoded(): '%' and 2 hex digit", "[AWS][auth][utility]") +{ + CHECK(true == isUriEncoded("XXX%12XXX")); + CHECK(true == isUriEncoded("XXX%12")); // test end of string case +} + +TEST_CASE("isUriEncoded(): space not encoded", "[AWS][auth][utility]") +{ + // Having a space always means it was not encoded. + CHECK(false == isUriEncoded("XXXXX XXXXXX")); +} + +TEST_CASE("isUriEncoded(): '/' in strings which are not object names", "[AWS][auth][utility]") +{ + // This is not an object name so if we have '/' => the string was not encoded. + CHECK(false == isUriEncoded("XXXXX/XXXXXX", /* isObjectName */ false)); + + // There is no '/' and '%2F' shows that it was encoded. + CHECK(true == isUriEncoded("XXXXX%2FXXXXXX", /* isObjectName */ false)); + + // This is not an object name so if we have '/' => the string was not encoded despite '%20' in it. + CHECK(false == isUriEncoded("XXXXX/%20XXXXX", /* isObjectName */ false)); +} + +TEST_CASE("isUriEncoded(): '/' in strings that are object names", "[AWS][auth][utility]") +{ + // This is an object name so having '/' is normal but not enough to conclude if it is encoded or not. + CHECK(false == isUriEncoded("XXXXX/XXXXXX", /* isObjectName */ true)); + + // There is no '/' and '%2F' shows it is encoded. + CHECK(true == isUriEncoded("XXXXX%2FXXXXXX", /* isObjectName */ true)); + + // This is an object name so having '/' is normal and because of '%20' we can conclude it was encoded. + CHECK(true == isUriEncoded("XXXXX/%20XXXXX", /* isObjectName */ true)); +} + +TEST_CASE("isUriEncoded(): no reserved chars in the input", "[AWS][auth][utility]") { const String encoded = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" "0123456789" "-._~"; - String decoded = uriDecode(encoded); - - CHECK(encoded.length() == encoded.length()); - CHECK_FALSE(decoded.compare("ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "abcdefghijklmnopqrstuvwxyz" - "0123456789" - "-._~")); + CHECK(false == isUriEncoded(encoded)); } -TEST_CASE("uriDecode(): decode reserved chars", "[AWS][auth][utility]") +TEST_CASE("isUriEncoded(): reserved chars in the input", "[AWS][auth][utility]") { - const String encoded = - "%20%2F%21%22%23%24%25%26%27%28%29%2A%2B%2C%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%5E%60%7B%7C%7D"; /* some printable but - reserved chars */ - String decoded = uriDecode(encoded); + // some printable but reserved chars " /!\"#$%&'()*+,:;<=>?@[\\]^`{|}" + const String encoded = "%20%2F%21%22%23%24%25%26%27%28%29%2A%2B%2C%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%5E%60%7B%7C%7D"; - CHECK(3 * decoded.length() == encoded.length()); /* size of "%NN" = 3 */ - CHECK_FALSE(decoded.compare(" /!\"#$%&'()*+,:;<=>?@[\\]^`{|}")); + CHECK(true == isUriEncoded(encoded)); } /* base16Encode() ************************************************************************************************************** */ diff --git a/plugins/s3_auth/unit_tests/test_aws_auth_v4.h b/plugins/s3_auth/unit_tests/test_aws_auth_v4.h index 5b0ea30..1341517 100644 --- a/plugins/s3_auth/unit_tests/test_aws_auth_v4.h +++ b/plugins/s3_auth/unit_tests/test_aws_auth_v4.h @@ -121,7 +121,7 @@ public: /* Expose the following methods only to the unit tests */ String base16Encode(const char *in, size_t inLen); String uriEncode(const String &in, bool isObjectName = false); -String uriDecode(const String &in); +bool isUriEncoded(const String &in, bool isObjectName = false); String lowercase(const char *in, size_t inLen); const char *trimWhiteSpaces(const char *in, size_t inLen, size_t &newLen);