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,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,

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{

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,

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)

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)
+	})
+}

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
+}

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
 }
 

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> = (props) => {
                   {...register('email', {
                     required: 'Email is required',
                     pattern: {
-                      value: /^\S+@\S+$/,
+                      value: w3cStandardEmailValidator,
                       message: 'Email is invalid',
                     },
                   })}