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

4 files changed, 48 insertions, 43 deletions

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)