From fe8930394fd5bf88fb6008e2fe313825a9e1790f Mon Sep 17 00:00:00 2001 From: Jo Date: Thu, 15 Sep 2022 16:07:41 +0200 Subject: [PATCH] Fix: Choose Lookup params per auth module. Fixes CVE-2022-31107 (#55176) --- pkg/api/ldap_debug.go | 5 ++++ pkg/api/login_oauth.go | 5 ++++ pkg/login/ldap_login.go | 5 ++++ pkg/models/user_auth.go | 19 +++++++++----- .../contexthandler/authproxy/authproxy.go | 10 +++++++ pkg/services/login/login.go | 8 +++--- pkg/services/login/login_test.go | 7 +++-- pkg/services/sqlstore/user_auth.go | 17 +++++++----- pkg/services/sqlstore/user_auth_test.go | 26 ++++++++++++------- pkg/services/sqlstore/user_test.go | 17 +++++++++--- 10 files changed, 85 insertions(+), 34 deletions(-) diff --git a/pkg/api/ldap_debug.go b/pkg/api/ldap_debug.go index 126e760b671..c9e2b606c5c 100644 --- a/pkg/api/ldap_debug.go +++ b/pkg/api/ldap_debug.go @@ -215,6 +215,11 @@ func (hs *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) response.Respon ReqContext: c, ExternalUser: user, SignupAllowed: hs.Cfg.LDAPAllowSignup, + UserLookupParams: models.UserLookupParams{ + UserID: &query.Result.Id, // Upsert by ID only + Email: nil, + Login: nil, + }, } err = bus.Dispatch(upsertCmd) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 1fce9b6f610..611d51444fb 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -250,6 +250,11 @@ func syncUser( ReqContext: ctx, ExternalUser: extUser, SignupAllowed: connect.IsSignupAllowed(), + UserLookupParams: models.UserLookupParams{ + Email: &extUser.Email, + UserID: nil, + Login: nil, + }, } if err := bus.Dispatch(cmd); err != nil { return nil, err diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index cb5d984e736..45ea0fc4473 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -56,6 +56,11 @@ var loginUsingLDAP = func(query *models.LoginUserQuery) (bool, error) { ReqContext: query.ReqContext, ExternalUser: externalUser, SignupAllowed: setting.LDAPAllowSignup, + UserLookupParams: models.UserLookupParams{ + Login: &externalUser.Login, + Email: &externalUser.Email, + UserID: nil, + }, } err = bus.Dispatch(upsert) if err != nil { diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index 2061cf048be..a98efe659e5 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -54,11 +54,11 @@ type RequestURIKey struct{} // COMMANDS type UpsertUserCommand struct { - ReqContext *ReqContext - ExternalUser *ExternalUserInfo + ReqContext *ReqContext + ExternalUser *ExternalUserInfo + UserLookupParams + Result *User SignupAllowed bool - - Result *User } type SetAuthInfoCommand struct { @@ -95,13 +95,18 @@ type LoginUserQuery struct { type GetUserByAuthInfoQuery struct { AuthModule string AuthId string - UserId int64 - Email string - Login string + UserLookupParams Result *User } +type UserLookupParams struct { + // Describes lookup order as well + UserID *int64 // if set, will try to find the user by id + Email *string // if set, will try to find the user by email + Login *string // if set, will try to find the user by login +} + type GetExternalUserInfoByLoginQuery struct { LoginOrEmail string diff --git a/pkg/services/contexthandler/authproxy/authproxy.go b/pkg/services/contexthandler/authproxy/authproxy.go index 80e5a5b9e0c..0d834748a71 100644 --- a/pkg/services/contexthandler/authproxy/authproxy.go +++ b/pkg/services/contexthandler/authproxy/authproxy.go @@ -246,6 +246,11 @@ func (auth *AuthProxy) LoginViaLDAP() (int64, error) { ReqContext: auth.ctx, SignupAllowed: auth.cfg.LDAPAllowSignup, ExternalUser: extUser, + UserLookupParams: models.UserLookupParams{ + Login: &extUser.Login, + Email: &extUser.Email, + UserID: nil, + }, } if err := bus.Dispatch(upsert); err != nil { return 0, err @@ -288,6 +293,11 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) { ReqContext: auth.ctx, SignupAllowed: auth.cfg.AuthProxyAutoSignUp, ExternalUser: extUser, + UserLookupParams: models.UserLookupParams{ + UserID: nil, + Login: &extUser.Login, + Email: &extUser.Email, + }, } err := bus.Dispatch(upsert) diff --git a/pkg/services/login/login.go b/pkg/services/login/login.go index 9e08a36b062..b74d1d3e8f2 100644 --- a/pkg/services/login/login.go +++ b/pkg/services/login/login.go @@ -37,11 +37,9 @@ func (ls *LoginService) UpsertUser(cmd *models.UpsertUserCommand) error { extUser := cmd.ExternalUser userQuery := &models.GetUserByAuthInfoQuery{ - AuthModule: extUser.AuthModule, - AuthId: extUser.AuthId, - UserId: extUser.UserId, - Email: extUser.Email, - Login: extUser.Login, + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + UserLookupParams: cmd.UserLookupParams, } if err := bus.Dispatch(userQuery); err != nil { if !errors.Is(err, models.ErrUserNotFound) { diff --git a/pkg/services/login/login_test.go b/pkg/services/login/login_test.go index 04953b567a1..372e244b226 100644 --- a/pkg/services/login/login_test.go +++ b/pkg/services/login/login_test.go @@ -81,11 +81,14 @@ func Test_teamSync(t *testing.T) { Bus: bus.New(), QuotaService: "a.QuotaService{}, } + email := "test_user@example.org" - upserCmd := &models.UpsertUserCommand{ExternalUser: &models.ExternalUserInfo{Email: "test_user@example.org"}} + upserCmd := &models.UpsertUserCommand{ + ExternalUser: &models.ExternalUserInfo{Email: email}, + UserLookupParams: models.UserLookupParams{Email: &email}} expectedUser := &models.User{ Id: 1, - Email: "test_user@example.org", + Email: email, Name: "test_user", Login: "test_user", } diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index 9605ccce76a..56c21ba7594 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -27,6 +27,7 @@ func GetUserByAuthInfo(query *models.GetUserByAuthInfoQuery) error { has := false var err error authQuery := &models.GetAuthInfoQuery{} + params := query.UserLookupParams // Try to find the user by auth module and id first if query.AuthModule != "" && query.AuthId != "" { @@ -40,7 +41,9 @@ func GetUserByAuthInfo(query *models.GetUserByAuthInfoQuery) error { } // if user id was specified and doesn't match the user_auth entry, remove it - if query.UserId != 0 && query.UserId != authQuery.Result.UserId { + if params.UserID != nil && + *params.UserID != 0 && + *params.UserID != authQuery.Result.UserId { err = DeleteAuthInfo(&models.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, }) @@ -71,16 +74,16 @@ func GetUserByAuthInfo(query *models.GetUserByAuthInfoQuery) error { } // If not found, try to find the user by id - if !has && query.UserId != 0 { - has, err = x.Id(query.UserId).Get(user) + if !has && params.UserID != nil && *params.UserID != 0 { + has, err = x.Id(*params.UserID).Get(user) if err != nil { return err } } // If not found, try to find the user by email address - if !has && query.Email != "" { - user = &models.User{Email: query.Email} + if !has && params.Email != nil && *params.Email != "" { + user = &models.User{Email: *params.Email} has, err = x.Get(user) if err != nil { return err @@ -88,8 +91,8 @@ func GetUserByAuthInfo(query *models.GetUserByAuthInfoQuery) error { } // If not found, try to find the user by login - if !has && query.Login != "" { - user = &models.User{Login: query.Login} + if !has && params.Login != nil && *params.Login != "" { + user = &models.User{Login: *params.Login} has, err = x.Get(user) if err != nil { return err diff --git a/pkg/services/sqlstore/user_auth_test.go b/pkg/services/sqlstore/user_auth_test.go index e5bb2379e5c..695d70b443a 100644 --- a/pkg/services/sqlstore/user_auth_test.go +++ b/pkg/services/sqlstore/user_auth_test.go @@ -1,3 +1,4 @@ +//go:build integration // +build integration package sqlstore @@ -45,7 +46,7 @@ func TestUserAuth(t *testing.T) { // By Login login := "loginuser0" - query := &models.GetUserByAuthInfoQuery{Login: login} + query := &models.GetUserByAuthInfoQuery{UserLookupParams: models.UserLookupParams{Login: &login}} err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -54,7 +55,7 @@ func TestUserAuth(t *testing.T) { // By ID id := query.Result.Id - query = &models.GetUserByAuthInfoQuery{UserId: id} + query = &models.GetUserByAuthInfoQuery{UserLookupParams: models.UserLookupParams{UserID: &id}} err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -63,7 +64,7 @@ func TestUserAuth(t *testing.T) { // By Email email := "user1@test.com" - query = &models.GetUserByAuthInfoQuery{Email: email} + query = &models.GetUserByAuthInfoQuery{UserLookupParams: models.UserLookupParams{Email: &email}} err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -72,7 +73,7 @@ func TestUserAuth(t *testing.T) { // Don't find nonexistent user email = "nonexistent@test.com" - query = &models.GetUserByAuthInfoQuery{Email: email} + query = &models.GetUserByAuthInfoQuery{UserLookupParams: models.UserLookupParams{Email: &email}} err = GetUserByAuthInfo(query) So(err, ShouldEqual, models.ErrUserNotFound) @@ -90,7 +91,7 @@ func TestUserAuth(t *testing.T) { // create user_auth entry login := "loginuser0" - query.Login = login + query.UserLookupParams.Login = &login err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -105,8 +106,9 @@ func TestUserAuth(t *testing.T) { // get with non-matching id id := query.Result.Id + idPlusOne := id + 1 - query.UserId = id + 1 + query.UserLookupParams.UserID = &idPlusOne err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -143,7 +145,9 @@ func TestUserAuth(t *testing.T) { login := "loginuser0" // Calling GetUserByAuthInfoQuery on an existing user will populate an entry in the user_auth table - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test", AuthId: "test"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err = GetUserByAuthInfo(query) So(err, ShouldBeNil) @@ -178,7 +182,9 @@ func TestUserAuth(t *testing.T) { // Calling GetUserByAuthInfoQuery on an existing user will populate an entry in the user_auth table // Make the first log-in during the past getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test1", AuthId: "test1"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "test1", AuthId: "test1", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err = GetUserByAuthInfo(query) getTime = time.Now @@ -188,7 +194,9 @@ func TestUserAuth(t *testing.T) { // Add a second auth module for this user // Have this module's last log-in be more recent getTime = func() time.Time { return time.Now().AddDate(0, 0, -1) } - query = &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test2", AuthId: "test2"} + query = &models.GetUserByAuthInfoQuery{AuthModule: "test2", AuthId: "test2", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err = GetUserByAuthInfo(query) getTime = time.Now diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go index 7da19f0ef4c..5520c0e4db3 100644 --- a/pkg/services/sqlstore/user_test.go +++ b/pkg/services/sqlstore/user_test.go @@ -1,3 +1,4 @@ +//go:build integration // +build integration package sqlstore @@ -455,7 +456,9 @@ func TestUserDataAccess(t *testing.T) { // Calling GetUserByAuthInfoQuery on an existing user will populate an entry in the user_auth table // Make the first log-in during the past getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "ldap", AuthId: "ldap0"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "ldap", AuthId: "ldap0", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err := GetUserByAuthInfo(query) getTime = time.Now @@ -465,7 +468,9 @@ func TestUserDataAccess(t *testing.T) { // Add a second auth module for this user // Have this module's last log-in be more recent getTime = func() time.Time { return time.Now().AddDate(0, 0, -1) } - query = &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "oauth", AuthId: "oauth0"} + query = &models.GetUserByAuthInfoQuery{AuthModule: "oauth", AuthId: "oauth0", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err = GetUserByAuthInfo(query) getTime = time.Now @@ -511,7 +516,9 @@ func TestUserDataAccess(t *testing.T) { // Calling GetUserByAuthInfoQuery on an existing user will populate an entry in the user_auth table // Make the first log-in during the past getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "ldap", AuthId: fmt.Sprint("ldap", i)} + query := &models.GetUserByAuthInfoQuery{AuthModule: "ldap", AuthId: fmt.Sprint("ldap", i), UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err := GetUserByAuthInfo(query) getTime = time.Now @@ -522,7 +529,9 @@ func TestUserDataAccess(t *testing.T) { // Log in first user with oauth login := "loginuser0" getTime = func() time.Time { return time.Now().AddDate(0, 0, -1) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "oauth", AuthId: "oauth0"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "oauth", AuthId: "oauth0", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} err := GetUserByAuthInfo(query) getTime = time.Now