You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ga...@apache.org on 2019/02/18 22:46:23 UTC
[trafficserver] branch master updated: s3_auth:fixed uncaught
exception on invalid input
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 <ga...@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);