Fix random string generator (#3953)

* Remove unused custom-alphabet feature of random string generator

* Fix modulo-biased random string generator

* Random string generator should return error if it fails to read random data via crypto/rand

models/migrations/migrations.go

@@ -451,8 +451,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
 		}

models/org.go

@@ -108,8 +108,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

models/user.go

@@ -474,7 +474,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)
 }
 
@@ -545,8 +545,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
 

models/user_mail.go

@@ -111,7 +111,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)

modules/base/tool.go

@@ -15,6 +15,7 @@ import (
 	"hash"
 	"html/template"
 	"math"
+	"math/big"
 	"net/http"
 	"strings"
 	"time"
@@ -82,18 +83,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(buffer), nil
+}
+
+func randomInt(max *big.Int) (int, error) {
+	rand, err := rand.Int(rand.Reader, max)
+	if err != nil {
+		return 0, err
 	}
-	return string(bytes)
+
+	return int(rand.Int64()), nil
 }
 
 // http://code.google.com/p/go/source/browse/pbkdf2/pbkdf2.go?repo=crypto

routers/admin/users.go

@@ -192,7 +192,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()
 	}
 

routers/api/v1/admin/user.go

@@ -85,7 +85,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()
 	}
 

routers/install.go

@@ -343,7 +343,12 @@ 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))
+	secretKey, err := base.GetRandomString(15)
+	if err != nil {
+		ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), INSTALL, &form)
+		return
+	}
+	cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey)
 
 	os.MkdirAll(filepath.Dir(setting.CustomConf), os.ModePerm)
 	if err := cfg.SaveTo(setting.CustomConf); err != nil {

routers/user/auth.go

@@ -273,7 +273,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)
@@ -407,8 +411,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)

routers/user/setting.go

@@ -189,7 +189,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)