diff --git a/dev_utils/README.md b/dev_utils/README.md index 12ba859..c1ae788 100644 --- a/dev_utils/README.md +++ b/dev_utils/README.md @@ -51,10 +51,9 @@ mc admin trace -v proxydev Run the go proxy from the root directory ```bash -export GO111MODULE=on export SERVER_CONFFILE=dev_utils/config.yaml -go build main.go -./main +go build . +./S3-Upload-Proxy ``` diff --git a/helper/helper.go b/helper/helper.go index 579949b..2c2ad5f 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -8,11 +8,14 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "fmt" "log" "math/big" "net" "os" "path/filepath" + "regexp" + "strings" "time" "github.com/golang-jwt/jwt/v4" @@ -363,3 +366,27 @@ func TLScertToFile(filename string, derBytes []byte) error { return err } + +// FormatUploadFilePath ensures that path separators are "/", and returns error if the +// filepath contains a disallowed character matched with regex +func FormatUploadFilePath(filePath string) (string, error) { + + // Check for mixed "\" and "/" in filepath. Stop and throw an error if true so that + // we do not end up with unintended folder structure when applying ReplaceAll below + if strings.Contains(filePath, "\\") && strings.Contains(filePath, "/") { + return filePath, fmt.Errorf("filepath contains mixed '\\' and '/' characters") + } + + // make any windows path separators linux compatible + outPath := strings.ReplaceAll(filePath, "\\", "/") + + // [\x00-\x1F\x7F] is the control character set + re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) + + dissallowedChars := re.FindAllString(outPath, -1) + if dissallowedChars != nil { + return outPath, fmt.Errorf("filepath contains disallowed characters: %+v", strings.Join(dissallowedChars, ", ")) + } + + return outPath, nil +} diff --git a/helper/helper_test.go b/helper/helper_test.go index 358ffcb..94826d7 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -70,3 +70,23 @@ func TestCreateECToken(t *testing.T) { defer os.RemoveAll("dummy-folder") } + +func TestFormatUploadFilePath(t *testing.T) { + + unixPath := "a/b/c.c4gh" + testPath := "a\\b\\c.c4gh" + uploadPath, err := FormatUploadFilePath(testPath) + assert.NoError(t, err) + assert.Equal(t, unixPath, uploadPath) + + // mixed "\" and "/" + weirdPath := `dq\sw:*?"<>|\t\s/df.c4gh` + _, err = FormatUploadFilePath(weirdPath) + assert.EqualError(t, err, "filepath contains mixed '\\' and '/' characters") + + // no mixed "\" and "/" but not allowed + weirdPath = `dq\sw:*?"<>|\t\sdf.c4gh` + _, err = FormatUploadFilePath(weirdPath) + assert.EqualError(t, err, "filepath contains disallowed characters: :, *, ?, \", <, >, |") + +} diff --git a/proxy.go b/proxy.go index 327e701..83f1620 100644 --- a/proxy.go +++ b/proxy.go @@ -14,6 +14,7 @@ import ( "strings" "time" + "github.com/NBISweden/S3-Upload-Proxy/helper" common "github.com/neicnordic/sda-common/database" "github.com/aws/aws-sdk-go/aws" @@ -77,19 +78,18 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func (p *Proxy) internalServerError(w http.ResponseWriter, r *http.Request) { - log.Debug("internal server error") log.Debugf("Internal server error for request (%v)", r) - w.WriteHeader(500) + w.WriteHeader(http.StatusInternalServerError) } func (p *Proxy) notAllowedResponse(w http.ResponseWriter, _ *http.Request) { log.Debug("not allowed response") - w.WriteHeader(403) + w.WriteHeader(http.StatusForbidden) } func (p *Proxy) notAuthorized(w http.ResponseWriter, _ *http.Request) { log.Debug("not authorized") - w.WriteHeader(401) // Actually correct! + w.WriteHeader(http.StatusUnauthorized) } func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) { @@ -105,7 +105,16 @@ func (p *Proxy) allowedResponse(w http.ResponseWriter, r *http.Request) { p.prependBucketToHostPath(r) username := fmt.Sprintf("%v", claims["sub"]) - filepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1) + rawFilepath := strings.Replace(r.URL.Path, "/"+p.s3.bucket+"/", "", 1) + + filepath, err := helper.FormatUploadFilePath(rawFilepath) + if err != nil { + log.Debugf(err.Error()) + w.WriteHeader(http.StatusNotAcceptable) + + return + } + // register file in database if it's the start of an upload if p.detectRequestType(r) == Put && p.fileIds[r.URL.Path] == "" { log.Debugf("registering file %v in the database", r.URL.Path) @@ -409,7 +418,6 @@ func (p *Proxy) requestInfo(fullPath string) (string, int64, error) { return "", 0, err } - fmt.Println(strings.ReplaceAll(*result.Contents[0].ETag, "\"", "")) return fmt.Sprintf("%x", sha256.Sum256([]byte(strings.ReplaceAll(*result.Contents[0].ETag, "\"", "")))), *result.Contents[0].Size, nil diff --git a/proxy_test.go b/proxy_test.go index 71d6433..a2dc732 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -310,6 +310,16 @@ func (suite *ProxyTests) TestServeHTTP_allowed() { proxy.ServeHTTP(w, r) assert.Equal(suite.T(), 200, w.Result().StatusCode) assert.Equal(suite.T(), true, suite.fakeServer.PingedAndRestore()) + + // Filenames with platform incompatible characters are disallowed + // not checked in TestServeHTTP_allowed() because we look for a non 403 response + r.Method = "PUT" + r.URL, _ = url.Parse("/username/fi|le") + w = httptest.NewRecorder() + proxy.ServeHTTP(w, r) + assert.Equal(suite.T(), 406, w.Result().StatusCode) + assert.Equal(suite.T(), false, suite.fakeServer.PingedAndRestore()) + } func (suite *ProxyTests) TestMessageFormatting() {