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

11 files changed, 94 insertions, 35 deletions

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 9c37da4a3a..2d5aba7546 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 4a81814bae..9908386f0d 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 a9a7ba5948..1b8ebce432 100644
--- a/models/user.go
+++ b/models/user.go
@@ -532,7 +532,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)
 }
 
@@ -604,8 +604,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 49d1bf78b2..ce29354771 100644
--- a/models/user_mail.go
+++ b/models/user_mail.go
@@ -122,7 +122,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 1722c88ac8..eb25108869 100644
--- a/modules/base/tool.go
+++ b/modules/base/tool.go
@@ -13,6 +13,7 @@ import (
 	"fmt"
 	"html/template"
 	"math"
+	"math/big"
 	"net/http"
 	"strconv"
 	"strings"
@@ -81,18 +82,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
 }
 
 // VerifyTimeLimitCode verify time limit code
diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go
index ec839e5e10..2ca70b8b32 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/admin/users.go b/routers/admin/users.go
index c95aba7729..fa61a46938 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 0a6dc5d456..36fea14f11 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 84f07e1e69..f18a8dbca3 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,15 +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")
-	cfg.Section("security").Key("SECRET_KEY").SetValue(base.GetRandomString(15))
+	var secretKey string
+	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
 	}
@@ -303,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
@@ -316,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
 		}
diff --git a/routers/user/auth.go b/routers/user/auth.go
index eecb5e051f..bb14ad5a5f 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 bbb4d99c02..e078f8c17a 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)