From c5fc504cb26be0c2e96083c0bad6c79d278e3afc Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 15 Aug 2016 17:38:55 -0400 Subject: PLT-3617 Switched public file links to use a sha256 hash (#3792) * Changed FileSettings.PublicLinkSalt to be a pointer * Switched public file links to use a sha256 hash --- api/admin_test.go | 2 +- api/file.go | 31 ++++++++++++++++++++----------- api/file_benchmark_test.go | 13 +------------ api/file_test.go | 45 ++++++++++++++++++++++++++------------------- 4 files changed, 48 insertions(+), 43 deletions(-) (limited to 'api') diff --git a/api/admin_test.go b/api/admin_test.go index a4420ccbc..013e72944 100644 --- a/api/admin_test.go +++ b/api/admin_test.go @@ -70,7 +70,7 @@ func TestGetConfig(t *testing.T) { if *cfg.LdapSettings.BindPassword != model.FAKE_SETTING && len(*cfg.LdapSettings.BindPassword) != 0 { t.Fatal("did not sanitize properly") } - if cfg.FileSettings.PublicLinkSalt != model.FAKE_SETTING { + if *cfg.FileSettings.PublicLinkSalt != model.FAKE_SETTING { t.Fatal("did not sanitize properly") } if cfg.FileSettings.AmazonS3SecretAccessKey != model.FAKE_SETTING && len(cfg.FileSettings.AmazonS3SecretAccessKey) != 0 { diff --git a/api/file.go b/api/file.go index ea07f16f8..113666270 100644 --- a/api/file.go +++ b/api/file.go @@ -5,6 +5,8 @@ package api import ( "bytes" + "crypto/sha256" + "encoding/base64" "fmt" "image" "image/color" @@ -14,7 +16,6 @@ import ( "io" "io/ioutil" "net/http" - "net/url" "os" "path/filepath" "strconv" @@ -377,7 +378,6 @@ func getPublicFile(c *Context, w http.ResponseWriter, r *http.Request) { filename := params["filename"] hash := r.URL.Query().Get("h") - data := r.URL.Query().Get("d") if !utils.Cfg.FileSettings.EnablePublicLink { c.Err = model.NewLocAppError("getPublicFile", "api.file.get_file.public_disabled.app_error", nil, "") @@ -385,8 +385,10 @@ func getPublicFile(c *Context, w http.ResponseWriter, r *http.Request) { return } - if len(hash) > 0 && len(data) > 0 { - if !model.ComparePassword(hash, fmt.Sprintf("%v:%v", data, utils.Cfg.FileSettings.PublicLinkSalt)) { + if len(hash) > 0 { + correctHash := generatePublicLinkHash(filename, *utils.Cfg.FileSettings.PublicLinkSalt) + + if hash != correctHash { c.Err = model.NewLocAppError("getPublicFile", "api.file.get_file.public_invalid.app_error", nil, "") c.Err.StatusCode = http.StatusBadRequest return @@ -512,13 +514,7 @@ func getPublicLink(c *Context, w http.ResponseWriter, r *http.Request) { cchan := Srv.Store.Channel().CheckPermissionsTo(c.TeamId, channelId, c.Session.UserId) - newProps := make(map[string]string) - newProps["filename"] = filename - - data := model.MapToJson(newProps) - hash := model.HashPassword(fmt.Sprintf("%v:%v", data, utils.Cfg.FileSettings.PublicLinkSalt)) - - url := fmt.Sprintf("%s/public/files/get/%s/%s/%s/%s?d=%s&h=%s", c.GetSiteURL()+model.API_URL_SUFFIX, c.TeamId, channelId, userId, filename, url.QueryEscape(data), url.QueryEscape(hash)) + url := generatePublicLink(c.GetSiteURL(), c.TeamId, channelId, userId, filename) if !c.HasPermissionsToChannel(cchan, "getPublicLink") { return @@ -527,6 +523,19 @@ func getPublicLink(c *Context, w http.ResponseWriter, r *http.Request) { w.Write([]byte(model.StringToJson(url))) } +func generatePublicLink(siteURL, teamId, channelId, userId, filename string) string { + hash := generatePublicLinkHash(filename, *utils.Cfg.FileSettings.PublicLinkSalt) + return fmt.Sprintf("%s%s/public/files/get/%s/%s/%s/%s?h=%s", siteURL, model.API_URL_SUFFIX, teamId, channelId, userId, filename, hash) +} + +func generatePublicLinkHash(filename, salt string) string { + hash := sha256.New() + hash.Write([]byte(salt)) + hash.Write([]byte(filename)) + + return base64.RawURLEncoding.EncodeToString(hash.Sum(nil)) +} + func WriteFile(f []byte, path string) *model.AppError { if utils.Cfg.FileSettings.DriverName == model.IMAGE_DRIVER_S3 { diff --git a/api/file_benchmark_test.go b/api/file_benchmark_test.go index f14d501ff..0e0fc105b 100644 --- a/api/file_benchmark_test.go +++ b/api/file_benchmark_test.go @@ -4,10 +4,7 @@ package api import ( - "fmt" - "github.com/mattermost/platform/model" "github.com/mattermost/platform/utils" - "net/url" "testing" "time" ) @@ -29,7 +26,6 @@ func BenchmarkUploadFile(b *testing.B) { func BenchmarkGetFile(b *testing.B) { th := Setup().InitBasic() Client := th.BasicClient - team := th.BasicTeam channel := th.BasicChannel testPoster := NewAutoPostCreator(Client, channel.Id) @@ -38,20 +34,13 @@ func BenchmarkGetFile(b *testing.B) { b.Fatal("Unable to upload file for benchmark") } - newProps := make(map[string]string) - newProps["filename"] = filenames[0] - newProps["time"] = fmt.Sprintf("%v", model.GetMillis()) - - data := model.MapToJson(newProps) - hash := model.HashPassword(fmt.Sprintf("%v:%v", data, utils.Cfg.FileSettings.PublicLinkSalt)) - // wait a bit for files to ready time.Sleep(5 * time.Second) // Benchmark Start b.ResetTimer() for i := 0; i < b.N; i++ { - if _, downErr := Client.GetFile(filenames[0]+"?d="+url.QueryEscape(data)+"&h="+url.QueryEscape(hash)+"&t="+team.Id, true); downErr != nil { + if _, downErr := Client.GetFile(filenames[0]+"?h="+generatePublicLinkHash(filenames[0], *utils.Cfg.FileSettings.PublicLinkSalt), true); downErr != nil { b.Fatal(downErr) } } diff --git a/api/file_test.go b/api/file_test.go index fe7355122..764f326cd 100644 --- a/api/file_test.go +++ b/api/file_test.go @@ -290,15 +290,7 @@ func TestGetPublicFile(t *testing.T) { } if resp, err := http.Get(link[:strings.LastIndex(link, "?")]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while logged in without query params", resp.Status) - } - - if resp, err := http.Get(link[:strings.LastIndex(link, "&")]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while logged in without second query param") - } - - if resp, err := http.Get(link[:strings.LastIndex(link, "?")] + "?" + link[strings.LastIndex(link, "&"):]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while logged in without first query param") + t.Fatal("should've failed to get image with public link while logged in without hash", resp.Status) } utils.Cfg.FileSettings.EnablePublicLink = false @@ -316,15 +308,7 @@ func TestGetPublicFile(t *testing.T) { } if resp, err := http.Get(link[:strings.LastIndex(link, "?")]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while not logged in without query params") - } - - if resp, err := http.Get(link[:strings.LastIndex(link, "&")]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while not logged in without second query param") - } - - if resp, err := http.Get(link[:strings.LastIndex(link, "?")] + "?" + link[strings.LastIndex(link, "&"):]); err == nil && resp.StatusCode != http.StatusBadRequest { - t.Fatal("should've failed to get image with public link while not logged in without first query param") + t.Fatal("should've failed to get image with public link while not logged in without hash") } utils.Cfg.FileSettings.EnablePublicLink = false @@ -335,7 +319,7 @@ func TestGetPublicFile(t *testing.T) { utils.Cfg.FileSettings.EnablePublicLink = true // test a user that's logged in after the salt has changed - utils.Cfg.FileSettings.PublicLinkSalt = model.NewId() + *utils.Cfg.FileSettings.PublicLinkSalt = model.NewId() th.LoginBasic() if resp, err := http.Get(link); err == nil && resp.StatusCode != http.StatusBadRequest { @@ -408,6 +392,29 @@ func TestGetPublicLink(t *testing.T) { } } +func TestGeneratePublicLinkHash(t *testing.T) { + filename1 := model.NewId() + "/" + model.NewRandomString(16) + ".txt" + filename2 := model.NewId() + "/" + model.NewRandomString(16) + ".txt" + salt1 := model.NewRandomString(32) + salt2 := model.NewRandomString(32) + + hash1 := generatePublicLinkHash(filename1, salt1) + hash2 := generatePublicLinkHash(filename2, salt1) + hash3 := generatePublicLinkHash(filename1, salt2) + + if hash1 != generatePublicLinkHash(filename1, salt1) { + t.Fatal("hash should be equal for the same file name and salt") + } + + if hash1 == hash2 { + t.Fatal("hashes for different files should not be equal") + } + + if hash1 == hash3 { + t.Fatal("hashes for the same file with different salts should not be equal") + } +} + func uploadTestFile(Client *model.Client, channelId string) ([]string, error) { body := &bytes.Buffer{} writer := multipart.NewWriter(body) -- cgit v1.2.3-1-g7c22