From 21269b1dd8061503a1825ee8e90d3ff7911b3600 Mon Sep 17 00:00:00 2001
From: Zachary Wasserman <zachwass2000@gmail.com>
Date: Wed, 12 Sep 2018 14:16:59 -0700
Subject: [PATCH] Prevent escalation of user privileges via ModifyUser method

The ability to modify a users admin and enabled status was erroneously left in
place during development of https://github.com/kolide/fleet/pull/959. To
mitigate a privilege escalation vulnerability we need to ensure those values
can only be modified through the explicit methods.

This patch includes a unit test and fix for the vulnerability.

Thanks to 'Quikke' for submitting this vulnerability.
---
 server/service/service_users.go      |  8 -------
 server/service/service_users_test.go | 33 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/server/service/service_users.go b/server/service/service_users.go
index 805c18d139..1b8c3b52da 100644
--- a/server/service/service_users.go
+++ b/server/service/service_users.go
@@ -92,14 +92,6 @@ func (svc service) ModifyUser(ctx context.Context, userID uint, p kolide.UserPay
 
 	// the method assumes that the correct authorization
 	// has been validated higher up the stack
-	if p.Admin != nil {
-		user.Admin = *p.Admin
-	}
-
-	if p.Enabled != nil {
-		user.Enabled = *p.Enabled
-	}
-
 	if p.Username != nil {
 		user.Username = *p.Username
 	}
diff --git a/server/service/service_users_test.go b/server/service/service_users_test.go
index 43405b09c7..86b0aeee1b 100644
--- a/server/service/service_users_test.go
+++ b/server/service/service_users_test.go
@@ -86,6 +86,39 @@ func TestModifyUserEmail(t *testing.T) {
 
 }
 
+func TestModifyUserCannotUpdateAdminEnabled(t *testing.T) {
+	// The modify user function should not be able to update the admin or
+	// enabled status of a user. These should only be updated explicitly
+	// through the ChangeUserAdmin and ChangeUserEnabled functions.
+	user := &kolide.User{
+		ID:      3,
+		Admin:   false,
+		Email:   "foo@bar.com",
+		Enabled: true,
+	}
+	user.SetPassword("password", 10, 10)
+	ms := new(mock.Store)
+	ms.UserByIDFunc = func(id uint) (*kolide.User, error) {
+		return user, nil
+	}
+	ms.SaveUserFunc = func(u *kolide.User) error {
+		assert.Equal(t, false, u.Admin, "should not be able to update admin status!")
+		assert.Equal(t, true, u.Enabled, "should not be able to update enabled status!")
+		return nil
+	}
+	svc, err := newTestService(ms, nil)
+	ctx := context.Background()
+	ctx = viewer.NewContext(ctx, viewer.Viewer{User: user})
+	payload := kolide.UserPayload{
+		Admin:   boolPtr(true),
+		Enabled: boolPtr(false),
+	}
+	_, err = svc.ModifyUser(ctx, 3, payload)
+	require.Nil(t, err)
+	assert.True(t, ms.SaveUserFuncInvoked)
+
+}
+
 func TestModifyUserEmailNoPassword(t *testing.T) {
 	user := &kolide.User{
 		ID:      3,