From be4228db5a43f65a989239f891185d45912d39ad Mon Sep 17 00:00:00 2001 From: Misi Date: Mon, 24 Oct 2022 20:53:02 +0200 Subject: [PATCH] Security fix for privilege escalation (#598) * Prevent external users to update their information * Trim leading and trailing whitespaces from email and username on signup * Check whether the provided email address is the same as where the invitation sent --- pkg/api/admin_users.go | 5 + pkg/api/org_invite.go | 27 +++- pkg/api/signup.go | 15 ++- pkg/api/user.go | 39 +++++- pkg/api/user_test.go | 117 ++++++++++++++++++ pkg/api/utils.go | 18 ++- pkg/services/login/logintest/logintest.go | 2 + .../app/core/components/Signup/SignupPage.tsx | 3 +- .../core/components/Signup/VerifyEmail.tsx | 3 +- public/app/features/admin/UserProfile.tsx | 8 +- public/app/features/admin/utils.ts | 4 + public/app/features/invites/SignupInvited.tsx | 4 +- 12 files changed, 229 insertions(+), 16 deletions(-) diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index c9fa2407533..31d81504e9d 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -19,6 +20,10 @@ func (hs *HTTPServer) AdminCreateUser(c *models.ReqContext) response.Response { if err := web.Bind(c.Req, &form); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } + + form.Email = strings.TrimSpace(form.Email) + form.Login = strings.TrimSpace(form.Login) + cmd := models.CreateUserCommand{ Login: form.Login, Email: form.Email, diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index 0071e3f4275..f8fb6cf6ed1 100644 --- a/pkg/api/org_invite.go +++ b/pkg/api/org_invite.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -186,21 +187,37 @@ func (hs *HTTPServer) GetInviteInfoByCode(c *models.ReqContext) response.Respons func (hs *HTTPServer) CompleteInvite(c *models.ReqContext) response.Response { completeInvite := dtos.CompleteInviteForm{} - if err := web.Bind(c.Req, &completeInvite); err != nil { + var err error + if err = web.Bind(c.Req, &completeInvite); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } - query := models.GetTempUserByCodeQuery{Code: completeInvite.InviteCode} + completeInvite.Email, err = ValidateAndNormalizeEmail(completeInvite.Email) + if err != nil { + return response.Error(http.StatusBadRequest, "Invalid email address provided", nil) + } + + completeInvite.Username = strings.TrimSpace(completeInvite.Username) + + query := models.GetTempUserByCodeQuery{Code: completeInvite.InviteCode} if err := hs.SQLStore.GetTempUserByCode(c.Req.Context(), &query); err != nil { if errors.Is(err, models.ErrTempUserNotFound) { - return response.Error(404, "Invite not found", nil) + return response.Error(http.StatusNotFound, "Invite not found", nil) } - return response.Error(500, "Failed to get invite", err) + return response.Error(http.StatusInternalServerError, "Failed to get invite", err) } invite := query.Result if invite.Status != models.TmpUserInvitePending { - return response.Error(412, fmt.Sprintf("Invite cannot be used in status %s", invite.Status), nil) + return response.Error(http.StatusPreconditionFailed, fmt.Sprintf("Invite cannot be used in status %s", invite.Status), nil) + } + + // In case the user is invited by email address + if inviteMail, err := ValidateAndNormalizeEmail(invite.Email); err == nil { + // Make sure that the email address is not amended + if completeInvite.Email != inviteMail { + return response.Error(http.StatusBadRequest, "The provided email is different from the address that is found in the invite", nil) + } } cmd := models.CreateUserCommand{ diff --git a/pkg/api/signup.go b/pkg/api/signup.go index f3568d03daf..f04e33a845b 100644 --- a/pkg/api/signup.go +++ b/pkg/api/signup.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -27,15 +28,21 @@ func GetSignUpOptions(c *models.ReqContext) response.Response { // POST /api/user/signup func (hs *HTTPServer) SignUp(c *models.ReqContext) response.Response { form := dtos.SignUpForm{} - if err := web.Bind(c.Req, &form); err != nil { + var err error + if err = web.Bind(c.Req, &form); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } if !setting.AllowUserSignUp { return response.Error(401, "User signup is disabled", nil) } + form.Email, err = ValidateAndNormalizeEmail(form.Email) + if err != nil { + return response.Error(http.StatusBadRequest, "Invalid email address", nil) + } + existing := models.GetUserByLoginQuery{LoginOrEmail: form.Email} - if err := hs.SQLStore.GetUserByLogin(c.Req.Context(), &existing); err == nil { + if err = hs.SQLStore.GetUserByLogin(c.Req.Context(), &existing); err == nil { return response.Error(422, "User with same email address already exists", nil) } @@ -44,7 +51,6 @@ func (hs *HTTPServer) SignUp(c *models.ReqContext) response.Response { cmd.Email = form.Email cmd.Status = models.TmpUserSignUpStarted cmd.InvitedByUserId = c.UserId - var err error cmd.Code, err = util.GetRandomString(20) if err != nil { return response.Error(500, "Failed to generate random string", err) @@ -76,6 +82,9 @@ func (hs *HTTPServer) SignUpStep2(c *models.ReqContext) response.Response { return response.Error(401, "User signup is disabled", nil) } + form.Email = strings.TrimSpace(form.Email) + form.Username = strings.TrimSpace(form.Username) + createUserCmd := models.CreateUserCommand{ Email: form.Email, Login: form.Username, diff --git a/pkg/api/user.go b/pkg/api/user.go index 95b69d0b462..4cb353b9ed5 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "strconv" + "strings" "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" @@ -79,9 +80,14 @@ func (hs *HTTPServer) GetUserByLoginOrEmail(c *models.ReqContext) response.Respo // POST /api/user func (hs *HTTPServer) UpdateSignedInUser(c *models.ReqContext) response.Response { cmd := models.UpdateUserCommand{} - if err := web.Bind(c.Req, &cmd); err != nil { + var err error + if err = web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } + + cmd.Email = strings.TrimSpace(cmd.Email) + cmd.Login = strings.TrimSpace(cmd.Login) + if setting.AuthProxyEnabled { if setting.AuthProxyHeaderProperty == "email" && cmd.Email != c.Email { return response.Error(400, "Not allowed to change email when auth proxy is using email property", nil) @@ -98,13 +104,18 @@ func (hs *HTTPServer) UpdateSignedInUser(c *models.ReqContext) response.Response func (hs *HTTPServer) UpdateUser(c *models.ReqContext) response.Response { cmd := models.UpdateUserCommand{} var err error - if err := web.Bind(c.Req, &cmd); err != nil { + if err = web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } + + cmd.Email = strings.TrimSpace(cmd.Email) + cmd.Login = strings.TrimSpace(cmd.Login) + cmd.UserId, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) if err != nil { return response.Error(http.StatusBadRequest, "id is invalid", err) } + return hs.handleUpdateUser(c.Req.Context(), cmd) } @@ -133,6 +144,16 @@ func (hs *HTTPServer) UpdateUserActiveOrg(c *models.ReqContext) response.Respons } func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd models.UpdateUserCommand) response.Response { + // external user -> user data cannot be updated + isExternal, err := hs.isExternalUser(ctx, cmd.UserId) + if err != nil { + return response.Error(http.StatusInternalServerError, "Failed to validate User", err) + } + + if isExternal { + return response.Error(http.StatusForbidden, "User info cannot be updated for external Users", nil) + } + if len(cmd.Login) == 0 { cmd.Login = cmd.Email if len(cmd.Login) == 0 { @@ -147,6 +168,20 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd models.UpdateUse return response.Success("User updated") } +func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, error) { + getAuthQuery := models.GetAuthInfoQuery{UserId: userID} + var err error + if err = hs.authInfoService.GetAuthInfo(ctx, &getAuthQuery); err == nil { + return true, nil + } + + if errors.Is(err, models.ErrUserNotFound) { + return false, nil + } + + return false, err +} + // GET /api/user/orgs func (hs *HTTPServer) GetSignedInUserOrgList(c *models.ReqContext) response.Response { return hs.getUserOrgList(c.Req.Context(), c.UserId) diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 0a19f3ef5e8..05f9306a8bd 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -13,11 +13,14 @@ import ( "golang.org/x/oauth2" "github.com/grafana/grafana/pkg/api/dtos" + "github.com/grafana/grafana/pkg/api/response" + "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/login/authinfoservice" authinfostore "github.com/grafana/grafana/pkg/services/login/authinfoservice/database" + "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/searchusers/filters" "github.com/grafana/grafana/pkg/services/secrets/database" @@ -184,3 +187,117 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) { assert.Equal(t, 10, respJSON.Get("perPage").MustInt()) }, mock) } + +func TestHTTPServer_UpdateUser(t *testing.T) { + settings := setting.NewCfg() + sqlStore := sqlstore.InitTestDB(t) + + hs := &HTTPServer{ + Cfg: settings, + SQLStore: sqlStore, + AccessControl: acmock.New(), + } + + updateUserCommand := models.UpdateUserCommand{ + Email: fmt.Sprint("admin", "@test.com"), + Name: "admin", + Login: "admin", + UserId: 1, + } + + updateUserScenario(t, updateUserContext{ + desc: "Should return 403 when the current User is an external user", + url: "/api/users/1", + routePattern: "/api/users/:id", + cmd: updateUserCommand, + fn: func(sc *scenarioContext) { + sc.authInfoService.ExpectedUserAuth = &models.UserAuth{} + sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec() + assert.Equal(t, 403, sc.resp.Code) + }, + }, hs) +} + +type updateUserContext struct { + desc string + url string + routePattern string + cmd models.UpdateUserCommand + fn scenarioFunc +} + +func updateUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) { + t.Run(fmt.Sprintf("%s %s", ctx.desc, ctx.url), func(t *testing.T) { + sc := setupScenarioContext(t, ctx.url) + + sc.authInfoService = &logintest.AuthInfoServiceFake{} + hs.authInfoService = sc.authInfoService + + sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { + c.Req.Body = mockRequestBody(ctx.cmd) + c.Req.Header.Add("Content-Type", "application/json") + sc.context = c + sc.context.OrgId = testOrgID + sc.context.UserId = testUserID + + return hs.UpdateUser(c) + }) + + sc.m.Put(ctx.routePattern, sc.defaultHandler) + + ctx.fn(sc) + }) +} + +func TestHTTPServer_UpdateSignedInUser(t *testing.T) { + settings := setting.NewCfg() + sqlStore := sqlstore.InitTestDB(t) + + hs := &HTTPServer{ + Cfg: settings, + SQLStore: sqlStore, + AccessControl: acmock.New(), + } + + updateUserCommand := models.UpdateUserCommand{ + Email: fmt.Sprint("admin", "@test.com"), + Name: "admin", + Login: "admin", + UserId: 1, + } + + updateSignedInUserScenario(t, updateUserContext{ + desc: "Should return 403 when the current User is an external user", + url: "/api/users/", + routePattern: "/api/users/", + cmd: updateUserCommand, + fn: func(sc *scenarioContext) { + sc.authInfoService.ExpectedUserAuth = &models.UserAuth{} + sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec() + assert.Equal(t, 403, sc.resp.Code) + }, + }, hs) +} + +func updateSignedInUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) { + t.Run(fmt.Sprintf("%s %s", ctx.desc, ctx.url), func(t *testing.T) { + sc := setupScenarioContext(t, ctx.url) + + sc.authInfoService = &logintest.AuthInfoServiceFake{} + hs.authInfoService = sc.authInfoService + + sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { + c.Req.Body = mockRequestBody(ctx.cmd) + c.Req.Header.Add("Content-Type", "application/json") + sc.context = c + sc.context.OrgId = testOrgID + sc.context.UserId = testUserID + + return hs.UpdateSignedInUser(c) + }) + + sc.m.Put(ctx.routePattern, sc.defaultHandler) + + ctx.fn(sc) + }) +} diff --git a/pkg/api/utils.go b/pkg/api/utils.go index d673b03af1b..716e8b98b60 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -1,9 +1,25 @@ package api -import "encoding/json" +import ( + "encoding/json" + "net/mail" +) func jsonMap(data []byte) (map[string]string, error) { jsonMap := make(map[string]string) err := json.Unmarshal(data, &jsonMap) return jsonMap, err } + +func ValidateAndNormalizeEmail(email string) (string, error) { + if email == "" { + return "", nil + } + + e, err := mail.ParseAddress(email) + if err != nil { + return "", err + } + + return e.Address, nil +} diff --git a/pkg/services/login/logintest/logintest.go b/pkg/services/login/logintest/logintest.go index bc24c855d20..2714da4041c 100644 --- a/pkg/services/login/logintest/logintest.go +++ b/pkg/services/login/logintest/logintest.go @@ -22,6 +22,7 @@ func (l *LoginServiceFake) SetTeamSyncFunc(login.TeamSyncFunc) {} type AuthInfoServiceFake struct { LatestUserID int64 + ExpectedUserAuth *models.UserAuth ExpectedUser *models.User ExpectedExternalUser *models.ExternalUserInfo ExpectedError error @@ -38,6 +39,7 @@ func (a *AuthInfoServiceFake) LookupAndUpdate(ctx context.Context, query *models func (a *AuthInfoServiceFake) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error { a.LatestUserID = query.UserId + query.Result = a.ExpectedUserAuth return a.ExpectedError } diff --git a/public/app/core/components/Signup/SignupPage.tsx b/public/app/core/components/Signup/SignupPage.tsx index 3d6a6188475..c757bb753fe 100644 --- a/public/app/core/components/Signup/SignupPage.tsx +++ b/public/app/core/components/Signup/SignupPage.tsx @@ -5,6 +5,7 @@ import { Form, Field, Input, Button, HorizontalGroup, LinkButton, FormAPI } from import { getConfig } from 'app/core/config'; import { useAppNotification } from 'app/core/copy/appNotification'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; +import { w3cStandardEmailValidator } from 'app/features/admin/utils'; import { InnerBox, LoginLayout } from '../Login/LoginLayout'; import { PasswordField } from '../PasswordField/PasswordField'; @@ -74,7 +75,7 @@ export const SignupPage: FC = (props) => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })} diff --git a/public/app/core/components/Signup/VerifyEmail.tsx b/public/app/core/components/Signup/VerifyEmail.tsx index 81eab2cc66b..d75f3dc22eb 100644 --- a/public/app/core/components/Signup/VerifyEmail.tsx +++ b/public/app/core/components/Signup/VerifyEmail.tsx @@ -4,6 +4,7 @@ import { getBackendSrv } from '@grafana/runtime'; import { Form, Field, Input, Button, Legend, Container, HorizontalGroup, LinkButton } from '@grafana/ui'; import { getConfig } from 'app/core/config'; import { useAppNotification } from 'app/core/copy/appNotification'; +import { w3cStandardEmailValidator } from 'app/features/admin/utils'; interface EmailDTO { email: string; @@ -53,7 +54,7 @@ export const VerifyEmail: FC = () => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })} diff --git a/public/app/features/admin/UserProfile.tsx b/public/app/features/admin/UserProfile.tsx index fe9b7646db5..4886d484d44 100644 --- a/public/app/features/admin/UserProfile.tsx +++ b/public/app/features/admin/UserProfile.tsx @@ -225,7 +225,9 @@ export class UserProfileRow extends PureComponent, status?: LegacyInputStatus) => { @@ -233,7 +235,9 @@ export class UserProfileRow extends PureComponent { diff --git a/public/app/features/admin/utils.ts b/public/app/features/admin/utils.ts index f4bdf531730..246f1279433 100644 --- a/public/app/features/admin/utils.ts +++ b/public/app/features/admin/utils.ts @@ -1,5 +1,9 @@ import { config } from '@grafana/runtime/src'; +// https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address +export const w3cStandardEmailValidator = + /^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/; + export function isTrial() { const expiry = config.licenseInfo?.trialExpiry; return !!(expiry && expiry > 0); diff --git a/public/app/features/invites/SignupInvited.tsx b/public/app/features/invites/SignupInvited.tsx index 4d862d6377d..4fbe80b8892 100644 --- a/public/app/features/invites/SignupInvited.tsx +++ b/public/app/features/invites/SignupInvited.tsx @@ -8,6 +8,8 @@ import { getConfig } from 'app/core/config'; import { contextSrv } from 'app/core/core'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; +import { w3cStandardEmailValidator } from '../admin/utils'; + interface FormModel { email: string; name?: string; @@ -77,7 +79,7 @@ export const SignupInvitedPage: FC = ({ match }) => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })}