From 2a259dd3b10993a93eb1c1ccdbb0b359336872b1 Mon Sep 17 00:00:00 2001
From: Florian Schade <fschade@datario.de>
Date: Thu, 26 Nov 2020 22:14:54 +0100
Subject: [PATCH] add Unset to cache.go harden accounts cache security

---
 accounts/pkg/service/v0/accounts.go | 41 ++++++++++++++++++++++++-----
 ocis-pkg/cache/cache.go             | 14 ++++++++--
 tests/k6/src/test-issue-162.ts      |  8 ++++--
 3 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/accounts/pkg/service/v0/accounts.go b/accounts/pkg/service/v0/accounts.go
index 6627e3e35..2d53adb06 100644
--- a/accounts/pkg/service/v0/accounts.go
+++ b/accounts/pkg/service/v0/accounts.go
@@ -1,6 +1,7 @@
 package service
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"fmt"
@@ -160,23 +161,49 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
 			return merrors.Unauthorized(s.id, "account not found or invalid credentials")
 		}
 
-		h := sha256.New()
-		h.Write([]byte(a.PasswordProfile.Password))
+		// isPasswordValid uses bcrypt.CompareHashAndPassword which is slow by design.
+		// if every request that matches authQuery regex needs to do this step over and over again,
+		// this is secure but also slow. In this implementation we keep it same secure but increase the speed.
+		//
+		// flow:
+		// - request comes in
+		// - it creates a sha256 based on found account PasswordProfile.LastPasswordChangeDateTime and requested password
+		// - it checks if the cache already contains an entry that matches found account Id
+		// - if no entry exists it runs the bcrypt.CompareHashAndPassword as before and if everything is ok it stores the
+		//   result by the account Id as key and mentioned hash as value. If not it errors
+		// - if a entry is found it checks if the given value matches the mentioned hash. If it doesnt match the cache entry gets removed
+		//   and it errors.
+		//
+		// if many concurrent requests from the same user come in within a short period of time, it's possible that e is still nil.
+		// This is why all this needs to be wrapped within a sync.Mutex locking to prevent calling bcrypt.CompareHashAndPassword unnecessarily too often.
+		{
+			var suspicious bool
 
-		k := fmt.Sprintf("%x", h.Sum([]byte(password)))
+			h := sha256.New()
+			h.Write([]byte(a.PasswordProfile.Password))
 
-		if hit := passwordValidCache.Get(k); hit == nil || !hit.V.(bool) {
-			var ok bool
+			m := h.Sum([]byte(password))
+			e := passwordValidCache.Get(a.Id)
 
-			if ok = isPasswordValid(s.log, a.PasswordProfile.Password, password); !ok {
+			if e == nil {
+				suspicious = !isPasswordValid(s.log, a.PasswordProfile.Password, password)
+			} else if bytes.Compare(e.V.([]byte), m) != 0 {
+				suspicious = true
+			}
+
+			if suspicious {
+				passwordValidCache.Unset(a.Id)
 				return merrors.Unauthorized(s.id, "account not found or invalid credentials")
 			}
 
-			passwordValidCache.Set(k, ok, time.Now().Add(passwordValidCacheExpiration))
+			if e == nil {
+				passwordValidCache.Set(a.Id, m, time.Now().Add(passwordValidCacheExpiration))
+			}
 		}
 
 		a.PasswordProfile.Password = ""
 		out.Accounts = []*proto.Account{a}
+
 		return nil
 	}
 
diff --git a/ocis-pkg/cache/cache.go b/ocis-pkg/cache/cache.go
index fcfa7e01b..164901898 100644
--- a/ocis-pkg/cache/cache.go
+++ b/ocis-pkg/cache/cache.go
@@ -28,7 +28,7 @@ func NewCache(o ...Option) Cache {
 	}
 }
 
-// Get gets a role-bundle by a given `roleID`.
+// Get gets an entry by given key
 func (c *Cache) Get(k string) *Entry {
 	c.m.Lock()
 	defer c.m.Unlock()
@@ -43,7 +43,7 @@ func (c *Cache) Get(k string) *Entry {
 	return nil
 }
 
-// Set sets a roleID / role-bundle.
+// Set adds an entry for given key and value
 func (c *Cache) Set(k string, val interface{}, expiration time.Time) {
 	c.m.Lock()
 	defer c.m.Unlock()
@@ -58,6 +58,16 @@ func (c *Cache) Set(k string, val interface{}, expiration time.Time) {
 	}
 }
 
+// Unset removes an entry by given key
+func (c *Cache) Unset(k string) bool {
+	if _, ok := c.entries[k]; !ok {
+		return false
+	}
+
+	delete(c.entries, k)
+	return true
+}
+
 // evict frees memory from the cache by removing entries that exceeded the cache TTL.
 func (c *Cache) evict() {
 	for i := range c.entries {
diff --git a/tests/k6/src/test-issue-162.ts b/tests/k6/src/test-issue-162.ts
index c2e57cf5c..c09f1c1ce 100644
--- a/tests/k6/src/test-issue-162.ts
+++ b/tests/k6/src/test-issue-162.ts
@@ -13,10 +13,14 @@ export let options: Options = {
 };
 
 export default () => {
-    const res = api.uploadFile(defaults.accounts.einstein, files['kb_50.jpg'], `kb_50-${__VU}-${__ITER}.jpg`)
+    const res = api.uploadFile(
+        defaults.accounts.einstein,
+        files['kb_50.jpg'],
+        `kb_50-${__VU}-${__ITER}.jpg`,
+    );
 
     check(res, {
-        'status is 201': () => res.status === 201,
+        'status is 204': () => res.status === 204,
     });
 
     sleep(1);