diff --git a/CHANGELOG.md b/CHANGELOG.md index 38d9eb7..3aa1d6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Changed + +- Recover and Confirm now use split tokens + + The reason for this change is that there's a timing attack possible + because of the use of memcmp() by databases to check if the token exists. + By using a separate piece of the token as a selector, we use memcmp() in + one place, but a crypto constant time compare in the other to check the + other value, and this value cannot be leaked by timing, and since you need + both to recover/confirm as the user, this attack should now be mitigated. + ## [2.0.0-rc2] - 2018-05-14 Mostly rewrote Authboss by changing many of the core interfaces. This release diff --git a/README.md b/README.md index 747fe19..aa529fa 100644 --- a/README.md +++ b/README.md @@ -441,6 +441,10 @@ A hook on register kicks off the start of a confirmation which sends an e-mail w When the user re-visits the page, the `BodyReader` must read the token and return a type that can return the token. +Confirmations carry two values in the database to prevent a timing attack. The selector and the +verifier, always make sure in the ConfirmingServerStorer you're searching by the selector and +not the verifier. + ## Password Recovery | Info and Requirements | | @@ -466,6 +470,10 @@ to be rendered. They enter their password into the form, and `POST` to `/recover/end` which sends the token and the new password which is retrieved by `RecoverEndValuer` which sets their password and saves them. +Password recovery has two values in the database to prevent a timing attack. The selector and the +verifier, always make sure in the RecoveringServerStorer you're searching by the selector and +not the verifier. + ## Remember Me | Info and Requirements | | diff --git a/confirm/confirm.go b/confirm/confirm.go index 8bba155..9852236 100644 --- a/confirm/confirm.go +++ b/confirm/confirm.go @@ -5,6 +5,7 @@ import ( "context" "crypto/rand" "crypto/sha512" + "crypto/subtle" "encoding/base64" "fmt" "net/http" @@ -31,6 +32,8 @@ const ( // DataConfirmURL is the name of the e-mail template variable // that gives the url to send to the user for confirmation. DataConfirmURL = "url" + + confirmTokenSize = 64 ) func init() { @@ -110,13 +113,14 @@ func (c *Confirm) StartConfirmationWeb(w http.ResponseWriter, r *http.Request, h func (c *Confirm) StartConfirmation(ctx context.Context, user authboss.ConfirmableUser, sendEmail bool) error { logger := c.Authboss.Logger(ctx) - hash, token, err := GenerateToken() + selector, verifier, token, err := GenerateConfirmCreds() if err != nil { return err } user.PutConfirmed(false) - user.PutConfirmToken(hash) + user.PutConfirmSelector(selector) + user.PutConfirmVerifier(verifier) logger.Infof("generated new confirm token for user: %s", user.GetPID()) if err := c.Authboss.Config.Storage.Server.Save(ctx, user); err != nil { @@ -170,45 +174,49 @@ func (c *Confirm) Get(w http.ResponseWriter, r *http.Request) error { if errs := validator.Validate(); errs != nil { logger.Infof("validation failed in Confirm.Get, this typically means a bad token: %+v", errs) - ro := authboss.RedirectOptions{ - Code: http.StatusTemporaryRedirect, - Failure: "Invalid confirm token.", - RedirectPath: c.Authboss.Config.Paths.ConfirmNotOK, - } - return c.Authboss.Config.Core.Redirector.Redirect(w, r, ro) + return c.invalidToken(w, r) } values := authboss.MustHaveConfirmValues(validator) - toHash, err := base64.URLEncoding.DecodeString(values.GetToken()) + rawToken, err := base64.URLEncoding.DecodeString(values.GetToken()) if err != nil { logger.Infof("error decoding token in Confirm.Get, this typically means a bad token: %s %+v", values.GetToken(), err) - ro := authboss.RedirectOptions{ - Code: http.StatusTemporaryRedirect, - Failure: "Invalid confirm token.", - RedirectPath: c.Authboss.Config.Paths.ConfirmNotOK, - } - return c.Authboss.Config.Core.Redirector.Redirect(w, r, ro) + return c.invalidToken(w, r) } - sum := sha512.Sum512(toHash) - token := base64.StdEncoding.EncodeToString(sum[:]) + if len(rawToken) != confirmTokenSize { + logger.Infof("invalid confirm token submitted, size was wrong: %d", len(rawToken)) + return c.invalidToken(w, r) + } + + selectorBytes := sha512.Sum512(rawToken[:32]) + verifierBytes := sha512.Sum512(rawToken[32:]) + selector := base64.StdEncoding.EncodeToString(selectorBytes[:]) storer := authboss.EnsureCanConfirm(c.Authboss.Config.Storage.Server) - user, err := storer.LoadByConfirmToken(r.Context(), token) + user, err := storer.LoadByConfirmSelector(r.Context(), selector) if err == authboss.ErrUserNotFound { - logger.Infof("confirm token was not found in database: %s", token) - ro := authboss.RedirectOptions{ - Code: http.StatusTemporaryRedirect, - Failure: "Invalid confirm token.", - RedirectPath: c.Authboss.Config.Paths.ConfirmNotOK, - } - return c.Authboss.Config.Core.Redirector.Redirect(w, r, ro) + logger.Infof("confirm selector was not found in database: %s", selector) + return c.invalidToken(w, r) } else if err != nil { return err } - user.PutConfirmToken("") + dbVerifierBytes, err := base64.StdEncoding.DecodeString(user.GetConfirmVerifier()) + if err != nil { + logger.Infof("invalid confirm verifier stored in database: %s", user.GetConfirmVerifier()) + return c.invalidToken(w, r) + } + + if subtle.ConstantTimeEq(int32(len(verifierBytes)), int32(len(dbVerifierBytes))) != 1 || + subtle.ConstantTimeCompare(verifierBytes[:], dbVerifierBytes) != 1 { + logger.Info("stored confirm verifier does not match provided one") + return c.invalidToken(w, r) + } + + user.PutConfirmSelector("") + user.PutConfirmVerifier("") user.PutConfirmed(true) logger.Infof("user %s confirmed their account", user.GetPID()) @@ -224,6 +232,15 @@ func (c *Confirm) Get(w http.ResponseWriter, r *http.Request) error { return c.Authboss.Config.Core.Redirector.Redirect(w, r, ro) } +func (c *Confirm) invalidToken(w http.ResponseWriter, r *http.Request) error { + ro := authboss.RedirectOptions{ + Code: http.StatusTemporaryRedirect, + Failure: "confirm token is invalid", + RedirectPath: c.Authboss.Config.Paths.ConfirmNotOK, + } + return c.Authboss.Config.Core.Redirector.Redirect(w, r, ro) +} + // Middleware ensures that a user is confirmed, or else it will intercept the request // and send them to the confirm page, this will load the user if he's not been loaded // yet from the session. @@ -253,12 +270,20 @@ func Middleware(ab *authboss.Authboss) func(http.Handler) http.Handler { } } -// GenerateToken creates a random token that will be used to confirm the user. -func GenerateToken() (hash string, token string, err error) { - tok := make([]byte, 32) - if _, err := rand.Read(tok); err != nil { - return "", "", err +// GenerateConfirmCreds generates pieces needed for user confirmy +// selector: hash of the first half of a 64 byte value (to be stored in the database and used in SELECT query) +// verifier: hash of the second half of a 64 byte value (to be stored in database but never used in SELECT query) +// token: the user-facing base64 encoded selector+verifier +func GenerateConfirmCreds() (selector, verifier, token string, err error) { + rawToken := make([]byte, confirmTokenSize) + if _, err = rand.Read(rawToken); err != nil { + return "", "", "", err } - sum := sha512.Sum512(tok) - return base64.StdEncoding.EncodeToString(sum[:]), base64.URLEncoding.EncodeToString(tok[:]), nil + selectorBytes := sha512.Sum512(rawToken[:32]) + verifierBytes := sha512.Sum512(rawToken[32:]) + + return base64.StdEncoding.EncodeToString(selectorBytes[:]), + base64.StdEncoding.EncodeToString(verifierBytes[:]), + base64.URLEncoding.EncodeToString(rawToken), + nil } diff --git a/confirm/confirm_test.go b/confirm/confirm_test.go index 28f3227..4ee8151 100644 --- a/confirm/confirm_test.go +++ b/confirm/confirm_test.go @@ -184,12 +184,12 @@ func TestGetSuccess(t *testing.T) { harness := testSetup() - hash, token, err := GenerateToken() + selector, verifier, token, err := GenerateConfirmCreds() if err != nil { t.Fatal(err) } - user := &mocks.User{Email: "test@test.com", Confirmed: false, ConfirmToken: hash} + user := &mocks.User{Email: "test@test.com", Confirmed: false, ConfirmSelector: selector, ConfirmVerifier: verifier} harness.storer.Users["test@test.com"] = user harness.bodyReader.Return = mocks.Values{ Token: token, @@ -209,8 +209,11 @@ func TestGetSuccess(t *testing.T) { t.Error("redir path was wrong:", p) } - if len(user.ConfirmToken) != 0 { - t.Error("the confirm token should have been erased") + if len(user.ConfirmSelector) != 0 { + t.Error("the confirm selector should have been erased") + } + if len(user.ConfirmVerifier) != 0 { + t.Error("the confirm verifier should have been erased") } if !user.Confirmed { t.Error("the user should have been confirmed") @@ -239,7 +242,7 @@ func TestGetValidationFailure(t *testing.T) { if p := harness.redirector.Options.RedirectPath; p != harness.ab.Paths.ConfirmNotOK { t.Error("redir path was wrong:", p) } - if reason := harness.redirector.Options.Failure; reason != "Invalid confirm token." { + if reason := harness.redirector.Options.Failure; reason != "confirm token is invalid" { t.Error("reason for failure was wrong:", reason) } } @@ -266,7 +269,7 @@ func TestGetBase64DecodeFailure(t *testing.T) { if p := harness.redirector.Options.RedirectPath; p != harness.ab.Paths.ConfirmNotOK { t.Error("redir path was wrong:", p) } - if reason := harness.redirector.Options.Failure; reason != "Invalid confirm token." { + if reason := harness.redirector.Options.Failure; reason != "confirm token is invalid" { t.Error("reason for failure was wrong:", reason) } } @@ -276,7 +279,7 @@ func TestGetUserNotFoundFailure(t *testing.T) { harness := testSetup() - _, token, err := GenerateToken() + _, _, token, err := GenerateConfirmCreds() if err != nil { t.Fatal(err) } @@ -298,7 +301,7 @@ func TestGetUserNotFoundFailure(t *testing.T) { if p := harness.redirector.Options.RedirectPath; p != harness.ab.Paths.ConfirmNotOK { t.Error("redir path was wrong:", p) } - if reason := harness.redirector.Options.Failure; reason != "Invalid confirm token." { + if reason := harness.redirector.Options.Failure; reason != "confirm token is invalid" { t.Error("reason for failure was wrong:", reason) } } @@ -362,22 +365,31 @@ func TestMiddlewareDisallow(t *testing.T) { } } -func TestGenerateToken(t *testing.T) { +func TestGenerateRecoverCreds(t *testing.T) { t.Parallel() - hash, token, err := GenerateToken() + selector, verifier, token, err := GenerateConfirmCreds() if err != nil { t.Error(err) } - // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 - if len(hash) != 88 { - t.Errorf("string length was wrong (%d): %s", len(hash), hash) + if verifier == selector { + t.Error("the verifier and selector should be different") } - // base64 length: n = 32; 4*(32/3) = 42.6; round to nearest 4: 44 - if len(token) != 44 { - t.Errorf("string length was wrong (%d): %s", len(token), token) + // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 + if len(verifier) != 88 { + t.Errorf("verifier length was wrong (%d): %s", len(verifier), verifier) + } + + // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 + if len(selector) != 88 { + t.Errorf("selector length was wrong (%d): %s", len(selector), selector) + } + + // base64 length: n = 64; 4*(64/3) = 85.33; round to nearest 4: 88 + if len(token) != 88 { + t.Errorf("token length was wrong (%d): %s", len(token), token) } rawToken, err := base64.URLEncoding.DecodeString(token) @@ -385,13 +397,21 @@ func TestGenerateToken(t *testing.T) { t.Error(err) } - rawHash, err := base64.StdEncoding.DecodeString(hash) + rawSelector, err := base64.StdEncoding.DecodeString(selector) + if err != nil { + t.Error(err) + } + rawVerifier, err := base64.StdEncoding.DecodeString(verifier) if err != nil { t.Error(err) } - checkHash := sha512.Sum512(rawToken) - if 0 != bytes.Compare(checkHash[:], rawHash) { - t.Error("expected hashes to match") + checkSelector := sha512.Sum512(rawToken[:32]) + if 0 != bytes.Compare(checkSelector[:], rawSelector) { + t.Error("expected selector to match") + } + checkVerifier := sha512.Sum512(rawToken[32:]) + if 0 != bytes.Compare(checkVerifier[:], rawVerifier) { + t.Error("expected verifier to match") } } diff --git a/internal/mocks/mocks.go b/internal/mocks/mocks.go index 3e756d5..2312fe6 100644 --- a/internal/mocks/mocks.go +++ b/internal/mocks/mocks.go @@ -18,9 +18,11 @@ type User struct { Username string Email string Password string - RecoverToken string + RecoverSelector string + RecoverVerifier string RecoverTokenExpiry time.Time - ConfirmToken string + ConfirmSelector string + ConfirmVerifier string Confirmed bool AttemptCount int LastAttempt time.Time @@ -47,14 +49,20 @@ func (u User) GetUsername() string { return u.Username } // GetPassword from user func (u User) GetPassword() string { return u.Password } -// GetRecoverToken from user -func (u User) GetRecoverToken() string { return u.RecoverToken } +// GetRecoverSelector from user +func (u User) GetRecoverSelector() string { return u.RecoverSelector } + +// GetRecoverVerifier from user +func (u User) GetRecoverVerifier() string { return u.RecoverVerifier } // GetRecoverExpiry from user func (u User) GetRecoverExpiry() time.Time { return u.RecoverTokenExpiry } -// GetConfirmToken from user -func (u User) GetConfirmToken() string { return u.ConfirmToken } +// GetConfirmSelector from user +func (u User) GetConfirmSelector() string { return u.ConfirmSelector } + +// GetConfirmVerifier from user +func (u User) GetConfirmVerifier() string { return u.ConfirmVerifier } // GetConfirmed from user func (u User) GetConfirmed() bool { return u.Confirmed } @@ -101,16 +109,22 @@ func (u *User) PutEmail(email string) { u.Email = email } // PutPassword into user func (u *User) PutPassword(password string) { u.Password = password } -// PutRecoverToken into user -func (u *User) PutRecoverToken(recoverToken string) { u.RecoverToken = recoverToken } +// PutRecoverSelector into user +func (u *User) PutRecoverSelector(recoverSelector string) { u.RecoverSelector = recoverSelector } + +// PutRecoverVerifier into user +func (u *User) PutRecoverVerifier(recoverVerifier string) { u.RecoverVerifier = recoverVerifier } // PutRecoverExpiry into user func (u *User) PutRecoverExpiry(recoverTokenExpiry time.Time) { u.RecoverTokenExpiry = recoverTokenExpiry } -// PutConfirmToken into user -func (u *User) PutConfirmToken(confirmToken string) { u.ConfirmToken = confirmToken } +// PutConfirmSelector into user +func (u *User) PutConfirmSelector(confirmSelector string) { u.ConfirmSelector = confirmSelector } + +// PutConfirmVerifier into user +func (u *User) PutConfirmVerifier(confirmVerifier string) { u.ConfirmVerifier = confirmVerifier } // PutConfirmed into user func (u *User) PutConfirmed(confirmed bool) { u.Confirmed = confirmed } @@ -223,10 +237,10 @@ func (s *ServerStorer) SaveOAuth2(ctx context.Context, user authboss.OAuth2User) return nil } -// LoadByConfirmToken finds a user by his confirm token -func (s *ServerStorer) LoadByConfirmToken(ctx context.Context, token string) (authboss.ConfirmableUser, error) { +// LoadByConfirmSelector finds a user by his confirm selector +func (s *ServerStorer) LoadByConfirmSelector(ctx context.Context, selector string) (authboss.ConfirmableUser, error) { for _, v := range s.Users { - if v.ConfirmToken == token { + if v.ConfirmSelector == selector { return v, nil } } @@ -234,10 +248,10 @@ func (s *ServerStorer) LoadByConfirmToken(ctx context.Context, token string) (au return nil, authboss.ErrUserNotFound } -// LoadByRecoverToken finds a user by his recover token -func (s *ServerStorer) LoadByRecoverToken(ctx context.Context, token string) (authboss.RecoverableUser, error) { +// LoadByRecoverSelector finds a user by his recover token +func (s *ServerStorer) LoadByRecoverSelector(ctx context.Context, selector string) (authboss.RecoverableUser, error) { for _, v := range s.Users { - if v.RecoverToken == token { + if v.RecoverSelector == selector { return v, nil } } diff --git a/mocks_test.go b/mocks_test.go index bd051d4..7054bbd 100644 --- a/mocks_test.go +++ b/mocks_test.go @@ -16,11 +16,13 @@ type mockUser struct { Password string Username string - RecoverToken string - RecoverTokenExpiry time.Time + RecoverSelector string + RecoverVerifier string + RecoverExpiry time.Time - ConfirmToken string - Confirmed bool + ConfirmSelector string + ConfirmVerifier string + Confirmed bool AttemptCount int LastAttempt time.Time @@ -96,51 +98,53 @@ func (m *mockServerStorer) Create(ctx context.Context, user User) error { panic( func (m *mockServerStorer) NewFromOAuth2(ctx context.Context, provider string, details map[string]string) (OAuth2User, error) { panic("not impl") } -func (m *mockServerStorer) LoadByConfirmToken(ctx context.Context, token string) (ConfirmableUser, error) { +func (m *mockServerStorer) LoadByConfirmSelector(ctx context.Context, selector string) (ConfirmableUser, error) { panic("not impl") } -func (m *mockServerStorer) LoadByRecoverToken(ctx context.Context, token string) (RecoverableUser, error) { +func (m *mockServerStorer) LoadByRecoverSelector(ctx context.Context, selector string) (RecoverableUser, error) { panic("not impl") } func (m *mockServerStorer) SaveOAuth2(ctx context.Context, user OAuth2User) error { panic("not impl") } -func (m mockUser) GetPID() string { return m.Email } -func (m mockUser) GetEmail() string { return m.Email } -func (m mockUser) GetUsername() string { return m.Username } -func (m mockUser) GetPassword() string { return m.Password } -func (m mockUser) GetRecoverToken() string { return m.RecoverToken } -func (m mockUser) GetRecoverExpiry() time.Time { return m.RecoverTokenExpiry } -func (m mockUser) GetConfirmToken() string { return m.ConfirmToken } -func (m mockUser) GetConfirmed() bool { return m.Confirmed } -func (m mockUser) GetAttemptCount() int { return m.AttemptCount } -func (m mockUser) GetLastAttempt() time.Time { return m.LastAttempt } -func (m mockUser) GetLocked() time.Time { return m.Locked } -func (m mockUser) IsOAuth2User() bool { return len(m.OAuth2Provider) != 0 } -func (m mockUser) GetOAuth2UID() string { return m.OAuth2UID } -func (m mockUser) GetOAuth2Provider() string { return m.OAuth2Provider } -func (m mockUser) GetOAuth2AccessToken() string { return m.OAuth2Token } -func (m mockUser) GetOAuth2RefreshToken() string { return m.OAuth2Refresh } -func (m mockUser) GetOAuth2Expiry() time.Time { return m.OAuth2Expiry } -func (m mockUser) GetArbitrary() map[string]string { return m.Arbitrary } -func (m *mockUser) PutPID(email string) { m.Email = email } -func (m *mockUser) PutUsername(username string) { m.Username = username } -func (m *mockUser) PutEmail(email string) { m.Email = email } -func (m *mockUser) PutPassword(password string) { m.Password = password } -func (m *mockUser) PutRecoverToken(recoverToken string) { m.RecoverToken = recoverToken } -func (m *mockUser) PutRecoverExpiry(recoverTokenExpiry time.Time) { - m.RecoverTokenExpiry = recoverTokenExpiry -} -func (m *mockUser) PutConfirmToken(confirmToken string) { m.ConfirmToken = confirmToken } -func (m *mockUser) PutConfirmed(confirmed bool) { m.Confirmed = confirmed } -func (m *mockUser) PutAttemptCount(attemptCount int) { m.AttemptCount = attemptCount } -func (m *mockUser) PutLastAttempt(attemptTime time.Time) { m.LastAttempt = attemptTime } -func (m *mockUser) PutLocked(locked time.Time) { m.Locked = locked } -func (m *mockUser) PutOAuth2UID(uid string) { m.OAuth2UID = uid } -func (m *mockUser) PutOAuth2Provider(provider string) { m.OAuth2Provider = provider } -func (m *mockUser) PutOAuth2AccessToken(token string) { m.OAuth2Token = token } -func (m *mockUser) PutOAuth2RefreshToken(refresh string) { m.OAuth2Refresh = refresh } -func (m *mockUser) PutOAuth2Expiry(expiry time.Time) { m.OAuth2Expiry = expiry } -func (m *mockUser) PutArbitrary(arb map[string]string) { m.Arbitrary = arb } +func (m mockUser) GetPID() string { return m.Email } +func (m mockUser) GetEmail() string { return m.Email } +func (m mockUser) GetUsername() string { return m.Username } +func (m mockUser) GetPassword() string { return m.Password } +func (m mockUser) GetRecoverSelector() string { return m.RecoverSelector } +func (m mockUser) GetRecoverVerifier() string { return m.RecoverVerifier } +func (m mockUser) GetRecoverExpiry() time.Time { return m.RecoverExpiry } +func (m mockUser) GetConfirmSelector() string { return m.ConfirmSelector } +func (m mockUser) GetConfirmVerifier() string { return m.ConfirmVerifier } +func (m mockUser) GetConfirmed() bool { return m.Confirmed } +func (m mockUser) GetAttemptCount() int { return m.AttemptCount } +func (m mockUser) GetLastAttempt() time.Time { return m.LastAttempt } +func (m mockUser) GetLocked() time.Time { return m.Locked } +func (m mockUser) IsOAuth2User() bool { return len(m.OAuth2Provider) != 0 } +func (m mockUser) GetOAuth2UID() string { return m.OAuth2UID } +func (m mockUser) GetOAuth2Provider() string { return m.OAuth2Provider } +func (m mockUser) GetOAuth2AccessToken() string { return m.OAuth2Token } +func (m mockUser) GetOAuth2RefreshToken() string { return m.OAuth2Refresh } +func (m mockUser) GetOAuth2Expiry() time.Time { return m.OAuth2Expiry } +func (m mockUser) GetArbitrary() map[string]string { return m.Arbitrary } +func (m *mockUser) PutPID(email string) { m.Email = email } +func (m *mockUser) PutUsername(username string) { m.Username = username } +func (m *mockUser) PutEmail(email string) { m.Email = email } +func (m *mockUser) PutPassword(password string) { m.Password = password } +func (m *mockUser) PutRecoverSelector(recoverSelector string) { m.RecoverSelector = recoverSelector } +func (m *mockUser) PutRecoverVerifier(recoverVerifier string) { m.RecoverVerifier = recoverVerifier } +func (m *mockUser) PutRecoverExpiry(recoverExpiry time.Time) { m.RecoverExpiry = recoverExpiry } +func (m *mockUser) PutConfirmSelector(confirmSelector string) { m.ConfirmSelector = confirmSelector } +func (m *mockUser) PutConfirmVerifier(confirmVerifier string) { m.ConfirmVerifier = confirmVerifier } +func (m *mockUser) PutConfirmed(confirmed bool) { m.Confirmed = confirmed } +func (m *mockUser) PutAttemptCount(attemptCount int) { m.AttemptCount = attemptCount } +func (m *mockUser) PutLastAttempt(attemptTime time.Time) { m.LastAttempt = attemptTime } +func (m *mockUser) PutLocked(locked time.Time) { m.Locked = locked } +func (m *mockUser) PutOAuth2UID(uid string) { m.OAuth2UID = uid } +func (m *mockUser) PutOAuth2Provider(provider string) { m.OAuth2Provider = provider } +func (m *mockUser) PutOAuth2AccessToken(token string) { m.OAuth2Token = token } +func (m *mockUser) PutOAuth2RefreshToken(refresh string) { m.OAuth2Refresh = refresh } +func (m *mockUser) PutOAuth2Expiry(expiry time.Time) { m.OAuth2Expiry = expiry } +func (m *mockUser) PutArbitrary(arb map[string]string) { m.Arbitrary = arb } type mockClientStateReadWriter struct { state mockClientState diff --git a/recover/recover.go b/recover/recover.go index 82c9ab6..f75d056 100644 --- a/recover/recover.go +++ b/recover/recover.go @@ -5,6 +5,7 @@ import ( "context" "crypto/rand" "crypto/sha512" + "crypto/subtle" "encoding/base64" "errors" "fmt" @@ -35,6 +36,8 @@ const ( recoverInitiateSuccessFlash = "An email has been sent to you with further instructions on how to reset your password." recoverTokenExpiredFlash = "Account recovery request has expired. Please try again." recoverFailedErrorFlash = "Account recovery has failed. Please contact tech support." + + recoverTokenSize = 64 ) func init() { @@ -97,12 +100,13 @@ func (r *Recover) StartPost(w http.ResponseWriter, req *http.Request) error { ru := authboss.MustBeRecoverable(user) - hash, token, err := GenerateToken() + selector, verifier, token, err := GenerateRecoverCreds() if err != nil { return err } - ru.PutRecoverToken(hash) + ru.PutRecoverSelector(selector) + ru.PutRecoverVerifier(verifier) ru.PutRecoverExpiry(time.Now().UTC().Add(r.Config.Modules.RecoverTokenDuration)) if err := r.Authboss.Storage.Server.Save(req.Context(), ru); err != nil { @@ -199,11 +203,17 @@ func (r *Recover) EndPost(w http.ResponseWriter, req *http.Request) error { return r.invalidToken(PageRecoverEnd, w, req) } - hash := sha512.Sum512(rawToken) - dbToken := base64.StdEncoding.EncodeToString(hash[:]) + if len(rawToken) != recoverTokenSize { + logger.Infof("invalid recover token submitted, size was wrong: %d", len(rawToken)) + return r.invalidToken(PageRecoverEnd, w, req) + } + + selectorBytes := sha512.Sum512(rawToken[:32]) + verifierBytes := sha512.Sum512(rawToken[32:]) + selector := base64.StdEncoding.EncodeToString(selectorBytes[:]) storer := authboss.EnsureCanRecover(r.Authboss.Config.Storage.Server) - user, err := storer.LoadByRecoverToken(req.Context(), dbToken) + user, err := storer.LoadByRecoverSelector(req.Context(), selector) if err == authboss.ErrUserNotFound { logger.Info("invalid recover token submitted, user not found") return r.invalidToken(PageRecoverEnd, w, req) @@ -216,13 +226,26 @@ func (r *Recover) EndPost(w http.ResponseWriter, req *http.Request) error { return r.invalidToken(PageRecoverEnd, w, req) } + dbVerifierBytes, err := base64.StdEncoding.DecodeString(user.GetRecoverVerifier()) + if err != nil { + logger.Infof("invalid recover verifier stored in database: %s", user.GetRecoverVerifier()) + return r.invalidToken(PageRecoverEnd, w, req) + } + + if subtle.ConstantTimeEq(int32(len(verifierBytes)), int32(len(dbVerifierBytes))) != 1 || + subtle.ConstantTimeCompare(verifierBytes[:], dbVerifierBytes) != 1 { + logger.Info("stored recover verifier does not match provided one") + return r.invalidToken(PageRecoverEnd, w, req) + } + pass, err := bcrypt.GenerateFromPassword([]byte(password), r.Authboss.Config.Modules.BCryptCost) if err != nil { return err } user.PutPassword(string(pass)) - user.PutRecoverToken("") // Don't allow another recovery + user.PutRecoverSelector("") // Don't allow another recovery + user.PutRecoverVerifier("") // Don't allow another recovery user.PutRecoverExpiry(time.Now().UTC()) // Put current time for those DBs that can't handle 0 time if err := storer.Save(req.Context(), user); err != nil { @@ -249,13 +272,20 @@ func (r *Recover) invalidToken(page string, w http.ResponseWriter, req *http.Req return r.Authboss.Core.Responder.Respond(w, req, http.StatusOK, PageRecoverEnd, data) } -// GenerateToken appropriate for user recovery -func GenerateToken() (hash, token string, err error) { - rawToken := make([]byte, 32) +// GenerateRecoverCreds generates pieces needed for user recovery +// selector: hash of the first half of a 64 byte value (to be stored in the database and used in SELECT query) +// verifier: hash of the second half of a 64 byte value (to be stored in database but never used in SELECT query) +// token: the user-facing base64 encoded selector+verifier +func GenerateRecoverCreds() (selector, verifier, token string, err error) { + rawToken := make([]byte, recoverTokenSize) if _, err = rand.Read(rawToken); err != nil { - return "", "", err + return "", "", "", err } - sum := sha512.Sum512(rawToken) + selectorBytes := sha512.Sum512(rawToken[:32]) + verifierBytes := sha512.Sum512(rawToken[32:]) - return base64.StdEncoding.EncodeToString(sum[:]), base64.URLEncoding.EncodeToString(rawToken), nil + return base64.StdEncoding.EncodeToString(selectorBytes[:]), + base64.StdEncoding.EncodeToString(verifierBytes[:]), + base64.URLEncoding.EncodeToString(rawToken), + nil } diff --git a/recover/recover_test.go b/recover/recover_test.go index 1cf49e1..9ffd5f9 100644 --- a/recover/recover_test.go +++ b/recover/recover_test.go @@ -17,8 +17,9 @@ import ( ) const ( - testURLBase64Token = "glL8qvO1YKmLxoyEQwVQPpUMM13f6_e4R-2hUQDzP2g=" - testStdBase64Token = "cn0uhfu5Ar2A2JsSs/zdj93zhC1lHJDyIhUYdSgyp71XL/nRb3be/I6AeMz4DACwTRqRAJ6loJedJyOcOtU1Jg==" + testSelector = `rnaGE8TDilrINHPxq/2xNU1FUTzsUSX8FvN5YzooyyWKk88fw1DjjbKBRGFtGew9OeZ+xeCC4mslfvQQMYspIg==` + testVerifier = `W1Mz30QhavVM4d8jKaFtxGBfb4GX+fOn7V0Pc1WeftgtyOtY5OX7sY9gIeY5CIY4n8LvfWy14W7/6rs2KO9pgA==` + testToken = `w5OZ51E61Q6wsJOVr9o7KmyepP7Od5VBHQ1ADDUBkiGGMjKfnMFPjtvNpLjLKJqffw72KWZzNLj0Cs8wqywdEQ==` ) func TestInit(t *testing.T) { @@ -233,12 +234,13 @@ func TestEndPostSuccess(t *testing.T) { h := testSetup() h.bodyReader.Return = &mocks.Values{ - Token: testURLBase64Token, + Token: testToken, } h.storer.Users["test@test.com"] = &mocks.User{ Email: "test@test.com", Password: "to-overwrite", - RecoverToken: testStdBase64Token, + RecoverSelector: testSelector, + RecoverVerifier: testVerifier, RecoverTokenExpiry: time.Now().UTC().AddDate(0, 0, 1), } @@ -273,12 +275,13 @@ func TestEndPostSuccessLogin(t *testing.T) { h.ab.Config.Modules.RecoverLoginAfterRecovery = true h.bodyReader.Return = &mocks.Values{ - Token: testURLBase64Token, + Token: testToken, } h.storer.Users["test@test.com"] = &mocks.User{ Email: "test@test.com", Password: "to-overwrite", - RecoverToken: testStdBase64Token, + RecoverSelector: testSelector, + RecoverVerifier: testVerifier, RecoverTokenExpiry: time.Now().UTC().AddDate(0, 0, 1), } @@ -314,7 +317,8 @@ func TestEndPostValidationFailure(t *testing.T) { h.storer.Users["test@test.com"] = &mocks.User{ Email: "test@test.com", Password: "to-overwrite", - RecoverToken: testStdBase64Token, + RecoverSelector: testSelector, + RecoverVerifier: testVerifier, RecoverTokenExpiry: time.Now().UTC().AddDate(0, 0, 1), } @@ -366,12 +370,13 @@ func TestEndPostExpiredToken(t *testing.T) { h := testSetup() h.bodyReader.Return = &mocks.Values{ - Token: testURLBase64Token, + Token: testToken, } h.storer.Users["test@test.com"] = &mocks.User{ Email: "test@test.com", Password: "to-overwrite", - RecoverToken: testStdBase64Token, + RecoverSelector: testSelector, + RecoverVerifier: testVerifier, RecoverTokenExpiry: time.Now().UTC().AddDate(0, 0, -1), } @@ -391,7 +396,7 @@ func TestEndPostUserNotExist(t *testing.T) { h := testSetup() h.bodyReader.Return = &mocks.Values{ - Token: testURLBase64Token, + Token: testToken, } r := mocks.Request("GET") @@ -418,22 +423,31 @@ func invalidCheck(t *testing.T, h *testHarness, w *httptest.ResponseRecorder) { } } -func TestGenerateToken(t *testing.T) { +func TestGenerateRecoverCreds(t *testing.T) { t.Parallel() - hash, token, err := GenerateToken() + selector, verifier, token, err := GenerateRecoverCreds() if err != nil { t.Error(err) } - // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 - if len(hash) != 88 { - t.Errorf("string length was wrong (%d): %s", len(hash), hash) + if verifier == selector { + t.Error("the verifier and selector should be different") } - // base64 length: n = 32; 4*(32/3) = 42.6; round to nearest 4: 44 - if len(token) != 44 { - t.Errorf("string length was wrong (%d): %s", len(token), token) + // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 + if len(verifier) != 88 { + t.Errorf("verifier length was wrong (%d): %s", len(verifier), verifier) + } + + // base64 length: n = 64; 4*(64/3) = 85.3; round to nearest 4: 88 + if len(selector) != 88 { + t.Errorf("selector length was wrong (%d): %s", len(selector), selector) + } + + // base64 length: n = 64; 4*(64/3) = 85.33; round to nearest 4: 88 + if len(token) != 88 { + t.Errorf("token length was wrong (%d): %s", len(token), token) } rawToken, err := base64.URLEncoding.DecodeString(token) @@ -441,13 +455,21 @@ func TestGenerateToken(t *testing.T) { t.Error(err) } - rawHash, err := base64.StdEncoding.DecodeString(hash) + rawSelector, err := base64.StdEncoding.DecodeString(selector) + if err != nil { + t.Error(err) + } + rawVerifier, err := base64.StdEncoding.DecodeString(verifier) if err != nil { t.Error(err) } - checkHash := sha512.Sum512(rawToken) - if 0 != bytes.Compare(checkHash[:], rawHash) { - t.Error("expected hashes to match") + checkSelector := sha512.Sum512(rawToken[:32]) + if 0 != bytes.Compare(checkSelector[:], rawSelector) { + t.Error("expected selector to match") + } + checkVerifier := sha512.Sum512(rawToken[32:]) + if 0 != bytes.Compare(checkVerifier[:], rawVerifier) { + t.Error("expected verifier to match") } } diff --git a/storage.go b/storage.go index 45624af..459577b 100644 --- a/storage.go +++ b/storage.go @@ -80,18 +80,18 @@ type OAuth2ServerStorer interface { type ConfirmingServerStorer interface { ServerStorer - // LoadByConfirmToken finds a user by his confirm token field + // LoadByConfirmSelector finds a user by his confirm selector field // and should return ErrUserNotFound if that user cannot be found. - LoadByConfirmToken(ctx context.Context, token string) (ConfirmableUser, error) + LoadByConfirmSelector(ctx context.Context, selector string) (ConfirmableUser, error) } // RecoveringServerStorer allows users to be recovered by a token type RecoveringServerStorer interface { ServerStorer - // LoadByRecoverToken finds a user by his recover token field + // LoadByRecoverSelector finds a user by his recover selector field // and should return ErrUserNotFound if that user cannot be found. - LoadByRecoverToken(ctx context.Context, token string) (RecoverableUser, error) + LoadByRecoverSelector(ctx context.Context, selector string) (RecoverableUser, error) } // RememberingServerStorer allows users to be remembered across sessions diff --git a/user.go b/user.go index f050594..d222042 100644 --- a/user.go +++ b/user.go @@ -34,13 +34,15 @@ type AuthableUser interface { type ConfirmableUser interface { User - GetConfirmed() (confirmed bool) - GetConfirmToken() (token string) GetEmail() (email string) + GetConfirmed() (confirmed bool) + GetConfirmSelector() (selector string) + GetConfirmVerifier() (verifier string) - PutConfirmed(confirmed bool) - PutConfirmToken(token string) PutEmail(email string) + PutConfirmed(confirmed bool) + PutConfirmSelector(selector string) + PutConfirmVerifier(verifier string) } // LockableUser is a user that can be locked @@ -61,11 +63,13 @@ type RecoverableUser interface { AuthableUser GetEmail() (email string) - GetRecoverToken() (token string) + GetRecoverSelector() (selector string) + GetRecoverVerifier() (verifier string) GetRecoverExpiry() (expiry time.Time) PutEmail(email string) - PutRecoverToken(token string) + PutRecoverSelector(selector string) + PutRecoverVerifier(verifier string) PutRecoverExpiry(expiry time.Time) }