Skip to content

Commit 380e32e

Browse files
denjitboerger
denji
authored and
tboerger
committed
Fix random string generator (#384)
* Remove unused custom-alphabet feature of random string generator Fix random string generator Random string generator should return error if it fails to read random data via crypto/rand * Fixes variable (un)initialization mixed assign Update test GetRandomString
1 parent 952587d commit 380e32e

File tree

11 files changed

+94
-35
lines changed

11 files changed

+94
-35
lines changed

‎models/migrations/migrations.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -457,8 +457,12 @@ func generateOrgRandsAndSalt(x *xorm.Engine) (err error) {
457457
}
458458

459459
for _, org := range orgs {
460-
org.Rands = base.GetRandomString(10)
461-
org.Salt = base.GetRandomString(10)
460+
if org.Rands, err = base.GetRandomString(10); err != nil {
461+
return err
462+
}
463+
if org.Salt, err = base.GetRandomString(10); err != nil {
464+
return err
465+
}
462466
if _, err = sess.Id(org.ID).Update(org); err != nil {
463467
return err
464468
}

‎models/org.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,12 @@ func CreateOrganization(org, owner *User) (err error) {
109109
}
110110

111111
org.LowerName = strings.ToLower(org.Name)
112-
org.Rands = GetUserSalt()
113-
org.Salt = GetUserSalt()
112+
if org.Rands, err = GetUserSalt(); err != nil {
113+
return err
114+
}
115+
if org.Salt, err = GetUserSalt(); err != nil {
116+
return err
117+
}
114118
org.UseCustomAvatar = true
115119
org.MaxRepoCreation = -1
116120
org.NumTeams = 1

‎models/user.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ func IsUserExist(uid int64, name string) (bool, error) {
532532
}
533533

534534
// GetUserSalt returns a ramdom user salt token.
535-
func GetUserSalt() string {
535+
func GetUserSalt() (string, error) {
536536
return base.GetRandomString(10)
537537
}
538538

@@ -604,8 +604,12 @@ func CreateUser(u *User) (err error) {
604604
u.LowerName = strings.ToLower(u.Name)
605605
u.AvatarEmail = u.Email
606606
u.Avatar = base.HashEmail(u.AvatarEmail)
607-
u.Rands = GetUserSalt()
608-
u.Salt = GetUserSalt()
607+
if u.Rands, err = GetUserSalt(); err != nil {
608+
return err
609+
}
610+
if u.Salt, err = GetUserSalt(); err != nil {
611+
return err
612+
}
609613
u.EncodePasswd()
610614
u.MaxRepoCreation = -1
611615

‎models/user_mail.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,9 @@ func (email *EmailAddress) Activate() error {
122122
if err != nil {
123123
return err
124124
}
125-
user.Rands = GetUserSalt()
125+
if user.Rands, err = GetUserSalt(); err != nil {
126+
return err
127+
}
126128

127129
sess := x.NewSession()
128130
defer sessionRelease(sess)

‎modules/base/tool.go

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"fmt"
1414
"html/template"
1515
"math"
16+
"math/big"
1617
"net/http"
1718
"strconv"
1819
"strings"
@@ -81,18 +82,31 @@ func BasicAuthEncode(username, password string) string {
8182
}
8283

8384
// GetRandomString generate random string by specify chars.
84-
func GetRandomString(n int, alphabets ...byte) string {
85+
func GetRandomString(n int) (string, error) {
8586
const alphanum = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
86-
var bytes = make([]byte, n)
87-
rand.Read(bytes)
88-
for i, b := range bytes {
89-
if len(alphabets) == 0 {
90-
bytes[i] = alphanum[b%byte(len(alphanum))]
91-
} else {
92-
bytes[i] = alphabets[b%byte(len(alphabets))]
87+
88+
buffer := make([]byte, n)
89+
max := big.NewInt(int64(len(alphanum)))
90+
91+
for i := 0; i < n; i++ {
92+
index, err := randomInt(max)
93+
if err != nil {
94+
return "", err
9395
}
96+
97+
buffer[i] = alphanum[index]
9498
}
95-
return string(bytes)
99+
100+
return string(buffer), nil
101+
}
102+
103+
func randomInt(max *big.Int) (int, error) {
104+
rand, err := rand.Int(rand.Reader, max)
105+
if err != nil {
106+
return 0, err
107+
}
108+
109+
return int(rand.Int64()), nil
96110
}
97111

98112
// VerifyTimeLimitCode verify time limit code

‎modules/base/tool_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ func TestBasicAuthEncode(t *testing.T) {
4343
}
4444

4545
func TestGetRandomString(t *testing.T) {
46-
assert.Len(t, GetRandomString(4), 4)
46+
randomString, err := GetRandomString(4)
47+
assert.NoError(t, err)
48+
assert.Len(t, randomString, 4)
4749
}
4850

4951
// TODO: Test PBKDF2()

‎routers/admin/users.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,11 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) {
197197

198198
if len(form.Password) > 0 {
199199
u.Passwd = form.Password
200-
u.Salt = models.GetUserSalt()
200+
var err error
201+
if u.Salt, err = models.GetUserSalt(); err != nil {
202+
ctx.Handle(500, "UpdateUser", err)
203+
return
204+
}
201205
u.EncodePasswd()
202206
}
203207

‎routers/api/v1/admin/user.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,11 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) {
8787

8888
if len(form.Password) > 0 {
8989
u.Passwd = form.Password
90-
u.Salt = models.GetUserSalt()
90+
var err error
91+
if u.Salt, err = models.GetUserSalt(); err != nil {
92+
ctx.Error(500, "UpdateUser", err)
93+
return
94+
}
9195
u.EncodePasswd()
9296
}
9397

‎routers/install.go

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ func Install(ctx *context.Context) {
115115

116116
// InstallPost response for submit install items
117117
func InstallPost(ctx *context.Context, form auth.InstallForm) {
118+
var err error
118119
ctx.Data["CurDbOption"] = form.DbType
119120

120121
if ctx.HasError() {
@@ -131,7 +132,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
131132
return
132133
}
133134

134-
if _, err := exec.LookPath("git"); err != nil {
135+
if _, err = exec.LookPath("git"); err != nil {
135136
ctx.RenderWithErr(ctx.Tr("install.test_git_failed", err), tplInstall, &form)
136137
return
137138
}
@@ -161,7 +162,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
161162

162163
// Set test engine.
163164
var x *xorm.Engine
164-
if err := models.NewTestEngine(x); err != nil {
165+
if err = models.NewTestEngine(x); err != nil {
165166
if strings.Contains(err.Error(), `Unknown database type: sqlite3`) {
166167
ctx.Data["Err_DbType"] = true
167168
ctx.RenderWithErr(ctx.Tr("install.sqlite3_not_available", "https://docs.gitea.io/installation/install_from_binary.html"), tplInstall, &form)
@@ -174,15 +175,15 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
174175

175176
// Test repository root path.
176177
form.RepoRootPath = strings.Replace(form.RepoRootPath, "\\", "/", -1)
177-
if err := os.MkdirAll(form.RepoRootPath, os.ModePerm); err != nil {
178+
if err = os.MkdirAll(form.RepoRootPath, os.ModePerm); err != nil {
178179
ctx.Data["Err_RepoRootPath"] = true
179180
ctx.RenderWithErr(ctx.Tr("install.invalid_repo_path", err), tplInstall, &form)
180181
return
181182
}
182183

183184
// Test log root path.
184185
form.LogRootPath = strings.Replace(form.LogRootPath, "\\", "/", -1)
185-
if err := os.MkdirAll(form.LogRootPath, os.ModePerm); err != nil {
186+
if err = os.MkdirAll(form.LogRootPath, os.ModePerm); err != nil {
186187
ctx.Data["Err_LogRootPath"] = true
187188
ctx.RenderWithErr(ctx.Tr("install.invalid_log_root_path", err), tplInstall, &form)
188189
return
@@ -225,7 +226,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
225226
cfg := ini.Empty()
226227
if com.IsFile(setting.CustomConf) {
227228
// Keeps custom settings if there is already something.
228-
if err := cfg.Append(setting.CustomConf); err != nil {
229+
if err = cfg.Append(setting.CustomConf); err != nil {
229230
log.Error(4, "Fail to load custom conf '%s': %v", setting.CustomConf, err)
230231
}
231232
}
@@ -279,15 +280,20 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
279280
cfg.Section("log").Key("ROOT_PATH").SetValue(form.LogRootPath)
280281

281282
cfg.Section("security").Key("INSTALL_LOCK").SetValue("true")
282-
cfg.Section("security").Key("SECRET_KEY").SetValue(base.GetRandomString(15))
283+
var secretKey string
284+
if secretKey, err = base.GetRandomString(10); err != nil {
285+
ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form)
286+
return
287+
}
288+
cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey)
283289

284-
err := os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm)
290+
err = os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm)
285291
if err != nil {
286292
ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form)
287293
return
288294
}
289295

290-
if err := cfg.SaveTo(setting.CustomConf); err != nil {
296+
if err = cfg.SaveTo(setting.CustomConf); err != nil {
291297
ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form)
292298
return
293299
}
@@ -303,7 +309,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
303309
IsAdmin: true,
304310
IsActive: true,
305311
}
306-
if err := models.CreateUser(u); err != nil {
312+
if err = models.CreateUser(u); err != nil {
307313
if !models.IsErrUserAlreadyExist(err) {
308314
setting.InstallLock = false
309315
ctx.Data["Err_AdminName"] = true
@@ -316,11 +322,11 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) {
316322
}
317323

318324
// Auto-login for admin
319-
if err := ctx.Session.Set("uid", u.ID); err != nil {
325+
if err = ctx.Session.Set("uid", u.ID); err != nil {
320326
ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form)
321327
return
322328
}
323-
if err := ctx.Session.Set("uname", u.Name); err != nil {
329+
if err = ctx.Session.Set("uname", u.Name); err != nil {
324330
ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form)
325331
return
326332
}

‎routers/user/auth.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,11 @@ func Activate(ctx *context.Context) {
289289
// Verify code.
290290
if user := models.VerifyUserActiveCode(code); user != nil {
291291
user.IsActive = true
292-
user.Rands = models.GetUserSalt()
292+
var err error
293+
if user.Rands, err = models.GetUserSalt(); err != nil {
294+
ctx.Handle(500, "UpdateUser", err)
295+
return
296+
}
293297
if err := models.UpdateUser(user); err != nil {
294298
if models.IsErrUserNotExist(err) {
295299
ctx.Error(404)
@@ -428,8 +432,15 @@ func ResetPasswdPost(ctx *context.Context) {
428432
}
429433

430434
u.Passwd = passwd
431-
u.Rands = models.GetUserSalt()
432-
u.Salt = models.GetUserSalt()
435+
var err error
436+
if u.Rands, err = models.GetUserSalt(); err != nil {
437+
ctx.Handle(500, "UpdateUser", err)
438+
return
439+
}
440+
if u.Salt, err = models.GetUserSalt(); err != nil {
441+
ctx.Handle(500, "UpdateUser", err)
442+
return
443+
}
433444
u.EncodePasswd()
434445
if err := models.UpdateUser(u); err != nil {
435446
ctx.Handle(500, "UpdateUser", err)

‎routers/user/setting.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,11 @@ func SettingsPasswordPost(ctx *context.Context, form auth.ChangePasswordForm) {
197197
ctx.Flash.Error(ctx.Tr("form.password_not_match"))
198198
} else {
199199
ctx.User.Passwd = form.Password
200-
ctx.User.Salt = models.GetUserSalt()
200+
var err error
201+
if ctx.User.Salt, err = models.GetUserSalt(); err != nil {
202+
ctx.Handle(500, "UpdateUser", err)
203+
return
204+
}
201205
ctx.User.EncodePasswd()
202206
if err := models.UpdateUser(ctx.User); err != nil {
203207
ctx.Handle(500, "UpdateUser", err)

0 commit comments

Comments
 (0)