From fb9706912d14b9a4e73eb9cfe3fb7f9c4b0627b8 Mon Sep 17 00:00:00 2001 From: Zach Wasserman Date: Wed, 24 Mar 2021 19:36:30 -0700 Subject: [PATCH] Prevent user enumeration (#533) - Return same error in all cases for login endpoint. - Log error details in server logs. - Make most login errors take ~1s to prevent timing attacks. - Don't return forgot password errors. - Log password errors in server logs. - Make most forgot password requests take ~1s to prevent timing attacks. Fixes #531 --- cmd/fleetctl/login.go | 4 +- cypress/integration/sessions/sessions.spec.ts | 2 +- server/service/client_errors.go | 15 ----- server/service/client_sessions.go | 2 - server/service/endpoint_middleware.go | 18 +++--- server/service/endpoint_sessions.go | 2 +- server/service/endpoint_users.go | 10 +-- server/service/logging.go | 16 +++-- server/service/service_errors.go | 61 ++++++++++++++----- server/service/service_sessions.go | 40 ++++++------ server/service/service_users.go | 6 ++ server/service/transport_error.go | 11 +++- 12 files changed, 113 insertions(+), 74 deletions(-) diff --git a/cmd/fleetctl/login.go b/cmd/fleetctl/login.go index 7ff71e2c3b..7abf69499f 100644 --- a/cmd/fleetctl/login.go +++ b/cmd/fleetctl/login.go @@ -71,12 +71,10 @@ Interactively prompts for email and password if not specified in the flags or en token, err := fleet.Login(flEmail, flPassword) if err != nil { switch err.(type) { - case service.InvalidLoginErr: - return err case service.NotSetupErr: return err } - return errors.Wrap(err, "error logging in") + return errors.Wrap(err, "Login failed") } configPath, context := c.String("config"), c.String("context") diff --git a/cypress/integration/sessions/sessions.spec.ts b/cypress/integration/sessions/sessions.spec.ts index 2ce2a0b884..dda6ff86af 100644 --- a/cypress/integration/sessions/sessions.spec.ts +++ b/cypress/integration/sessions/sessions.spec.ts @@ -40,7 +40,7 @@ describe('Sessions', () => { .click(); cy.url().should('match', /\/login$/); - cy.contains('username or email and password do not match'); + cy.contains('Authentication failed'); }); it('Fails to access authenticated resource', () => { diff --git a/server/service/client_errors.go b/server/service/client_errors.go index 455cd28990..81543153d8 100644 --- a/server/service/client_errors.go +++ b/server/service/client_errors.go @@ -20,21 +20,6 @@ func (e setupAlreadyErr) SetupAlready() bool { return true } -type InvalidLoginErr interface { - InvalidLogin() bool - Error() string -} - -type invalidLoginErr struct{} - -func (e invalidLoginErr) Error() string { - return "The credentials supplied were invalid" -} - -func (e invalidLoginErr) InvalidLogin() bool { - return true -} - type NotSetupErr interface { NotSetup() bool Error() string diff --git a/server/service/client_sessions.go b/server/service/client_sessions.go index 7ebf45b022..ae92b21c54 100644 --- a/server/service/client_sessions.go +++ b/server/service/client_sessions.go @@ -24,8 +24,6 @@ func (c *Client) Login(email, password string) (string, error) { switch response.StatusCode { case http.StatusNotFound: return "", notSetupErr{} - case http.StatusUnauthorized: - return "", invalidLoginErr{} } if response.StatusCode != http.StatusOK { return "", errors.Errorf( diff --git a/server/service/endpoint_middleware.go b/server/service/endpoint_middleware.go index e5ac8141a8..89a7a47001 100644 --- a/server/service/endpoint_middleware.go +++ b/server/service/endpoint_middleware.go @@ -70,7 +70,7 @@ func authenticatedUser(jwtKey string, svc kolide.Service, next endpoint.Endpoint // if not succesful, try again this time with errors bearer, ok := token.FromContext(ctx) if !ok { - return nil, authError{reason: "no auth token"} + return nil, authRequiredError{internal: "no auth token"} } v, err := authViewer(ctx, jwtKey, bearer, svc) @@ -92,30 +92,30 @@ func authViewer(ctx context.Context, jwtKey string, bearerToken token.Token, svc return []byte(jwtKey), nil }) if err != nil { - return nil, authError{reason: err.Error()} + return nil, authRequiredError{internal: err.Error()} } - if jwtToken.Valid != true { - return nil, authError{reason: "invalid jwt token"} + if !jwtToken.Valid { + return nil, authRequiredError{internal: "invalid jwt token"} } claims, ok := jwtToken.Claims.(jwt.MapClaims) if !ok { - return nil, authError{reason: "no jwt claims"} + return nil, authRequiredError{internal: "no jwt claims"} } sessionKeyClaim, ok := claims["session_key"] if !ok { - return nil, authError{reason: "no session_key in JWT claims"} + return nil, authRequiredError{internal: "no session_key in JWT claims"} } sessionKey, ok := sessionKeyClaim.(string) if !ok { - return nil, authError{reason: "non-string key in sessionClaim"} + return nil, authRequiredError{internal: "non-string key in sessionClaim"} } session, err := svc.GetSessionByKey(ctx, sessionKey) if err != nil { - return nil, authError{reason: err.Error()} + return nil, authRequiredError{internal: err.Error()} } user, err := svc.User(ctx, session.UserID) if err != nil { - return nil, authError{reason: err.Error()} + return nil, authRequiredError{internal: err.Error()} } return &viewer.Viewer{User: user, Session: session}, nil } diff --git a/server/service/endpoint_sessions.go b/server/service/endpoint_sessions.go index 328bd1c727..1116b121c7 100644 --- a/server/service/endpoint_sessions.go +++ b/server/service/endpoint_sessions.go @@ -6,8 +6,8 @@ import ( "html/template" "time" - "github.com/go-kit/kit/endpoint" "github.com/fleetdm/fleet/server/kolide" + "github.com/go-kit/kit/endpoint" ) //////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/endpoint_users.go b/server/service/endpoint_users.go index 4103a5d5d7..dc2c1376f4 100644 --- a/server/service/endpoint_users.go +++ b/server/service/endpoint_users.go @@ -4,8 +4,8 @@ import ( "context" "net/http" - "github.com/go-kit/kit/endpoint" "github.com/fleetdm/fleet/server/kolide" + "github.com/go-kit/kit/endpoint" ) //////////////////////////////////////////////////////////////////////////////// @@ -308,10 +308,10 @@ func (r forgotPasswordResponse) status() int { return http.StatusAccepted } func makeForgotPasswordEndpoint(svc kolide.Service) endpoint.Endpoint { return func(ctx context.Context, request interface{}) (interface{}, error) { req := request.(forgotPasswordRequest) - err := svc.RequestPasswordReset(ctx, req.Email) - if err != nil { - return forgotPasswordResponse{Err: err}, nil - } + // Any error returned by the service should not be returned to the + // client to prevent information disclosure (it will be logged in the + // server logs). + _ = svc.RequestPasswordReset(ctx, req.Email) return forgotPasswordResponse{}, nil } } diff --git a/server/service/logging.go b/server/service/logging.go index f0f5ced8a6..e5acbcb2cc 100644 --- a/server/service/logging.go +++ b/server/service/logging.go @@ -19,13 +19,21 @@ func NewLoggingService(svc kolide.Service, logger kitlog.Logger) kolide.Service // loggerDebug returns the the info level if there error is non-nil, otherwise defaulting to the debug level. func (mw loggingMiddleware) loggerDebug(err error) kitlog.Logger { - if err != nil { - return level.Info(mw.logger) + logger := mw.logger + if e, ok := err.(ErrWithInternal); ok { + logger = kitlog.With(logger, "err_internal", e.Internal()) } - return level.Debug(mw.logger) + if err != nil { + return level.Info(logger) + } + return level.Debug(logger) } // loggerInfo returns the info level func (mw loggingMiddleware) loggerInfo(err error) kitlog.Logger { - return level.Info(mw.logger) + logger := mw.logger + if e, ok := err.(ErrWithInternal); ok { + logger = kitlog.With(logger, "err_internal", e.Internal()) + } + return level.Info(logger) } diff --git a/server/service/service_errors.go b/server/service/service_errors.go index 275652a918..56af71fa52 100644 --- a/server/service/service_errors.go +++ b/server/service/service_errors.go @@ -1,6 +1,23 @@ package service -import "fmt" +import ( + "fmt" + "net/http" +) + +// ErrWithInternal is an interface for errors that include extra "internal" +// information that should be logged in server logs but not sent to clients. +type ErrWithInternal interface { + error + Internal() string +} + +// ErrWithStatusCode is an interface for errors that should set a specific HTTP +// status when encoding. +type ErrWithStatusCode interface { + error + StatusCode() int +} type invalidArgumentError []invalidArgument type invalidArgument struct { @@ -57,24 +74,38 @@ func (e invalidArgumentError) Invalid() []map[string]string { return invalid } -// authentication error -type authError struct { - reason string - // client reason is used to provide - // a different error message to the client - // when security is a concern - clientReason string +type authFailedError struct { + // internal is the reason that should only be logged internally + internal string } -func (e authError) Error() string { - return e.reason +func (e authFailedError) Error() string { + return "Authentication failed" } -func (e authError) AuthError() string { - if e.clientReason != "" { - return e.clientReason - } - return "username or email and password do not match" +func (e authFailedError) Internal() string { + return e.internal +} + +func (e authFailedError) StatusCode() int { + return http.StatusUnauthorized +} + +type authRequiredError struct { + // internal is the reason that should only be logged internally + internal string +} + +func (e authRequiredError) Error() string { + return "Authentication required" +} + +func (e authRequiredError) Internal() string { + return e.internal +} + +func (e authRequiredError) StatusCode() int { + return http.StatusUnauthorized } // permissionError, set when user is authenticated, but not allowed to perform action diff --git a/server/service/service_sessions.go b/server/service/service_sessions.go index 0ff33e595e..caef5b5623 100644 --- a/server/service/service_sessions.go +++ b/server/service/service_sessions.go @@ -123,26 +123,38 @@ func (svc service) CallbackSSO(ctx context.Context, auth kolide.Auth) (*kolide.S } func (svc service) Login(ctx context.Context, username, password string) (*kolide.User, string, error) { + // If there is an error, sleep until the request has taken at least 1 + // second. This means that generally a login failure for any reason will + // take ~1s and frustrate a timing attack. + var err error + defer func(start time.Time) { + if err != nil { + time.Sleep(time.Until(start.Add(1 * time.Second))) + } + }(time.Now()) + user, err := svc.userByEmailOrUsername(username) if _, ok := err.(kolide.NotFoundError); ok { - return nil, "", authError{reason: "no such user"} + return nil, "", authFailedError{internal: "user not found"} } if err != nil { - return nil, "", err + return nil, "", authFailedError{internal: err.Error()} } + + if err = user.ValidatePassword(password); err != nil { + return nil, "", authFailedError{internal: "invalid password"} + } + if !user.Enabled { - return nil, "", authError{reason: "account disabled", clientReason: "account disabled"} + return nil, "", authFailedError{internal: "account disabled"} } if user.SSOEnabled { - const errMessage = "password login not allowed for single sign on users" - return nil, "", authError{reason: errMessage, clientReason: errMessage} - } - if err = user.ValidatePassword(password); err != nil { - return nil, "", authError{reason: "bad password"} + return nil, "", authFailedError{internal: "password login disabled for sso users"} } + token, err := svc.makeSession(user.ID) if err != nil { - return nil, "", err + return nil, "", authFailedError{internal: err.Error()} } return user, token, nil @@ -261,10 +273,7 @@ func (svc service) DeleteSession(ctx context.Context, id uint) error { func (svc service) validateSession(session *kolide.Session) error { if session == nil { - return authError{ - reason: "active session not present", - clientReason: "session error", - } + return authRequiredError{internal: "active session not present"} } sessionDuration := svc.config.Session.Duration @@ -274,10 +283,7 @@ func (svc service) validateSession(session *kolide.Session) error { if err != nil { return errors.Wrap(err, "destroying session") } - return authError{ - reason: "expired session", - clientReason: "session error", - } + return authRequiredError{internal: "expired session"} } return svc.ds.MarkSessionAccessed(session) diff --git a/server/service/service_users.go b/server/service/service_users.go index 968543119e..81c6b49c31 100644 --- a/server/service/service_users.go +++ b/server/service/service_users.go @@ -322,6 +322,12 @@ func (svc service) RequirePasswordReset(ctx context.Context, uid uint, require b } func (svc service) RequestPasswordReset(ctx context.Context, email string) error { + // Regardless of error, sleep until the request has taken at least 1 second. + // This means that any request to this method will take ~1s and frustrate a timing attack. + defer func(start time.Time) { + time.Sleep(time.Until(start.Add(1 * time.Second))) + }(time.Now()) + user, err := svc.ds.UserByEmail(email) if err != nil { return err diff --git a/server/service/transport_error.go b/server/service/transport_error.go index 45ef305807..d069f6d1d2 100644 --- a/server/service/transport_error.go +++ b/server/service/transport_error.go @@ -149,9 +149,16 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) { return } - w.WriteHeader(http.StatusInternalServerError) + // Get specific status code if it is available from this error type, + // defaulting to HTTP 500 + status := http.StatusInternalServerError + if e, ok := err.(ErrWithStatusCode); ok { + status = e.StatusCode() + } + + w.WriteHeader(status) je := jsonError{ - Message: "Unknown Error", + Message: err.Error(), Errors: baseError(err.Error()), } enc.Encode(je)