From 9a742b564379acb96091a25ee4a4007158b88277 Mon Sep 17 00:00:00 2001 From: Leon Klingele Date: Wed, 14 Dec 2016 13:07:35 +0200 Subject: [PATCH 1/2] 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 --- models/migrations/migrations.go | 8 ++++++-- models/org.go | 8 ++++++-- models/user.go | 10 +++++++--- models/user_mail.go | 4 +++- modules/base/tool.go | 32 +++++++++++++++++++++++--------- routers/admin/users.go | 6 +++++- routers/api/v1/admin/user.go | 6 +++++- routers/install.go | 9 ++++++++- routers/user/auth.go | 17 ++++++++++++++--- routers/user/setting.go | 6 +++++- 10 files changed, 82 insertions(+), 24 deletions(-) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 9c37da4a3a9a9..2d5aba7546dfc 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -457,8 +457,12 @@ func generateOrgRandsAndSalt(x *xorm.Engine) (err error) { } for _, org := range orgs { - org.Rands = base.GetRandomString(10) - org.Salt = base.GetRandomString(10) + if org.Rands, err = base.GetRandomString(10); err != nil { + return err + } + if org.Salt, err = base.GetRandomString(10); err != nil { + return err + } if _, err = sess.Id(org.ID).Update(org); err != nil { return err } diff --git a/models/org.go b/models/org.go index 4a81814baec9e..9908386f0d1e6 100644 --- a/models/org.go +++ b/models/org.go @@ -109,8 +109,12 @@ func CreateOrganization(org, owner *User) (err error) { } org.LowerName = strings.ToLower(org.Name) - org.Rands = GetUserSalt() - org.Salt = GetUserSalt() + if org.Rands, err = GetUserSalt(); err != nil { + return err + } + if org.Salt, err = GetUserSalt(); err != nil { + return err + } org.UseCustomAvatar = true org.MaxRepoCreation = -1 org.NumTeams = 1 diff --git a/models/user.go b/models/user.go index 40afc484913e7..77a6a89071154 100644 --- a/models/user.go +++ b/models/user.go @@ -531,7 +531,7 @@ func IsUserExist(uid int64, name string) (bool, error) { } // GetUserSalt returns a ramdom user salt token. -func GetUserSalt() string { +func GetUserSalt() (string, error) { return base.GetRandomString(10) } @@ -603,8 +603,12 @@ func CreateUser(u *User) (err error) { u.LowerName = strings.ToLower(u.Name) u.AvatarEmail = u.Email u.Avatar = base.HashEmail(u.AvatarEmail) - u.Rands = GetUserSalt() - u.Salt = GetUserSalt() + if u.Rands, err = GetUserSalt(); err != nil { + return err + } + if u.Salt, err = GetUserSalt(); err != nil { + return err + } u.EncodePasswd() u.MaxRepoCreation = -1 diff --git a/models/user_mail.go b/models/user_mail.go index 69f87c2b3727d..0fe45c018f6e2 100644 --- a/models/user_mail.go +++ b/models/user_mail.go @@ -116,7 +116,9 @@ func (email *EmailAddress) Activate() error { if err != nil { return err } - user.Rands = GetUserSalt() + if user.Rands, err = GetUserSalt(); err != nil { + return err + } sess := x.NewSession() defer sessionRelease(sess) diff --git a/modules/base/tool.go b/modules/base/tool.go index 981f89b6a5cb0..c7948d2160e8e 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -15,6 +15,7 @@ import ( "hash" "html/template" "math" + "math/big" "net/http" "strconv" "strings" @@ -83,18 +84,31 @@ func BasicAuthEncode(username, password string) string { } // GetRandomString generate random string by specify chars. -func GetRandomString(n int, alphabets ...byte) string { +func GetRandomString(n int) (string, error) { const alphanum = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" - var bytes = make([]byte, n) - rand.Read(bytes) - for i, b := range bytes { - if len(alphabets) == 0 { - bytes[i] = alphanum[b%byte(len(alphanum))] - } else { - bytes[i] = alphabets[b%byte(len(alphabets))] + + buffer := make([]byte, n) + max := big.NewInt(int64(len(alphanum))) + + for i := 0; i < n; i++ { + index, err := randomInt(max) + if err != nil { + return "", err } + + buffer[i] = alphanum[index] } - return string(bytes) + + return string(buffer), nil +} + +func randomInt(max *big.Int) (int, error) { + rand, err := rand.Int(rand.Reader, max) + if err != nil { + return 0, err + } + + return int(rand.Int64()), nil } // PBKDF2 http://code.google.com/p/go/source/browse/pbkdf2/pbkdf2.go?repo=crypto diff --git a/routers/admin/users.go b/routers/admin/users.go index c95aba7729662..fa61a469385d0 100644 --- a/routers/admin/users.go +++ b/routers/admin/users.go @@ -197,7 +197,11 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) { if len(form.Password) > 0 { u.Passwd = form.Password - u.Salt = models.GetUserSalt() + var err error + if u.Salt, err = models.GetUserSalt(); err != nil { + ctx.Handle(500, "UpdateUser", err) + return + } u.EncodePasswd() } diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 0a6dc5d456d25..36fea14f115db 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -87,7 +87,11 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) { if len(form.Password) > 0 { u.Passwd = form.Password - u.Salt = models.GetUserSalt() + var err error + if u.Salt, err = models.GetUserSalt(); err != nil { + ctx.Error(500, "UpdateUser", err) + return + } u.EncodePasswd() } diff --git a/routers/install.go b/routers/install.go index 84f07e1e69d78..5422457c9bac8 100644 --- a/routers/install.go +++ b/routers/install.go @@ -279,7 +279,14 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { cfg.Section("log").Key("ROOT_PATH").SetValue(form.LogRootPath) cfg.Section("security").Key("INSTALL_LOCK").SetValue("true") - cfg.Section("security").Key("SECRET_KEY").SetValue(base.GetRandomString(15)) + + var secretKey string + secretKey, err := base.GetRandomString(10) + if err != nil { + ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form) + return + } + cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey) err := os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm) if err != nil { diff --git a/routers/user/auth.go b/routers/user/auth.go index eecb5e051f21e..bb14ad5a5f17c 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -289,7 +289,11 @@ func Activate(ctx *context.Context) { // Verify code. if user := models.VerifyUserActiveCode(code); user != nil { user.IsActive = true - user.Rands = models.GetUserSalt() + var err error + if user.Rands, err = models.GetUserSalt(); err != nil { + ctx.Handle(500, "UpdateUser", err) + return + } if err := models.UpdateUser(user); err != nil { if models.IsErrUserNotExist(err) { ctx.Error(404) @@ -428,8 +432,15 @@ func ResetPasswdPost(ctx *context.Context) { } u.Passwd = passwd - u.Rands = models.GetUserSalt() - u.Salt = models.GetUserSalt() + var err error + if u.Rands, err = models.GetUserSalt(); err != nil { + ctx.Handle(500, "UpdateUser", err) + return + } + if u.Salt, err = models.GetUserSalt(); err != nil { + ctx.Handle(500, "UpdateUser", err) + return + } u.EncodePasswd() if err := models.UpdateUser(u); err != nil { ctx.Handle(500, "UpdateUser", err) diff --git a/routers/user/setting.go b/routers/user/setting.go index 1d405fba375ea..194076259ceac 100644 --- a/routers/user/setting.go +++ b/routers/user/setting.go @@ -197,7 +197,11 @@ func SettingsPasswordPost(ctx *context.Context, form auth.ChangePasswordForm) { ctx.Flash.Error(ctx.Tr("form.password_not_match")) } else { ctx.User.Passwd = form.Password - ctx.User.Salt = models.GetUserSalt() + var err error + if ctx.User.Salt, err = models.GetUserSalt(); err != nil { + ctx.Handle(500, "UpdateUser", err) + return + } ctx.User.EncodePasswd() if err := models.UpdateUser(ctx.User); err != nil { ctx.Handle(500, "UpdateUser", err) From 76afd53c06b71478481b8a2f88e17f0ba6cf775c Mon Sep 17 00:00:00 2001 From: Denis Denisov Date: Sat, 17 Dec 2016 23:45:13 +0200 Subject: [PATCH 2/2] Fixes variable (un)initialization mixed assign Update test GetRandomString --- modules/base/tool_test.go | 4 +++- routers/install.go | 25 ++++++++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go index ec839e5e1073a..2ca70b8b32558 100644 --- a/modules/base/tool_test.go +++ b/modules/base/tool_test.go @@ -43,7 +43,9 @@ func TestBasicAuthEncode(t *testing.T) { } func TestGetRandomString(t *testing.T) { - assert.Len(t, GetRandomString(4), 4) + randomString, err := GetRandomString(4) + assert.NoError(t, err) + assert.Len(t, randomString, 4) } // TODO: Test PBKDF2() diff --git a/routers/install.go b/routers/install.go index 5422457c9bac8..f18a8dbca3ccd 100644 --- a/routers/install.go +++ b/routers/install.go @@ -115,6 +115,7 @@ func Install(ctx *context.Context) { // InstallPost response for submit install items func InstallPost(ctx *context.Context, form auth.InstallForm) { + var err error ctx.Data["CurDbOption"] = form.DbType if ctx.HasError() { @@ -131,7 +132,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { return } - if _, err := exec.LookPath("git"); err != nil { + if _, err = exec.LookPath("git"); err != nil { ctx.RenderWithErr(ctx.Tr("install.test_git_failed", err), tplInstall, &form) return } @@ -161,7 +162,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { // Set test engine. var x *xorm.Engine - if err := models.NewTestEngine(x); err != nil { + if err = models.NewTestEngine(x); err != nil { if strings.Contains(err.Error(), `Unknown database type: sqlite3`) { ctx.Data["Err_DbType"] = true ctx.RenderWithErr(ctx.Tr("install.sqlite3_not_available", "https://docs.gitea.io/installation/install_from_binary.html"), tplInstall, &form) @@ -174,7 +175,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { // Test repository root path. form.RepoRootPath = strings.Replace(form.RepoRootPath, "\\", "/", -1) - if err := os.MkdirAll(form.RepoRootPath, os.ModePerm); err != nil { + if err = os.MkdirAll(form.RepoRootPath, os.ModePerm); err != nil { ctx.Data["Err_RepoRootPath"] = true ctx.RenderWithErr(ctx.Tr("install.invalid_repo_path", err), tplInstall, &form) return @@ -182,7 +183,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { // Test log root path. form.LogRootPath = strings.Replace(form.LogRootPath, "\\", "/", -1) - if err := os.MkdirAll(form.LogRootPath, os.ModePerm); err != nil { + if err = os.MkdirAll(form.LogRootPath, os.ModePerm); err != nil { ctx.Data["Err_LogRootPath"] = true ctx.RenderWithErr(ctx.Tr("install.invalid_log_root_path", err), tplInstall, &form) return @@ -225,7 +226,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { cfg := ini.Empty() if com.IsFile(setting.CustomConf) { // Keeps custom settings if there is already something. - if err := cfg.Append(setting.CustomConf); err != nil { + if err = cfg.Append(setting.CustomConf); err != nil { log.Error(4, "Fail to load custom conf '%s': %v", setting.CustomConf, err) } } @@ -279,22 +280,20 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { cfg.Section("log").Key("ROOT_PATH").SetValue(form.LogRootPath) cfg.Section("security").Key("INSTALL_LOCK").SetValue("true") - var secretKey string - secretKey, err := base.GetRandomString(10) - if err != nil { + if secretKey, err = base.GetRandomString(10); err != nil { ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form) return } cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey) - err := os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm) + err = os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm) if err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } - if err := cfg.SaveTo(setting.CustomConf); err != nil { + if err = cfg.SaveTo(setting.CustomConf); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } @@ -310,7 +309,7 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { IsAdmin: true, IsActive: true, } - if err := models.CreateUser(u); err != nil { + if err = models.CreateUser(u); err != nil { if !models.IsErrUserAlreadyExist(err) { setting.InstallLock = false ctx.Data["Err_AdminName"] = true @@ -323,11 +322,11 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { } // Auto-login for admin - if err := ctx.Session.Set("uid", u.ID); err != nil { + if err = ctx.Session.Set("uid", u.ID); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } - if err := ctx.Session.Set("uname", u.Name); err != nil { + if err = ctx.Session.Set("uname", u.Name); err != nil { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return }