Skip to content

Commit

Permalink
Change ordering of user lookup vs. password hashing (#5614)
Browse files Browse the repository at this point in the history
* Change ordering of user lookup vs. password hashing

This fixes a very minor information leak where someone could brute force
the existence of a username. It's not perfect as the underlying storage
plays a part but bcrypt's slowness puts that much more in the noise.
  • Loading branch information
jefferai authored Oct 27, 2018
1 parent 17f8834 commit d2c23d5
Showing 1 changed file with 35 additions and 14 deletions.
49 changes: 35 additions & 14 deletions builtin/credential/userpass/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,32 +62,53 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framew
}

// Get the user and validate auth
user, err := b.user(ctx, req.Storage, username)
if err != nil {
return nil, err
}
if user == nil {
return logical.ErrorResponse("invalid username or password"), nil
}

// Check for a CIDR match.
if !cidrutil.RemoteAddrIsOk(req.Connection.RemoteAddr, user.BoundCIDRs) {
return logical.ErrorResponse("login request originated from invalid CIDR"), nil
user, userError := b.user(ctx, req.Storage, username)

var userPassword []byte
var err error
var legacyPassword bool
// If there was an error or it's nil, we fake a password for the bcrypt
// check so as not to have a timing leak. Specifics of the underlying
// storage still leaks a bit but generally much more in the noise compared
// to bcrypt.
if user != nil && userError == nil {
if user.PasswordHash == nil {
userPassword = []byte(user.Password)
legacyPassword = true
} else {
userPassword = user.PasswordHash
}
} else {
// This is still acceptable as bcrypt will still make sure it takes
// a long time, it's just nicer to be random if possible
userPassword = []byte("dummy")
}

// Check for a password match. Check for a hash collision for Vault 0.2+,
// but handle the older legacy passwords with a constant time comparison.
passwordBytes := []byte(password)
if user.PasswordHash != nil {
if err := bcrypt.CompareHashAndPassword(user.PasswordHash, passwordBytes); err != nil {
if !legacyPassword {
if err := bcrypt.CompareHashAndPassword(userPassword, passwordBytes); err != nil {
return logical.ErrorResponse("invalid username or password"), nil
}
} else {
if subtle.ConstantTimeCompare([]byte(user.Password), passwordBytes) != 1 {
if subtle.ConstantTimeCompare(userPassword, passwordBytes) != 1 {
return logical.ErrorResponse("invalid username or password"), nil
}
}

if userError != nil {
return nil, userError
}
if user == nil {
return logical.ErrorResponse("invalid username or password"), nil
}

// Check for a CIDR match.
if !cidrutil.RemoteAddrIsOk(req.Connection.RemoteAddr, user.BoundCIDRs) {
return logical.ErrorResponse("login request originated from invalid CIDR"), nil
}

return &logical.Response{
Auth: &logical.Auth{
Policies: user.Policies,
Expand Down

0 comments on commit d2c23d5

Please sign in to comment.