grafana/011-use-hmac-sha-256-for-password-reset-tokens.patch

commit f13c08e9f45d7776cb264b17ec41bc4ff51fc0b9
Author: Andreas Gerstmayr <agerstmayr@redhat.com>
Date:   Thu Nov 25 18:49:52 2021 +0100

    notifications: use HMAC-SHA256 to generate time limit codes
    
    * changes the time limit code generation function to use HMAC-SHA256
      instead of SHA-1
    * multiple new testcases

diff --git a/pkg/services/notifications/codes.go b/pkg/services/notifications/codes.go
index ea9beb30cc..1ddf05dc69 100644
--- a/pkg/services/notifications/codes.go
+++ b/pkg/services/notifications/codes.go
@@ -1,48 +1,53 @@
 package notifications
 
 import (
-	"crypto/sha1" // #nosec
+	"crypto/hmac"
+	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"strconv"
 	"time"
 
-	"github.com/unknwon/com"
-
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
-const timeLimitCodeLength = 12 + 6 + 40
+const timeLimitStartDateLength = 12
+const timeLimitMinutesLength = 6
+const timeLimitHmacLength = 64
+const timeLimitCodeLength = timeLimitStartDateLength + timeLimitMinutesLength + timeLimitHmacLength
 
 // create a time limit code
-// code format: 12 length date time string + 6 minutes string + 40 sha1 encoded string
-func createTimeLimitCode(data string, minutes int, startInf interface{}) (string, error) {
+// code format: 12 length date time string + 6 minutes string + 64 HMAC-SHA256 encoded string
+func createTimeLimitCode(payload string, minutes int, startStr string) (string, error) {
 	format := "200601021504"
 
 	var start, end time.Time
-	var startStr, endStr string
+	var endStr string
 
-	if startInf == nil {
+	if startStr == "" {
 		// Use now time create code
 		start = time.Now()
 		startStr = start.Format(format)
 	} else {
 		// use start string create code
-		startStr = startInf.(string)
-		start, _ = time.ParseInLocation(format, startStr, time.Local)
-		startStr = start.Format(format)
+		var err error
+		start, err = time.ParseInLocation(format, startStr, time.Local)
+		if err != nil {
+			return "", err
+		}
 	}
 
 	end = start.Add(time.Minute * time.Duration(minutes))
 	endStr = end.Format(format)
 
-	// create sha1 encode string
-	sh := sha1.New()
-	if _, err := sh.Write([]byte(data + setting.SecretKey + startStr + endStr +
-		com.ToStr(minutes))); err != nil {
-		return "", err
+	// create HMAC-SHA256 encoded string
+	key := []byte(setting.SecretKey)
+	h := hmac.New(sha256.New, key)
+	if _, err := h.Write([]byte(payload + startStr + endStr)); err != nil {
+		return "", fmt.Errorf("cannot create hmac: %v", err)
 	}
-	encoded := hex.EncodeToString(sh.Sum(nil))
+	encoded := hex.EncodeToString(h.Sum(nil))
 
 	code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded)
 	return code, nil
@@ -50,30 +55,29 @@ func createTimeLimitCode(data string, minutes int, startInf interface{}) (string
 
 // verify time limit code
 func validateUserEmailCode(user *models.User, code string) (bool, error) {
-	if len(code) <= 18 {
+	if len(code) < timeLimitCodeLength {
 		return false, nil
 	}
 
-	minutes := setting.EmailCodeValidMinutes
 	code = code[:timeLimitCodeLength]
 
 	// split code
-	start := code[:12]
-	lives := code[12:18]
-	if d, err := com.StrTo(lives).Int(); err == nil {
-		minutes = d
+	startStr := code[:timeLimitStartDateLength]
+	minutesStr := code[timeLimitStartDateLength : timeLimitStartDateLength+timeLimitMinutesLength]
+	minutes, err := strconv.Atoi(minutesStr)
+	if err != nil {
+		return false, fmt.Errorf("invalid time limit code: %v", err)
 	}
 
-	// right active code
-	data := com.ToStr(user.Id) + user.Email + user.Login + user.Password + user.Rands
-	retCode, err := createTimeLimitCode(data, minutes, start)
+	// verify code
+	payload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands
+	expectedCode, err := createTimeLimitCode(payload, minutes, startStr)
 	if err != nil {
 		return false, err
 	}
-	fmt.Printf("code : %s\ncode2: %s", retCode, code)
-	if retCode == code && minutes > 0 {
+	if hmac.Equal([]byte(code), []byte(expectedCode)) && minutes > 0 {
 		// check time is expired or not
-		before, _ := time.ParseInLocation("200601021504", start, time.Local)
+		before, _ := time.ParseInLocation("200601021504", startStr, time.Local)
 		now := time.Now()
 		if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() {
 			return true, nil
@@ -94,15 +98,15 @@ func getLoginForEmailCode(code string) string {
 	return string(b)
 }
 
-func createUserEmailCode(u *models.User, startInf interface{}) (string, error) {
+func createUserEmailCode(user *models.User, startStr string) (string, error) {
 	minutes := setting.EmailCodeValidMinutes
-	data := com.ToStr(u.Id) + u.Email + u.Login + u.Password + u.Rands
-	code, err := createTimeLimitCode(data, minutes, startInf)
+	payload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands
+	code, err := createTimeLimitCode(payload, minutes, startStr)
 	if err != nil {
 		return "", err
 	}
 
 	// add tail hex username
-	code += hex.EncodeToString([]byte(u.Login))
+	code += hex.EncodeToString([]byte(user.Login))
 	return code, nil
 }
diff --git a/pkg/services/notifications/codes_test.go b/pkg/services/notifications/codes_test.go
index d2b1f3a617..bea88e0bf5 100644
--- a/pkg/services/notifications/codes_test.go
+++ b/pkg/services/notifications/codes_test.go
@@ -1,19 +1,129 @@
 package notifications
 
 import (
+	"fmt"
+	"strconv"
 	"testing"
+	"time"
 
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/require"
 )
 
+func TestTimeLimitCodes(t *testing.T) {
+	user := &models.User{Id: 10, Email: "t@a.com", Login: "asd", Password: "1", Rands: "2"}
+
+	format := "200601021504"
+	mailPayload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands
+	tenMinutesAgo := time.Now().Add(-time.Minute * 10)
+
+	tests := []struct {
+		desc    string
+		payload string
+		start   time.Time
+		minutes int
+		valid   bool
+	}{
+		{
+			desc:    "code generated 10 minutes ago, 5 minutes valid",
+			payload: mailPayload,
+			start:   tenMinutesAgo,
+			minutes: 5,
+			valid:   false,
+		},
+		{
+			desc:    "code generated 10 minutes ago, 9 minutes valid",
+			payload: mailPayload,
+			start:   tenMinutesAgo,
+			minutes: 9,
+			valid:   false,
+		},
+		{
+			desc:    "code generated 10 minutes ago, 10 minutes valid",
+			payload: mailPayload,
+			start:   tenMinutesAgo,
+			minutes: 10,
+			// code was valid exactly 10 minutes since evaluating the tenMinutesAgo assignment
+			// by the time this test is run the code is already expired
+			valid: false,
+		},
+		{
+			desc:    "code generated 10 minutes ago, 11 minutes valid",
+			payload: mailPayload,
+			start:   tenMinutesAgo,
+			minutes: 11,
+			valid:   true,
+		},
+		{
+			desc:    "code generated 10 minutes ago, 20 minutes valid",
+			payload: mailPayload,
+			start:   tenMinutesAgo,
+			minutes: 20,
+			valid:   true,
+		},
+		{
+			desc:    "code generated 10 minutes ago, 20 minutes valid, tampered payload",
+			payload: mailPayload[:len(mailPayload)-1] + "x",
+			start:   tenMinutesAgo,
+			minutes: 20,
+			valid:   false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			code, err := createTimeLimitCode(test.payload, test.minutes, test.start.Format(format))
+			require.NoError(t, err)
+
+			isValid, err := validateUserEmailCode(user, code)
+			require.NoError(t, err)
+			require.Equal(t, test.valid, isValid)
+		})
+	}
+
+	t.Run("tampered minutes", func(t *testing.T) {
+		code, err := createTimeLimitCode(mailPayload, 5, tenMinutesAgo.Format(format))
+		require.NoError(t, err)
+
+		// code is expired
+		isValid, err := validateUserEmailCode(user, code)
+		require.NoError(t, err)
+		require.Equal(t, false, isValid)
+
+		// let's try to extend the code by tampering the minutes
+		code = code[:12] + fmt.Sprintf("%06d", 20) + code[18:]
+		isValid, err = validateUserEmailCode(user, code)
+		require.NoError(t, err)
+		require.Equal(t, false, isValid)
+	})
+
+	t.Run("tampered start string", func(t *testing.T) {
+		code, err := createTimeLimitCode(mailPayload, 5, tenMinutesAgo.Format(format))
+		require.NoError(t, err)
+
+		// code is expired
+		isValid, err := validateUserEmailCode(user, code)
+		require.NoError(t, err)
+		require.Equal(t, false, isValid)
+
+		// let's try to extend the code by tampering the start string
+		oneMinuteAgo := time.Now().Add(-time.Minute)
+
+		code = oneMinuteAgo.Format(format) + code[12:]
+		isValid, err = validateUserEmailCode(user, code)
+		require.NoError(t, err)
+		require.Equal(t, false, isValid)
+	})
+}
+
 func TestEmailCodes(t *testing.T) {
 	Convey("When generating code", t, func() {
 		setting.EmailCodeValidMinutes = 120
 
 		user := &models.User{Id: 10, Email: "t@a.com", Login: "asd", Password: "1", Rands: "2"}
-		code, err := createUserEmailCode(user, nil)
+		code, err := createUserEmailCode(user, "")
 		So(err, ShouldBeNil)
 
 		Convey("getLoginForCode should return login", func() {
@@ -27,7 +137,7 @@ func TestEmailCodes(t *testing.T) {
 			So(isValid, ShouldBeTrue)
 		})
 
-		Convey("Cannot verify in-valid code", func() {
+		Convey("Cannot verify invalid code", func() {
 			code = "ASD"
 			isValid, err := validateUserEmailCode(user, code)
 			So(err, ShouldBeNil)
diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go
index beea82f43e..5a575d1415 100644
--- a/pkg/services/notifications/notifications.go
+++ b/pkg/services/notifications/notifications.go
@@ -149,7 +149,7 @@ func (ns *NotificationService) sendEmailCommandHandler(cmd *models.SendEmailComm
 }
 
 func (ns *NotificationService) sendResetPasswordEmail(cmd *models.SendResetPasswordEmailCommand) error {
-	code, err := createUserEmailCode(cmd.User, nil)
+	code, err := createUserEmailCode(cmd.User, "")
 	if err != nil {
 		return err
 	}
diff --git a/pkg/services/notifications/notifications_test.go b/pkg/services/notifications/notifications_test.go
index e7680c3943..fb73e332ea 100644
--- a/pkg/services/notifications/notifications_test.go
+++ b/pkg/services/notifications/notifications_test.go
@@ -1,12 +1,14 @@
 package notifications
 
 import (
+	"regexp"
 	"testing"
 
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNotifications(t *testing.T) {
@@ -25,13 +27,28 @@ func TestNotifications(t *testing.T) {
 		So(err, ShouldBeNil)
 
 		Convey("When sending reset email password", func() {
-			err := ns.sendResetPasswordEmail(&models.SendResetPasswordEmailCommand{User: &models.User{Email: "asd@asd.com"}})
+			user := models.User{Email: "asd@asd.com", Login: "asd@asd.com"}
+			err := ns.sendResetPasswordEmail(&models.SendResetPasswordEmailCommand{User: &user})
 			So(err, ShouldBeNil)
 
 			sentMsg := <-ns.mailQueue
 			So(sentMsg.Body, ShouldContainSubstring, "body")
 			So(sentMsg.Subject, ShouldEqual, "Reset your Grafana password - asd@asd.com")
 			So(sentMsg.Body, ShouldNotContainSubstring, "Subject")
+
+			// find code in mail
+			r, _ := regexp.Compile(`code=(\w+)`)
+			match := r.FindString(sentMsg.Body)
+			code := match[len("code="):]
+
+			// verify code
+			bus.AddHandler("test", func(query *models.GetUserByLoginQuery) error {
+				query.Result = &user
+				return nil
+			})
+			query := models.ValidateResetPasswordCodeQuery{Code: code}
+			err = ns.validateResetPasswordCode(&query)
+			require.NoError(t, err)
 		})
 	})
 }