From 5612cf32e5c66d4e7757a4f1c19f6b9edd44b9dc Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 20 May 2024 23:12:50 +0800 Subject: [PATCH] Refactor sha1 and time-limited code (#31023) Remove "EncodeSha1", it shouldn't be used as a general purpose hasher (just like we have removed "EncodeMD5" in #28622) Rewrite the "time-limited code" related code and write better tests, the old code doesn't seem quite right. (cherry picked from commit fb1ad920b769799aa1287441289d15477d9878c5) Conflicts: modules/git/utils_test.go trivial context conflict because sha256 testing in Forgejo has diverged --- models/user/email_address.go | 5 +- models/user/user.go | 7 +-- modules/base/tool.go | 85 ++++++++++++++++------------------ modules/base/tool_test.go | 89 ++++++++++++++++++++---------------- modules/git/utils.go | 8 ++++ modules/git/utils_test.go | 13 +++++- routers/web/repo/compare.go | 2 +- services/gitdiff/gitdiff.go | 3 +- 8 files changed, 115 insertions(+), 97 deletions(-) diff --git a/models/user/email_address.go b/models/user/email_address.go index 85824fcdc..18bf6d0b8 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -10,6 +10,7 @@ import ( "net/mail" "regexp" "strings" + "time" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/base" @@ -362,14 +363,12 @@ func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error { // VerifyActiveEmailCode verifies active email code when active account func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress { - minutes := setting.Service.ActiveCodeLives - if user := GetVerifyUser(ctx, code); user != nil { // time limit code prefix := code[:base.TimeLimitCodeLength] data := fmt.Sprintf("%d%s%s%s%s", user.ID, email, user.LowerName, user.Passwd, user.Rands) - if base.VerifyTimeLimitCode(data, minutes, prefix) { + if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) { emailAddress := &EmailAddress{UID: user.ID, Email: email} if has, _ := db.GetEngine(ctx).Get(emailAddress); has { return emailAddress diff --git a/models/user/user.go b/models/user/user.go index 5844189e1..58808c71b 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -321,7 +321,7 @@ func (u *User) OrganisationLink() string { func (u *User) GenerateEmailActivateCode(email string) string { code := base.CreateTimeLimitCode( fmt.Sprintf("%d%s%s%s%s", u.ID, email, u.LowerName, u.Passwd, u.Rands), - setting.Service.ActiveCodeLives, nil) + setting.Service.ActiveCodeLives, time.Now(), nil) // Add tail hex username code += hex.EncodeToString([]byte(u.LowerName)) @@ -818,14 +818,11 @@ func GetVerifyUser(ctx context.Context, code string) (user *User) { // VerifyUserActiveCode verifies active code when active account func VerifyUserActiveCode(ctx context.Context, code string) (user *User) { - minutes := setting.Service.ActiveCodeLives - if user = GetVerifyUser(ctx, code); user != nil { // time limit code prefix := code[:base.TimeLimitCodeLength] data := fmt.Sprintf("%d%s%s%s%s", user.ID, user.Email, user.LowerName, user.Passwd, user.Rands) - - if base.VerifyTimeLimitCode(data, minutes, prefix) { + if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) { return user } } diff --git a/modules/base/tool.go b/modules/base/tool.go index e4c3fb181..c4c0ec2df 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -4,12 +4,15 @@ package base import ( + "crypto/hmac" "crypto/sha1" "crypto/sha256" + "crypto/subtle" "encoding/base64" "encoding/hex" "errors" "fmt" + "hash" "os" "path/filepath" "runtime" @@ -25,13 +28,6 @@ import ( "github.com/dustin/go-humanize" ) -// EncodeSha1 string to sha1 hex value. -func EncodeSha1(str string) string { - h := sha1.New() - _, _ = h.Write([]byte(str)) - return hex.EncodeToString(h.Sum(nil)) -} - // EncodeSha256 string to sha256 hex value. func EncodeSha256(str string) string { h := sha256.New() @@ -62,63 +58,62 @@ func BasicAuthDecode(encoded string) (string, string, error) { } // VerifyTimeLimitCode verify time limit code -func VerifyTimeLimitCode(data string, minutes int, code string) bool { +func VerifyTimeLimitCode(now time.Time, data string, minutes int, code string) bool { if len(code) <= 18 { return false } - // split code - start := code[:12] - lives := code[12:18] - if d, err := strconv.ParseInt(lives, 10, 0); err == nil { - minutes = int(d) - } + startTimeStr := code[:12] + aliveTimeStr := code[12:18] + aliveTime, _ := strconv.Atoi(aliveTimeStr) // no need to check err, if anything wrong, the following code check will fail soon - // right active code - retCode := CreateTimeLimitCode(data, minutes, start) - if retCode == code && minutes > 0 { - // check time is expired or not - before, _ := time.ParseInLocation("200601021504", start, time.Local) - now := time.Now() - if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() { - return true + // check code + retCode := CreateTimeLimitCode(data, aliveTime, startTimeStr, nil) + if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { + retCode = CreateTimeLimitCode(data, aliveTime, startTimeStr, sha1.New()) // TODO: this is only for the support of legacy codes, remove this in/after 1.23 + if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { + return false } } - return false + // check time is expired or not: startTime <= now && now < startTime + minutes + startTime, _ := time.ParseInLocation("200601021504", startTimeStr, time.Local) + return (startTime.Before(now) || startTime.Equal(now)) && now.Before(startTime.Add(time.Minute*time.Duration(minutes))) } // TimeLimitCodeLength default value for time limit code const TimeLimitCodeLength = 12 + 6 + 40 -// CreateTimeLimitCode 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 any) string { - format := "200601021504" +// CreateTimeLimitCode create a time-limited code. +// Format: 12 length date time string + 6 minutes string (not used) + 40 hash string, some other code depends on this fixed length +// If h is nil, then use the default hmac hash. +func CreateTimeLimitCode[T time.Time | string](data string, minutes int, startTimeGeneric T, h hash.Hash) string { + const format = "200601021504" - var start, end time.Time - var startStr, endStr string - - if startInf == nil { - // Use now time create code - start = time.Now() - startStr = start.Format(format) + var start time.Time + var startTimeAny any = startTimeGeneric + if t, ok := startTimeAny.(time.Time); ok { + start = t } 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, startTimeAny.(string), time.Local) + if err != nil { + return "" // return an invalid code because the "parse" failed + } } + startStr := start.Format(format) + end := start.Add(time.Minute * time.Duration(minutes)) - end = start.Add(time.Minute * time.Duration(minutes)) - endStr = end.Format(format) - - // create sha1 encode string - sh := sha1.New() - _, _ = sh.Write([]byte(fmt.Sprintf("%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, endStr, minutes))) - encoded := hex.EncodeToString(sh.Sum(nil)) + if h == nil { + h = hmac.New(sha1.New, setting.GetGeneralTokenSigningSecret()) + } + _, _ = fmt.Fprintf(h, "%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, end.Format(format), minutes) + encoded := hex.EncodeToString(h.Sum(nil)) code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) + if len(code) != TimeLimitCodeLength { + panic("there is a hard requirement for the length of time-limited code") // it shouldn't happen + } return code } diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go index f21b89c74..62de7229a 100644 --- a/modules/base/tool_test.go +++ b/modules/base/tool_test.go @@ -4,20 +4,18 @@ package base import ( + "crypto/sha1" + "fmt" "os" "testing" "time" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/assert" ) -func TestEncodeSha1(t *testing.T) { - assert.Equal(t, - "8843d7f92416211de9ebb963ff4ce28125932878", - EncodeSha1("foobar"), - ) -} - func TestEncodeSha256(t *testing.T) { assert.Equal(t, "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2", @@ -46,43 +44,54 @@ func TestBasicAuthDecode(t *testing.T) { } func TestVerifyTimeLimitCode(t *testing.T) { - tc := []struct { - data string - minutes int - code string - valid bool - }{{ - data: "data", - minutes: 2, - code: testCreateTimeLimitCode(t, "data", 2), - valid: true, - }, { - data: "abc123-ß", - minutes: 1, - code: testCreateTimeLimitCode(t, "abc123-ß", 1), - valid: true, - }, { - data: "data", - minutes: 2, - code: "2021012723240000005928251dac409d2c33a6eb82c63410aaad569bed", - valid: false, - }} - for _, test := range tc { - actualValid := VerifyTimeLimitCode(test.data, test.minutes, test.code) - assert.Equal(t, test.valid, actualValid, "data: '%s' code: '%s' should be valid: %t", test.data, test.code, test.valid) + defer test.MockVariableValue(&setting.InstallLock, true)() + initGeneralSecret := func(secret string) { + setting.InstallLock = true + setting.CfgProvider, _ = setting.NewConfigProviderFromData(fmt.Sprintf(` +[oauth2] +JWT_SECRET = %s +`, secret)) + setting.LoadCommonSettings() } -} -func testCreateTimeLimitCode(t *testing.T, data string, m int) string { - result0 := CreateTimeLimitCode(data, m, nil) - result1 := CreateTimeLimitCode(data, m, time.Now().Format("200601021504")) - result2 := CreateTimeLimitCode(data, m, time.Unix(time.Now().Unix()+int64(time.Minute)*int64(m), 0).Format("200601021504")) + initGeneralSecret("KZb_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko") + now := time.Now() - assert.Equal(t, result0, result1) - assert.NotEqual(t, result0, result2) + t.Run("TestGenericParameter", func(t *testing.T) { + time2000 := time.Date(2000, 1, 2, 3, 4, 5, 0, time.Local) + assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, time2000, sha1.New())) + assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, "200001020304", sha1.New())) + assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, time2000, nil)) + assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, "200001020304", nil)) + }) - assert.True(t, len(result0) != 0) - return result0 + t.Run("TestInvalidCode", func(t *testing.T) { + assert.False(t, VerifyTimeLimitCode(now, "data", 2, "")) + assert.False(t, VerifyTimeLimitCode(now, "data", 2, "invalid code")) + }) + + t.Run("TestCreateAndVerify", func(t *testing.T) { + code := CreateTimeLimitCode("data", 2, now, nil) + assert.False(t, VerifyTimeLimitCode(now.Add(-time.Minute), "data", 2, code)) // not started yet + assert.True(t, VerifyTimeLimitCode(now, "data", 2, code)) + assert.True(t, VerifyTimeLimitCode(now.Add(time.Minute), "data", 2, code)) + assert.False(t, VerifyTimeLimitCode(now.Add(time.Minute), "DATA", 2, code)) // invalid data + assert.False(t, VerifyTimeLimitCode(now.Add(2*time.Minute), "data", 2, code)) // expired + }) + + t.Run("TestDifferentSecret", func(t *testing.T) { + // use another secret to ensure the code is invalid for different secret + verifyDataCode := func(c string) bool { + return VerifyTimeLimitCode(now, "data", 2, c) + } + code1 := CreateTimeLimitCode("data", 2, now, sha1.New()) + code2 := CreateTimeLimitCode("data", 2, now, nil) + assert.True(t, verifyDataCode(code1)) + assert.True(t, verifyDataCode(code2)) + initGeneralSecret("000_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko") + assert.False(t, verifyDataCode(code1)) + assert.False(t, verifyDataCode(code2)) + }) } func TestFileSize(t *testing.T) { diff --git a/modules/git/utils.go b/modules/git/utils.go index 0d6741270..53211c645 100644 --- a/modules/git/utils.go +++ b/modules/git/utils.go @@ -4,6 +4,8 @@ package git import ( + "crypto/sha1" + "encoding/hex" "fmt" "io" "os" @@ -128,3 +130,9 @@ func (l *LimitedReaderCloser) Read(p []byte) (n int, err error) { func (l *LimitedReaderCloser) Close() error { return l.C.Close() } + +func HashFilePathForWebUI(s string) string { + h := sha1.New() + _, _ = h.Write([]byte(s)) + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/modules/git/utils_test.go b/modules/git/utils_test.go index 876a22924..a3c2b7f8e 100644 --- a/modules/git/utils_test.go +++ b/modules/git/utils_test.go @@ -3,7 +3,11 @@ package git -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) // This file contains utility functions that are used across multiple tests, // but not in production code. @@ -13,3 +17,10 @@ func skipIfSHA256NotSupported(t *testing.T) { t.Skip("skipping because installed Git version doesn't support SHA256") } } + +func TestHashFilePathForWebUI(t *testing.T) { + assert.Equal(t, + "8843d7f92416211de9ebb963ff4ce28125932878", + HashFilePathForWebUI("foobar"), + ) +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 2a9f60891..088e5150f 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -931,7 +931,7 @@ func ExcerptBlob(ctx *context.Context) { } } ctx.Data["section"] = section - ctx.Data["FileNameHash"] = base.EncodeSha1(filePath) + ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath) ctx.Data["AfterCommitID"] = commitID ctx.Data["Anchor"] = anchor ctx.HTML(http.StatusOK, tplBlobExcerpt) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index d9dbeedee..c4430339e 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -23,7 +23,6 @@ import ( pull_model "code.gitea.io/gitea/models/pull" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/analyze" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/highlight" @@ -742,7 +741,7 @@ parsingLoop: diffLineTypeBuffers[DiffLineAdd] = new(bytes.Buffer) diffLineTypeBuffers[DiffLineDel] = new(bytes.Buffer) for _, f := range diff.Files { - f.NameHash = base.EncodeSha1(f.Name) + f.NameHash = git.HashFilePathForWebUI(f.Name) for _, buffer := range diffLineTypeBuffers { buffer.Reset()