From 626086906879c51d473b5ed9ab4e8b5c007c5dcb Mon Sep 17 00:00:00 2001 From: Daniel Oaks Date: Sun, 1 Apr 2018 17:12:41 +1000 Subject: [PATCH 1/4] Upgrade password hashing. Previously, we generated and prepended a long salt before generating password hashes. This resulted in the hash verification cutting off long before it should do. This form of salting is also not necessary with bcrypt as it's provided by the password hashing and verification functions themselves, so totally rip it out. This commit also adds the functionality for the server to automagically upgrade users to use the new hashing system, which means better security and more assurance that people can't bruteforce passwords. No need to apply a database upgrade to do this, whoo! \o/ --- irc/accounts.go | 55 ++++++++++++++++++++++++++++++++++++------ irc/errors.go | 13 +++++----- irc/passwd/unsalted.go | 15 +++++++----- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/irc/accounts.go b/irc/accounts.go index b663d525..9b4905ea 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -189,15 +189,11 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames certFPKey := fmt.Sprintf(keyCertToAccount, certfp) var creds AccountCredentials - // always set passphrase salt - creds.PassphraseSalt, err = passwd.NewSalt() - if err != nil { - return errAccountCreation - } // it's fine if this is empty, that just means no certificate is authorized creds.Certificate = certfp if passphrase != "" { - creds.PassphraseHash, err = am.server.passwords.GenerateFromPassword(creds.PassphraseSalt, passphrase) + creds.PassphraseHash, err = passwd.GenerateEncodedPasswordBytes(passphrase) + creds.PassphraseIsV2 = true if err != nil { am.server.logger.Error("internal", fmt.Sprintf("could not hash password: %v", err)) return errAccountCreation @@ -522,8 +518,50 @@ func (am *AccountManager) AuthenticateByPassphrase(client *Client, accountName s return errAccountUnverified } - err = am.server.passwords.CompareHashAndPassword( - account.Credentials.PassphraseHash, account.Credentials.PassphraseSalt, passphrase) + if account.Credentials.PassphraseIsV2 { + err = passwd.ComparePassword(account.Credentials.PassphraseHash, []byte(passphrase)) + } else { + // compare using legacy method + err = am.server.passwords.CompareHashAndPassword(account.Credentials.PassphraseHash, account.Credentials.PassphraseSalt, passphrase) + if err == nil { + // passphrase worked! silently upgrade them to use v2 hashing going forward. + //TODO(dan): in future, replace this with an am.updatePassphrase(blah) function, which we can reuse in /ns update pass? + err = am.server.store.Update(func(tx *buntdb.Tx) error { + var creds AccountCredentials + creds.Certificate = account.Credentials.Certificate + creds.PassphraseHash, err = passwd.GenerateEncodedPasswordBytes(passphrase) + creds.PassphraseIsV2 = true + if err != nil { + am.server.logger.Error("internal", fmt.Sprintf("could not hash password (updating existing hash version): %v", err)) + return errAccountCredUpdate + } + + credText, err := json.Marshal(creds) + if err != nil { + am.server.logger.Error("internal", fmt.Sprintf("could not marshal credentials (updating existing hash version): %v", err)) + return errAccountCredUpdate + } + credStr := string(credText) + + // we know the account name is valid if this line is reached, otherwise the + // above would have failed. as such, chuck out and ignore err on casefolding + casefoldedAccountName, _ := CasefoldName(accountName) + credentialsKey := fmt.Sprintf(keyAccountCredentials, casefoldedAccountName) + + //TODO(dan): sling, can you please checkout this mutex usage, see if it + // makes sense or not? bleh + am.serialCacheUpdateMutex.Lock() + defer am.serialCacheUpdateMutex.Unlock() + + tx.Set(credentialsKey, credStr, nil) + + return nil + }) + } + if err != nil { + return err + } + } if err != nil { return errAccountInvalidCredentials } @@ -984,6 +1022,7 @@ var ( type AccountCredentials struct { PassphraseSalt []byte PassphraseHash []byte + PassphraseIsV2 bool `json:"passphrase-is-v2"` Certificate string // fingerprint } diff --git a/irc/errors.go b/irc/errors.go index 761f6a00..94a9ad69 100644 --- a/irc/errors.go +++ b/irc/errors.go @@ -10,17 +10,18 @@ import "errors" // Runtime Errors var ( errAccountAlreadyRegistered = errors.New("Account already exists") + errAccountAlreadyVerified = errors.New("Account is already verified") + errAccountCantDropPrimaryNick = errors.New("Can't unreserve primary nickname") errAccountCreation = errors.New("Account could not be created") + errAccountCredUpdate = errors.New("Could not update password hash to new method") errAccountDoesNotExist = errors.New("Account does not exist") + errAccountInvalidCredentials = errors.New("Invalid account credentials") + errAccountNickReservationFailed = errors.New("Could not (un)reserve nick") errAccountNotLoggedIn = errors.New("You're not logged into an account") + errAccountTooManyNicks = errors.New("Account has too many reserved nicks") + errAccountUnverified = errors.New("Account is not yet verified") errAccountVerificationFailed = errors.New("Account verification failed") errAccountVerificationInvalidCode = errors.New("Invalid account verification code") - errAccountUnverified = errors.New("Account is not yet verified") - errAccountAlreadyVerified = errors.New("Account is already verified") - errAccountInvalidCredentials = errors.New("Invalid account credentials") - errAccountTooManyNicks = errors.New("Account has too many reserved nicks") - errAccountNickReservationFailed = errors.New("Could not (un)reserve nick") - errAccountCantDropPrimaryNick = errors.New("Can't unreserve primary nickname") errAccountUpdateFailed = errors.New("Error while updating your account information") errCallbackFailed = errors.New("Account verification could not be sent") errCertfpAlreadyExists = errors.New("An account already exists with your certificate") diff --git a/irc/passwd/unsalted.go b/irc/passwd/unsalted.go index 0c8478e1..2c23fd26 100644 --- a/irc/passwd/unsalted.go +++ b/irc/passwd/unsalted.go @@ -15,16 +15,19 @@ var ( ErrEmptyPassword = errors.New("empty password") ) -// GenerateEncodedPassword returns an encrypted password, encoded into a string with base64. -func GenerateEncodedPassword(passwd string) (encoded string, err error) { +// GenerateEncodedPasswordBytes returns an encrypted password, returning the bytes directly. +func GenerateEncodedPasswordBytes(passwd string) (encoded []byte, err error) { if passwd == "" { err = ErrEmptyPassword return } - bcrypted, err := bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.MinCost) - if err != nil { - return - } + encoded, err = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.MinCost) + return +} + +// GenerateEncodedPassword returns an encrypted password, encoded into a string with base64. +func GenerateEncodedPassword(passwd string) (encoded string, err error) { + bcrypted, err := GenerateEncodedPasswordBytes(passwd) encoded = base64.StdEncoding.EncodeToString(bcrypted) return } From dfb0a570403bc90dd21dca3cc6a0e53aee67f46d Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Sun, 5 Aug 2018 22:51:39 -0400 Subject: [PATCH 2/4] refactor the password hashing / password autoupgrade system --- Makefile | 1 + irc/accounts.go | 148 +++++++++++++++++++++--------------- irc/config.go | 23 +++--- irc/database.go | 12 --- irc/errors.go | 1 + irc/gateways.go | 3 +- irc/handlers.go | 7 +- irc/legacy.go | 72 ++++++++++++++++++ irc/nickserv.go | 2 + irc/passwd/bcrypt.go | 34 +++++++++ irc/passwd/bcrypt_test.go | 58 ++++++++++++++ irc/passwd/salted.go | 66 ---------------- irc/passwd/salted_test.go | 86 --------------------- irc/passwd/unsalted.go | 53 ------------- irc/passwd/unsalted_test.go | 54 ------------- irc/server.go | 24 ------ oragono.go | 6 +- oragono.yaml | 7 +- 18 files changed, 277 insertions(+), 380 deletions(-) create mode 100644 irc/legacy.go create mode 100644 irc/passwd/bcrypt.go create mode 100644 irc/passwd/bcrypt_test.go delete mode 100644 irc/passwd/salted.go delete mode 100644 irc/passwd/salted_test.go delete mode 100644 irc/passwd/unsalted.go delete mode 100644 irc/passwd/unsalted_test.go diff --git a/Makefile b/Makefile index 32ea0c27..540ac727 100644 --- a/Makefile +++ b/Makefile @@ -22,5 +22,6 @@ test: cd irc/caps && go test . && go vet . cd irc/isupport && go test . && go vet . cd irc/modes && go test . && go vet . + cd irc/passwd && go test . && go vet . cd irc/utils && go test . && go vet . ./.check-gofmt.sh diff --git a/irc/accounts.go b/irc/accounts.go index 9b4905ea..910a3025 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -16,6 +16,7 @@ import ( "sync" "sync/atomic" "time" + "unicode" "github.com/oragono/oragono/irc/caps" "github.com/oragono/oragono/irc/passwd" @@ -175,7 +176,8 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames } // can't register a guest nickname - renamePrefix := strings.ToLower(am.server.AccountConfig().NickReservation.RenamePrefix) + config := am.server.AccountConfig() + renamePrefix := strings.ToLower(config.NickReservation.RenamePrefix) if renamePrefix != "" && strings.HasPrefix(casefoldedAccount, renamePrefix) { return errAccountAlreadyRegistered } @@ -188,30 +190,16 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames verificationCodeKey := fmt.Sprintf(keyAccountVerificationCode, casefoldedAccount) certFPKey := fmt.Sprintf(keyCertToAccount, certfp) - var creds AccountCredentials - // it's fine if this is empty, that just means no certificate is authorized - creds.Certificate = certfp - if passphrase != "" { - creds.PassphraseHash, err = passwd.GenerateEncodedPasswordBytes(passphrase) - creds.PassphraseIsV2 = true - if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not hash password: %v", err)) - return errAccountCreation - } - } - - credText, err := json.Marshal(creds) + credStr, err := am.serializeCredentials(passphrase, certfp) if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not marshal credentials: %v", err)) - return errAccountCreation + return err } - credStr := string(credText) registeredTimeStr := strconv.FormatInt(time.Now().Unix(), 10) callbackSpec := fmt.Sprintf("%s:%s", callbackNamespace, callbackValue) var setOptions *buntdb.SetOptions - ttl := am.server.AccountConfig().Registration.VerifyTimeout + ttl := config.Registration.VerifyTimeout if ttl != 0 { setOptions = &buntdb.SetOptions{Expires: true, TTL: ttl} } @@ -267,6 +255,75 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames } } +// validatePassphrase checks whether a passphrase is allowed by our rules +func validatePassphrase(passphrase string) error { + // sanity check the length + if len(passphrase) == 0 || len(passphrase) > 600 { + return errAccountBadPassphrase + } + // for now, just enforce that spaces are not allowed + for _, r := range passphrase { + if unicode.IsSpace(r) { + return errAccountBadPassphrase + } + } + return nil +} + +// helper to assemble the serialized JSON for an account's credentials +func (am *AccountManager) serializeCredentials(passphrase string, certfp string) (result string, err error) { + var creds AccountCredentials + creds.Version = 1 + // we need at least one of passphrase and certfp: + if passphrase == "" && certfp == "" { + return "", errAccountBadPassphrase + } + // but if we have one, it's fine if the other is missing, it just means no + // credential of that type will be accepted. + creds.Certificate = certfp + if passphrase != "" { + if validatePassphrase(passphrase) != nil { + return "", errAccountBadPassphrase + } + bcryptCost := int(am.server.Config().Accounts.Registration.BcryptCost) + creds.PassphraseHash, err = passwd.GenerateFromPassword([]byte(passphrase), bcryptCost) + if err != nil { + am.server.logger.Error("internal", fmt.Sprintf("could not hash password: %v", err)) + return "", errAccountCreation + } + } + + credText, err := json.Marshal(creds) + if err != nil { + am.server.logger.Error("internal", fmt.Sprintf("could not marshal credentials: %v", err)) + return "", errAccountCreation + } + return string(credText), nil +} + +// changes the password for an account +func (am *AccountManager) setPassword(account string, password string) (err error) { + casefoldedAccount, err := CasefoldName(account) + if err != nil { + return err + } + act, err := am.LoadAccount(casefoldedAccount) + if err != nil { + return err + } + + credStr, err := am.serializeCredentials(password, act.Credentials.Certificate) + if err != nil { + return err + } + + credentialsKey := fmt.Sprintf(keyAccountCredentials, casefoldedAccount) + return am.server.store.Update(func(tx *buntdb.Tx) error { + _, _, err := tx.Set(credentialsKey, credStr, nil) + return err + }) +} + func (am *AccountManager) dispatchCallback(client *Client, casefoldedAccount string, callbackNamespace string, callbackValue string) (string, error) { if callbackNamespace == "*" || callbackNamespace == "none" { return "", nil @@ -518,50 +575,15 @@ func (am *AccountManager) AuthenticateByPassphrase(client *Client, accountName s return errAccountUnverified } - if account.Credentials.PassphraseIsV2 { - err = passwd.ComparePassword(account.Credentials.PassphraseHash, []byte(passphrase)) - } else { - // compare using legacy method - err = am.server.passwords.CompareHashAndPassword(account.Credentials.PassphraseHash, account.Credentials.PassphraseSalt, passphrase) - if err == nil { - // passphrase worked! silently upgrade them to use v2 hashing going forward. - //TODO(dan): in future, replace this with an am.updatePassphrase(blah) function, which we can reuse in /ns update pass? - err = am.server.store.Update(func(tx *buntdb.Tx) error { - var creds AccountCredentials - creds.Certificate = account.Credentials.Certificate - creds.PassphraseHash, err = passwd.GenerateEncodedPasswordBytes(passphrase) - creds.PassphraseIsV2 = true - if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not hash password (updating existing hash version): %v", err)) - return errAccountCredUpdate - } - - credText, err := json.Marshal(creds) - if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not marshal credentials (updating existing hash version): %v", err)) - return errAccountCredUpdate - } - credStr := string(credText) - - // we know the account name is valid if this line is reached, otherwise the - // above would have failed. as such, chuck out and ignore err on casefolding - casefoldedAccountName, _ := CasefoldName(accountName) - credentialsKey := fmt.Sprintf(keyAccountCredentials, casefoldedAccountName) - - //TODO(dan): sling, can you please checkout this mutex usage, see if it - // makes sense or not? bleh - am.serialCacheUpdateMutex.Lock() - defer am.serialCacheUpdateMutex.Unlock() - - tx.Set(credentialsKey, credStr, nil) - - return nil - }) - } - if err != nil { - return err - } + switch account.Credentials.Version { + case 0: + err = handleLegacyPasswordV0(am.server, accountName, account.Credentials, passphrase) + case 1: + err = passwd.CompareHashAndPassword(account.Credentials.PassphraseHash, []byte(passphrase)) + default: + err = errAccountInvalidCredentials } + if err != nil { return errAccountInvalidCredentials } @@ -1020,9 +1042,9 @@ var ( // AccountCredentials stores the various methods for verifying accounts. type AccountCredentials struct { - PassphraseSalt []byte + Version uint + PassphraseSalt []byte // legacy field, not used by v1 and later PassphraseHash []byte - PassphraseIsV2 bool `json:"passphrase-is-v2"` Certificate string // fingerprint } diff --git a/irc/config.go b/irc/config.go index e3680ad1..1f1c6bf1 100644 --- a/irc/config.go +++ b/irc/config.go @@ -82,6 +82,7 @@ type AccountRegistrationConfig struct { } } AllowMultiplePerConnection bool `yaml:"allow-multiple-per-connection"` + BcryptCost uint `yaml:"bcrypt-cost"` } type VHostConfig struct { @@ -152,15 +153,6 @@ type OperConfig struct { Modes string } -// PasswordBytes returns the bytes represented by the password hash. -func (conf *OperConfig) PasswordBytes() []byte { - bytes, err := passwd.DecodePasswordHash(conf.Password) - if err != nil { - log.Fatal("decode password error: ", err) - } - return bytes -} - // LineLenConfig controls line lengths. type LineLenLimits struct { Tags int @@ -384,7 +376,11 @@ func (conf *Config) Operators(oc map[string]*OperClass) (map[string]*Oper, error } oper.Name = name - oper.Pass = opConf.PasswordBytes() + oper.Pass, err = decodeLegacyPasswordHash(opConf.Password) + if err != nil { + return nil, err + } + oper.Vhost = opConf.Vhost class, exists := oc[opConf.Class] if !exists { @@ -713,11 +709,14 @@ func LoadConfig(filename string) (config *Config, err error) { config.Channels.defaultModes = ParseDefaultChannelModes(config.Channels.RawDefaultModes) if config.Server.Password != "" { - bytes, err := passwd.DecodePasswordHash(config.Server.Password) + config.Server.passwordBytes, err = decodeLegacyPasswordHash(config.Server.Password) if err != nil { return nil, err } - config.Server.passwordBytes = bytes + } + + if config.Accounts.Registration.BcryptCost == 0 { + config.Accounts.Registration.BcryptCost = passwd.DefaultCost } return config, nil diff --git a/irc/database.go b/irc/database.go index 99baa5ab..b617fe3f 100644 --- a/irc/database.go +++ b/irc/database.go @@ -5,7 +5,6 @@ package irc import ( - "encoding/base64" "encoding/json" "fmt" "log" @@ -14,7 +13,6 @@ import ( "time" "github.com/oragono/oragono/irc/modes" - "github.com/oragono/oragono/irc/passwd" "github.com/oragono/oragono/irc/utils" "github.com/tidwall/buntdb" @@ -25,8 +23,6 @@ const ( keySchemaVersion = "db.version" // latest schema of the db latestDbSchema = "3" - // key for the primary salt used by the ircd - keySalt = "crypto.salt" ) type SchemaChanger func(*Config, *buntdb.Tx) error @@ -68,14 +64,6 @@ func InitDB(path string) { defer store.Close() err = store.Update(func(tx *buntdb.Tx) error { - // set base db salt - salt, err := passwd.NewSalt() - encodedSalt := base64.StdEncoding.EncodeToString(salt) - if err != nil { - log.Fatal("Could not generate cryptographically-secure salt for the user:", err.Error()) - } - tx.Set(keySalt, encodedSalt, nil) - // set schema version tx.Set(keySchemaVersion, latestDbSchema, nil) return nil diff --git a/irc/errors.go b/irc/errors.go index 94a9ad69..fe117080 100644 --- a/irc/errors.go +++ b/irc/errors.go @@ -16,6 +16,7 @@ var ( errAccountCredUpdate = errors.New("Could not update password hash to new method") errAccountDoesNotExist = errors.New("Account does not exist") errAccountInvalidCredentials = errors.New("Invalid account credentials") + errAccountBadPassphrase = errors.New("Passphrase contains forbidden characters or is otherwise invalid") errAccountNickReservationFailed = errors.New("Could not (un)reserve nick") errAccountNotLoggedIn = errors.New("You're not logged into an account") errAccountTooManyNicks = errors.New("Account has too many reserved nicks") diff --git a/irc/gateways.go b/irc/gateways.go index f30b4600..9d5d584a 100644 --- a/irc/gateways.go +++ b/irc/gateways.go @@ -10,7 +10,6 @@ import ( "net" "github.com/oragono/oragono/irc/modes" - "github.com/oragono/oragono/irc/passwd" "github.com/oragono/oragono/irc/utils" ) @@ -29,7 +28,7 @@ func (wc *webircConfig) Populate() (err error) { if wc.PasswordString != "" { var password []byte - password, err = passwd.DecodePasswordHash(wc.PasswordString) + wc.Password, err = decodeLegacyPasswordHash(wc.PasswordString) wc.Password = password } return err diff --git a/irc/handlers.go b/irc/handlers.go index 789cbdf8..2e4e7b6c 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -27,7 +27,6 @@ import ( "github.com/oragono/oragono/irc/caps" "github.com/oragono/oragono/irc/custime" "github.com/oragono/oragono/irc/modes" - "github.com/oragono/oragono/irc/passwd" "github.com/oragono/oragono/irc/sno" "github.com/oragono/oragono/irc/utils" "github.com/tidwall/buntdb" @@ -159,6 +158,8 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r } else if err == errAccountAlreadyRegistered { msg = "Account already exists" code = ERR_ACCOUNT_ALREADY_EXISTS + } else if err == errAccountBadPassphrase { + msg = "Passphrase contains forbidden characters or is otherwise invalid" } if err == errAccountAlreadyRegistered || err == errAccountCreation || err == errCertfpAlreadyExists { msg = err.Error() @@ -1822,7 +1823,7 @@ func passHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Resp // check the provided password password := []byte(msg.Params[0]) - if passwd.ComparePassword(serverPassword, password) != nil { + if bcrypt.CompareHashAndPassword(serverPassword, password) != nil { rb.Add(nil, server.name, ERR_PASSWDMISMATCH, client.nick, client.t("Password incorrect")) rb.Add(nil, server.name, "ERROR", client.t("Password incorrect")) return true @@ -2406,7 +2407,7 @@ func webircHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Re if isGatewayAllowed(client.socket.conn.RemoteAddr(), gateway) { // confirm password and/or fingerprint givenPassword := msg.Params[0] - if 0 < len(info.Password) && passwd.ComparePasswordString(info.Password, givenPassword) != nil { + if 0 < len(info.Password) && bcrypt.CompareHashAndPassword(info.Password, []byte(givenPassword)) != nil { continue } if 0 < len(info.Fingerprint) && client.certfp != info.Fingerprint { diff --git a/irc/legacy.go b/irc/legacy.go new file mode 100644 index 00000000..e320b21f --- /dev/null +++ b/irc/legacy.go @@ -0,0 +1,72 @@ +// Copyright (c) 2018 Shivaram Lingamneni + +package irc + +import ( + "encoding/base64" + "errors" + "fmt" + + "github.com/tidwall/buntdb" + "golang.org/x/crypto/bcrypt" +) + +var ( + errInvalidPasswordHash = errors.New("invalid password hash") +) + +// Decode a hashed passphrase as it would appear in a config file, +// retaining compatibility with old versions of `oragono genpasswd` +// that used to apply a redundant layer of base64 +func decodeLegacyPasswordHash(hash string) ([]byte, error) { + // a correctly formatted bcrypt hash is 60 bytes of printable ASCII + if len(hash) == 80 { + // double-base64, remove the outer layer: + return base64.StdEncoding.DecodeString(hash) + } else if len(hash) == 60 { + return []byte(hash), nil + } else { + return nil, errInvalidPasswordHash + } +} + +// helper to check a version 0 password hash, with global and per-passphrase salts +func checkLegacyPasswordV0(hashedPassword, globalSalt, passphraseSalt []byte, passphrase string) error { + var assembledPasswordBytes []byte + assembledPasswordBytes = append(assembledPasswordBytes, globalSalt...) + assembledPasswordBytes = append(assembledPasswordBytes, '-') + assembledPasswordBytes = append(assembledPasswordBytes, passphraseSalt...) + assembledPasswordBytes = append(assembledPasswordBytes, '-') + assembledPasswordBytes = append(assembledPasswordBytes, []byte(passphrase)...) + return bcrypt.CompareHashAndPassword(hashedPassword, assembledPasswordBytes) +} + +// checks a version 0 password hash; if successful, upgrades the database entry to version 1 +func handleLegacyPasswordV0(server *Server, account string, credentials AccountCredentials, passphrase string) (err error) { + var globalSaltString string + err = server.store.View(func(tx *buntdb.Tx) (err error) { + globalSaltString, err = tx.Get("crypto.salt") + return err + }) + if err != nil { + return err + } + globalSalt, err := base64.StdEncoding.DecodeString(globalSaltString) + if err != nil { + return err + } + + err = checkLegacyPasswordV0(credentials.PassphraseHash, globalSalt, credentials.PassphraseSalt, passphrase) + if err != nil { + // invalid password + return err + } + + // upgrade credentials + err = server.accounts.setPassword(account, passphrase) + if err != nil { + server.logger.Error("internal", fmt.Sprintf("could not upgrade user password: %v", err)) + } + + return nil +} diff --git a/irc/nickserv.go b/irc/nickserv.go index fda9669d..b311b6ac 100644 --- a/irc/nickserv.go +++ b/irc/nickserv.go @@ -312,6 +312,8 @@ func nsRegisterHandler(server *Server, client *Client, command, params string, r errMsg = client.t("An account already exists for your certificate fingerprint") } else if err == errAccountAlreadyRegistered { errMsg = client.t("Account already exists") + } else if err == errAccountBadPassphrase { + errMsg = client.t("Passphrase contains forbidden characters or is otherwise invalid") } nsNotice(rb, errMsg) return diff --git a/irc/passwd/bcrypt.go b/irc/passwd/bcrypt.go new file mode 100644 index 00000000..d4daf298 --- /dev/null +++ b/irc/passwd/bcrypt.go @@ -0,0 +1,34 @@ +// Copyright (c) 2018 Shivaram Lingamneni +// released under the MIT license + +package passwd + +import "golang.org/x/crypto/bcrypt" +import "golang.org/x/crypto/sha3" + +const ( + MinCost = bcrypt.MinCost + DefaultCost = 12 // ballpark: 250 msec on a modern Intel CPU +) + +// implements Dropbox's strategy of applying an initial pass of a "normal" +// (i.e., fast) cryptographically secure hash with 512 bits of output before +// applying bcrypt. This allows the use of, e.g., Diceware/XKCD-style passphrases +// that may be longer than the 80-character bcrypt limit. +// https://blogs.dropbox.com/tech/2016/09/how-dropbox-securely-stores-your-passwords/ + +// we are only using this for user-generated passwords, as opposed to the server +// and operator passwords that are hashed by `oragono genpasswd` and then +// hard-coded by the server admins into the config file, to avoid breaking +// backwards compatibility (since we can't upgrade the config file on the fly +// the way we can with the database). + +func GenerateFromPassword(password []byte, cost int) (result []byte, err error) { + sum := sha3.Sum512(password) + return bcrypt.GenerateFromPassword(sum[:], cost) +} + +func CompareHashAndPassword(hashedPassword, password []byte) error { + sum := sha3.Sum512(password) + return bcrypt.CompareHashAndPassword(hashedPassword, sum[:]) +} diff --git a/irc/passwd/bcrypt_test.go b/irc/passwd/bcrypt_test.go new file mode 100644 index 00000000..8849ab7d --- /dev/null +++ b/irc/passwd/bcrypt_test.go @@ -0,0 +1,58 @@ +// Copyright (c) 2018 Shivaram Lingamneni +// released under the MIT license + +package passwd + +import ( + "testing" +) + +func TestBasic(t *testing.T) { + hash, err := GenerateFromPassword([]byte("this is my passphrase"), DefaultCost) + if err != nil || len(hash) != 60 { + t.Errorf("bad password hash output: error %s, output %s, len %d", err, hash, len(hash)) + } + + if CompareHashAndPassword(hash, []byte("this is my passphrase")) != nil { + t.Errorf("hash comparison failed unexpectedly") + } + + if CompareHashAndPassword(hash, []byte("this is not my passphrase")) == nil { + t.Errorf("hash comparison succeeded unexpectedly") + } +} + +func TestLongPassphrases(t *testing.T) { + longPassphrase := make([]byte, 168) + for i := range longPassphrase { + longPassphrase[i] = 'a' + } + hash, err := GenerateFromPassword(longPassphrase, DefaultCost) + if err != nil { + t.Errorf("bad password hash output: error %s", err) + } + + if CompareHashAndPassword(hash, longPassphrase) != nil { + t.Errorf("hash comparison failed unexpectedly") + } + + // change a byte of the passphrase beyond the normal 80-character + // bcrypt truncation boundary: + longPassphrase[150] = 'b' + if CompareHashAndPassword(hash, longPassphrase) == nil { + t.Errorf("hash comparison succeeded unexpectedly") + } +} + +// this could be useful for tuning the cost parameter on specific hardware +func BenchmarkComparisons(b *testing.B) { + pass := []byte("passphrase for benchmarking") + hash, err := GenerateFromPassword(pass, DefaultCost) + if err != nil { + b.Errorf("bad output") + } + b.ResetTimer() + for i := 0; i < b.N; i++ { + CompareHashAndPassword(hash, pass) + } +} diff --git a/irc/passwd/salted.go b/irc/passwd/salted.go deleted file mode 100644 index decf9190..00000000 --- a/irc/passwd/salted.go +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright (c) 2016 Daniel Oaks -// released under the MIT license - -package passwd - -import ( - "crypto/rand" - - "golang.org/x/crypto/bcrypt" -) - -const ( - // newSaltLen is how many bytes long newly-generated salts are. - newSaltLen = 30 - // defaultPasswordCost is the bcrypt cost we use for passwords. - defaultPasswordCost = 14 -) - -// NewSalt returns a salt for crypto uses. -func NewSalt() ([]byte, error) { - salt := make([]byte, newSaltLen) - _, err := rand.Read(salt) - - if err != nil { - var emptySalt []byte - return emptySalt, err - } - - return salt, nil -} - -// SaltedManager supports the hashing and comparing of passwords with the given salt. -type SaltedManager struct { - salt []byte -} - -// NewSaltedManager returns a new SaltedManager with the given salt. -func NewSaltedManager(salt []byte) SaltedManager { - return SaltedManager{ - salt: salt, - } -} - -// assemblePassword returns an assembled slice of bytes for the given password details. -func (sm *SaltedManager) assemblePassword(specialSalt []byte, password string) []byte { - var assembledPasswordBytes []byte - assembledPasswordBytes = append(assembledPasswordBytes, sm.salt...) - assembledPasswordBytes = append(assembledPasswordBytes, '-') - assembledPasswordBytes = append(assembledPasswordBytes, specialSalt...) - assembledPasswordBytes = append(assembledPasswordBytes, '-') - assembledPasswordBytes = append(assembledPasswordBytes, []byte(password)...) - return assembledPasswordBytes -} - -// GenerateFromPassword encrypts the given password. -func (sm *SaltedManager) GenerateFromPassword(specialSalt []byte, password string) ([]byte, error) { - assembledPasswordBytes := sm.assemblePassword(specialSalt, password) - return bcrypt.GenerateFromPassword(assembledPasswordBytes, defaultPasswordCost) -} - -// CompareHashAndPassword compares a hashed password with its possible plaintext equivalent. -// Returns nil on success, or an error on failure. -func (sm *SaltedManager) CompareHashAndPassword(hashedPassword []byte, specialSalt []byte, password string) error { - assembledPasswordBytes := sm.assemblePassword(specialSalt, password) - return bcrypt.CompareHashAndPassword(hashedPassword, assembledPasswordBytes) -} diff --git a/irc/passwd/salted_test.go b/irc/passwd/salted_test.go deleted file mode 100644 index 68ddc796..00000000 --- a/irc/passwd/salted_test.go +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright (c) 2016 Daniel Oaks -// released under the MIT license - -package passwd - -import ( - "encoding/base64" - "testing" -) - -type SaltedPasswordTest struct { - ManagerSalt string - Salt string - Hash string - Password string -} - -var SaltedPasswords = []SaltedPasswordTest{ - { - ManagerSalt: "3TPITDVf/NGb4OlCyV1uZNW1H7zy3BFos+Dsu7dj", - Salt: "b6oVqshJUfcm1zWEtqwKqUVylqLONAZfqt17ns+Y", - Hash: "JDJhJDE0JFYuT28xOFFNZldaaTI1UWpzNENMeHVKdm5vS1lkL2tFL1lFVkQ2a0loUEY2Vzk3UTZSVDVP", - Password: "test", - }, - { - ManagerSalt: "iNGeNEfuPihM8kYDZ/C6qAJ0JERKeKkUYp6wYDU0", - Salt: "U7TA6k6VLSLHfdjSsQH0vc3Jqq6cUezJNyd0DC9c", - Hash: "JDJhJDE0JEguY2Rva3VOTVRrNm1VeGdXWjAwamViMGNvV0xYZFdHcTZjenFCRWE3Ymt2N1JiSFJDZlYy", - Password: "test2", - }, - { - ManagerSalt: "ghKJaaSNTjuFmgLRqrgY4FGfx8wXEGOBE02PZvbv", - Salt: "NO/mtrMhGjX1FGDGdpGrDJIi4jxsb0aFa7ybId7r", - Hash: "JDJhJDE0JEI0M055Z2NDcjNUanB5ZEJ5MzUybi5FT3o4Y1MyNXp2c1NDVS9hS0hOcUxSRDZTWmUxTnN5", - Password: "supermono", - }, -} - -func TestSaltedPassword(t *testing.T) { - // check newly-generated password - managerSalt, err := NewSalt() - if err != nil { - t.Error("Could not generate manager salt") - } - - salt, err := NewSalt() - if err != nil { - t.Error("Could not generate salt") - } - - manager := NewSaltedManager(managerSalt) - - passHash, err := manager.GenerateFromPassword(salt, "this is a test password") - if err != nil { - t.Error("Could not generate from password") - } - - if manager.CompareHashAndPassword(passHash, salt, "this is a test password") != nil { - t.Error("Generated password does not match") - } - - // check our stored passwords - for i, info := range SaltedPasswords { - // decode strings to bytes - managerSalt, err = base64.StdEncoding.DecodeString(info.ManagerSalt) - if err != nil { - t.Errorf("Could not decode manager salt for test %d", i) - } - - salt, err := base64.StdEncoding.DecodeString(info.Salt) - if err != nil { - t.Errorf("Could not decode salt for test %d", i) - } - - hash, err := base64.StdEncoding.DecodeString(info.Hash) - if err != nil { - t.Errorf("Could not decode hash for test %d", i) - } - - // make sure our test values are still correct - manager := NewSaltedManager(managerSalt) - if manager.CompareHashAndPassword(hash, salt, info.Password) != nil { - t.Errorf("Password does not match for [%s]", info.Password) - } - } -} diff --git a/irc/passwd/unsalted.go b/irc/passwd/unsalted.go deleted file mode 100644 index 2c23fd26..00000000 --- a/irc/passwd/unsalted.go +++ /dev/null @@ -1,53 +0,0 @@ -// Copyright (c) 2012-2014 Jeremy Latt -// released under the MIT license - -package passwd - -import ( - "encoding/base64" - "errors" - - "golang.org/x/crypto/bcrypt" -) - -var ( - // ErrEmptyPassword means that an empty password was given. - ErrEmptyPassword = errors.New("empty password") -) - -// GenerateEncodedPasswordBytes returns an encrypted password, returning the bytes directly. -func GenerateEncodedPasswordBytes(passwd string) (encoded []byte, err error) { - if passwd == "" { - err = ErrEmptyPassword - return - } - encoded, err = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.MinCost) - return -} - -// GenerateEncodedPassword returns an encrypted password, encoded into a string with base64. -func GenerateEncodedPassword(passwd string) (encoded string, err error) { - bcrypted, err := GenerateEncodedPasswordBytes(passwd) - encoded = base64.StdEncoding.EncodeToString(bcrypted) - return -} - -// DecodePasswordHash takes a base64-encoded password hash and returns the appropriate bytes. -func DecodePasswordHash(encoded string) (decoded []byte, err error) { - if encoded == "" { - err = ErrEmptyPassword - return - } - decoded, err = base64.StdEncoding.DecodeString(encoded) - return -} - -// ComparePassword compares a given password with the given hash. -func ComparePassword(hash, password []byte) error { - return bcrypt.CompareHashAndPassword(hash, password) -} - -// ComparePasswordString compares a given password string with the given hash. -func ComparePasswordString(hash []byte, password string) error { - return ComparePassword(hash, []byte(password)) -} diff --git a/irc/passwd/unsalted_test.go b/irc/passwd/unsalted_test.go deleted file mode 100644 index 80d69c66..00000000 --- a/irc/passwd/unsalted_test.go +++ /dev/null @@ -1,54 +0,0 @@ -// Copyright (c) 2016 Daniel Oaks -// released under the MIT license - -package passwd - -import ( - "testing" -) - -var UnsaltedPasswords = map[string]string{ - "test1": "JDJhJDA0JFFwZ1V0RWZTMFVaMkFrdlRrTG9FZk9FNEZWbWkvVEhsdGFnSXlIUC5wVmpYTkNERFJPNlcu", - "test2": "JDJhJDA0JHpQTGNqczlIanc3V2NFQ3JEOVlTM09aNkRTbGRsQzRyNmt3Q01aSUs2Y2xyWURVODZ1V0px", - "supernomo": "JDJhJDA0JHdJekhnQmk1VXQ4WUphL0pIL0tXQWVKVXJ6dXcvRDJ3WFljWW9XOGhzNllIbW1DRlFkL1VL", -} - -func TestUnsaltedPassword(t *testing.T) { - for password, hash := range UnsaltedPasswords { - generatedHash, err := GenerateEncodedPassword(password) - if err != nil { - t.Errorf("Could not hash password for [%s]: %s", password, err.Error()) - } - - hashBytes, err := DecodePasswordHash(hash) - if err != nil { - t.Errorf("Could not decode hash for [%s]: %s", password, err.Error()) - } - - generatedHashBytes, err := DecodePasswordHash(generatedHash) - if err != nil { - t.Errorf("Could not decode generated hash for [%s]: %s", password, err.Error()) - } - - passwordBytes := []byte(password) - - if ComparePassword(hashBytes, passwordBytes) != nil { - t.Errorf("Stored hash for [%s] did not match", password) - } - if ComparePassword(generatedHashBytes, passwordBytes) != nil { - t.Errorf("Generated hash for [%s] did not match", password) - } - } -} - -func TestUnsaltedPasswordFailures(t *testing.T) { - _, err := GenerateEncodedPassword("") - if err != ErrEmptyPassword { - t.Error("Generating empty password did not fail as expected!") - } - - _, err = DecodePasswordHash("") - if err != ErrEmptyPassword { - t.Error("Decoding empty password hash did not fail as expected!") - } -} diff --git a/irc/server.go b/irc/server.go index c05767ab..fb7c3baa 100644 --- a/irc/server.go +++ b/irc/server.go @@ -8,7 +8,6 @@ package irc import ( "bufio" "crypto/tls" - "encoding/base64" "fmt" "log" "math/rand" @@ -31,7 +30,6 @@ import ( "github.com/oragono/oragono/irc/languages" "github.com/oragono/oragono/irc/logger" "github.com/oragono/oragono/irc/modes" - "github.com/oragono/oragono/irc/passwd" "github.com/oragono/oragono/irc/sno" "github.com/oragono/oragono/irc/utils" "github.com/tidwall/buntdb" @@ -90,7 +88,6 @@ type Server struct { motdLines []string name string nameCasefolded string - passwords *passwd.SaltedManager rehashMutex sync.Mutex // tier 4 rehashSignal chan os.Signal pprofServer *http.Server @@ -996,27 +993,6 @@ func (server *Server) loadDatastore(config *Config) error { server.loadDLines() server.loadKLines() - // load password manager - server.logger.Debug("startup", "Loading passwords") - err = server.store.View(func(tx *buntdb.Tx) error { - saltString, err := tx.Get(keySalt) - if err != nil { - return fmt.Errorf("Could not retrieve salt string: %s", err.Error()) - } - - salt, err := base64.StdEncoding.DecodeString(saltString) - if err != nil { - return err - } - - pwm := passwd.NewSaltedManager(salt) - server.passwords = &pwm - return nil - }) - if err != nil { - return fmt.Errorf("Could not load salt: %s", err.Error()) - } - server.channelRegistry = NewChannelRegistry(server) server.accounts = NewAccountManager(server) diff --git a/oragono.go b/oragono.go index 61547131..4952cb28 100644 --- a/oragono.go +++ b/oragono.go @@ -17,8 +17,8 @@ import ( "github.com/oragono/oragono/irc" "github.com/oragono/oragono/irc/logger" "github.com/oragono/oragono/irc/mkcerts" - "github.com/oragono/oragono/irc/passwd" stackimpact "github.com/stackimpact/stackimpact-go" + "golang.org/x/crypto/bcrypt" "golang.org/x/crypto/ssh/terminal" ) @@ -73,11 +73,11 @@ Options: if confirm != password { log.Fatal("passwords do not match") } - encoded, err := passwd.GenerateEncodedPassword(password) + hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.MinCost) if err != nil { log.Fatal("encoding error:", err.Error()) } - fmt.Println(encoded) + fmt.Println(string(hash)) } else if arguments["initdb"].(bool) { irc.InitDB(config.Datastore.Path) if !arguments["--quiet"].(bool) { diff --git a/oragono.yaml b/oragono.yaml index 5ea4ffdd..7011de19 100644 --- a/oragono.yaml +++ b/oragono.yaml @@ -76,7 +76,7 @@ server: fingerprint: 938dd33f4b76dcaf7ce5eb25c852369cb4b8fb47ba22fc235aa29c6623a5f182 # password the gateway uses to connect, made with oragono genpasswd - password: JDJhJDA0JG9rTTVERlNRa0hpOEZpNkhjZE95SU9Da1BseFdlcWtOTEQxNEFERVlqbEZNTkdhOVlYUkMu + password: "$2a$04$sLEFDpIOyUp55e6gTMKbOeroT6tMXTjPFvA0eGvwvImVR9pkwv7ee" # hosts that can use this webirc command # you should also add these addresses to the connection limits and throttling exemption lists @@ -145,6 +145,9 @@ accounts: # can users register new accounts? enabled: true + # this is the bcrypt cost we'll use for account passwords + bcrypt-cost: 12 + # length of time a user has to verify their account before it can be re-registered verify-timeout: "32h" @@ -304,7 +307,7 @@ opers: # password to login with /OPER command # generated using "oragono genpasswd" - password: JDJhJDA0JE1vZmwxZC9YTXBhZ3RWT2xBbkNwZnV3R2N6VFUwQUI0RUJRVXRBRHliZVVoa0VYMnlIaGsu + password: "$2a$04$LiytCxaY0lI.guDj2pBN4eLRD5cdM2OLDwqmGAgB6M2OPirbF5Jcu" # logging, takes inspiration from Insp logging: From eb5f2c1db95f549f219cf7c1ab7c211a824684dd Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Mon, 6 Aug 2018 04:55:39 -0400 Subject: [PATCH 3/4] fix webirc password handling --- irc/gateways.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/irc/gateways.go b/irc/gateways.go index 9d5d584a..3870ef53 100644 --- a/irc/gateways.go +++ b/irc/gateways.go @@ -27,9 +27,7 @@ func (wc *webircConfig) Populate() (err error) { } if wc.PasswordString != "" { - var password []byte wc.Password, err = decodeLegacyPasswordHash(wc.PasswordString) - wc.Password = password } return err } From 7ebd35f5a0df7f66babcb45e74dc711e267f72a9 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 15 Aug 2018 13:07:28 -0400 Subject: [PATCH 4/4] update Gopkg.lock to include sha3 --- Gopkg.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gopkg.lock b/Gopkg.lock index 45ef4275..bc974650 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -134,6 +134,7 @@ packages = [ "bcrypt", "blowfish", + "sha3", "ssh/terminal", ] pruneopts = "" @@ -200,6 +201,7 @@ "github.com/oragono/go-ident", "github.com/tidwall/buntdb", "golang.org/x/crypto/bcrypt", + "golang.org/x/crypto/sha3", "golang.org/x/crypto/ssh/terminal", "golang.org/x/text/secure/precis", "gopkg.in/yaml.v2",