trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gan...@apache.org
Subject [trafficserver] branch master updated: s3_auth:fixed uncaught exception on invalid input
Date Mon, 18 Feb 2019 22:46:23 GMT
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 <gancho@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);
 


Mime
View raw message