From d86405e79b04df920e2b196e521959f33649183c Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Tue, 15 Jun 2021 14:57:59 -0400
Subject: [PATCH] 17810: Fix S3 signature verification.

All chars (other than "/") that are not unreserved must be
percent-encoded.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 services/keep-web/s3.go      | 34 +++++++++++++++++++++++++++++-----
 services/keep-web/s3_test.go | 22 +++++++++++++++++-----
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index f03ff01b81..6ea9bf9f7a 100644
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -136,15 +136,39 @@ func s3stringToSign(alg, scope, signedHeaders string, r *http.Request) (string,
 		}
 	}
 
-	normalizedURL := *r.URL
-	normalizedURL.RawPath = ""
-	normalizedURL.Path = reMultipleSlashChars.ReplaceAllString(normalizedURL.Path, "/")
-	ctxlog.FromContext(r.Context()).Infof("escapedPath %s", normalizedURL.EscapedPath())
-	canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", r.Method, normalizedURL.EscapedPath(), s3querystring(r.URL), canonicalHeaders, signedHeaders, r.Header.Get("X-Amz-Content-Sha256"))
+	normalizedPath := normalizePath(r.URL.Path)
+	ctxlog.FromContext(r.Context()).Debugf("normalizedPath %q", normalizedPath)
+	canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", r.Method, normalizedPath, s3querystring(r.URL), canonicalHeaders, signedHeaders, r.Header.Get("X-Amz-Content-Sha256"))
 	ctxlog.FromContext(r.Context()).Debugf("s3stringToSign: canonicalRequest %s", canonicalRequest)
 	return fmt.Sprintf("%s\n%s\n%s\n%s", alg, r.Header.Get("X-Amz-Date"), scope, hashdigest(sha256.New(), canonicalRequest)), nil
 }
 
+func normalizePath(s string) string {
+	// (url.URL).EscapedPath() would be incorrect here. AWS
+	// documentation specifies the URL path should be normalized
+	// according to RFC 3986, i.e., unescaping ALPHA / DIGIT / "-"
+	// / "." / "_" / "~". The implication is that everything other
+	// than those chars (and "/") _must_ be percent-encoded --
+	// even chars like ";" and "," that are not normally
+	// percent-encoded in paths.
+	out := ""
+	for _, c := range []byte(reMultipleSlashChars.ReplaceAllString(s, "/")) {
+		if (c >= 'a' && c <= 'z') ||
+			(c >= 'A' && c <= 'Z') ||
+			(c >= '0' && c <= '9') ||
+			c == '-' ||
+			c == '.' ||
+			c == '_' ||
+			c == '~' ||
+			c == '/' {
+			out += string(c)
+		} else {
+			out += fmt.Sprintf("%%%02X", c)
+		}
+	}
+	return out
+}
+
 func s3signature(secretKey, scope, signedHeaders, stringToSign string) (string, error) {
 	// scope is {datestamp}/{region}/{service}/aws4_request
 	drs := strings.Split(scope, "/")
diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index 4f70168b56..d336c5f09f 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -558,12 +558,15 @@ func (s *IntegrationSuite) TestS3NormalizeURIForSignature(c *check.C) {
 		rawPath        string
 		normalizedPath string
 	}{
-		{"/foo", "/foo"},             // boring case
-		{"/foo%5fbar", "/foo_bar"},   // _ must not be escaped
-		{"/foo%2fbar", "/foo/bar"},   // / must not be escaped
-		{"/(foo)", "/%28foo%29"},     // () must be escaped
-		{"/foo%5bbar", "/foo%5Bbar"}, // %XX must be uppercase
+		{"/foo", "/foo"},                           // boring case
+		{"/foo%5fbar", "/foo_bar"},                 // _ must not be escaped
+		{"/foo%2fbar", "/foo/bar"},                 // / must not be escaped
+		{"/(foo)/[];,", "/%28foo%29/%5B%5D%3B%2C"}, // ()[];, must be escaped
+		{"/foo%5bbar", "/foo%5Bbar"},               // %XX must be uppercase
+		{"//foo///bar", "/foo/bar"},                // "//" and "///" must be squashed to "/"
 	} {
+		c.Logf("trial %q", trial)
+
 		date := time.Now().UTC().Format("20060102T150405Z")
 		scope := "20200202/zzzzz/S3/aws4_request"
 		canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", "GET", trial.normalizedPath, "", "host:host.example.com\n", "host", "")
@@ -1098,6 +1101,15 @@ func (s *IntegrationSuite) TestS3cmd(c *check.C) {
 	buf, err := cmd.CombinedOutput()
 	c.Check(err, check.IsNil)
 	c.Check(string(buf), check.Matches, `.* 3 +s3://`+arvadostest.FooCollection+`/foo\n`)
+
+	// This tests whether s3cmd's path normalization agrees with
+	// keep-web's signature verification wrt chars like "|"
+	// (neither reserved nor unreserved) and "," (not normally
+	// percent-encoded in a path).
+	cmd = exec.Command("s3cmd", "--no-ssl", "--host="+s.testServer.Addr, "--host-bucket="+s.testServer.Addr, "--access_key="+arvadostest.ActiveTokenUUID, "--secret_key="+arvadostest.ActiveToken, "get", "s3://"+arvadostest.FooCollection+"/foo,;$[|]bar")
+	buf, err = cmd.CombinedOutput()
+	c.Check(err, check.NotNil)
+	c.Check(string(buf), check.Matches, `(?ms).*NoSuchKey.*\n`)
 }
 
 func (s *IntegrationSuite) TestS3BucketInHost(c *check.C) {
-- 
2.30.2