CVE-2024-3283: Improperly Controlled Modification of Dynamically-Determined Object Attributes (CWE-915)
value: A vulnerability in mintplex-labs/anything-llm allows users with manager roles to escalate their privileges to admin roles through a mass assignment issue. The '/admin/system-preferences' API endpoint improperly authorizes manager-level users to modify the 'multi_user_mode' system variable, enabling them to access the '/api/system/enable-multi-user' endpoint and create a new admin user. This issue results from the endpoint accepting a full JSON object in the request body without proper validation of modifiable fields, leading to unauthorized modification of system settings and subsequent privilege escalation.
CWE description: The product receives input from an upstream component that specifies multiple attributes, properties, or fields that are to be initialized or updated in an object, but it does not properly control which attributes can be modified.
Published: 2024-04-10T17:15Z
Repository: https://github.com/mintplex-labs/anything-llm
Language: JavaScript
Files changed: server/endpoints/system.js, server/models/systemSettings.js, server/models/telemetry.js
Commit date: 2024-03-29 10:56:32 -0700
Commit message: Patch ability to update multi-user-flag once set (#993)

* Patch ability to update multi-user-flag once set

* update logo function to safe update key values
Diff: diff --git a/server/endpoints/system.js b/server/endpoints/system.js
index 3c8cf6bc9..901276810 100644
--- a/server/endpoints/system.js
+++ b/server/endpoints/system.js
@@ -398,11 +398,11 @@ function systemEndpoints(app) {
         const { user, error } = await User.create({
           username,
           password,
           role: ROLES.admin,
         });
-        await SystemSettings.updateSettings({
+        await SystemSettings._updateSettings({
           multi_user_mode: true,
           users_can_delete_workspaces: false,
           limit_user_messages: false,
           message_limit: 25,
         });
@@ -420,11 +420,11 @@ function systemEndpoints(app) {
         });
         await EventLogs.logEvent("multi_user_mode_enabled", {}, user?.id);
         response.status(200).json({ success: !!user, error });
       } catch (e) {
         await User.delete({});
-        await SystemSettings.updateSettings({
+        await SystemSettings._updateSettings({
           multi_user_mode: false,
         });
 
         console.log(e.message, e);
         response.sendStatus(500).end();
@@ -621,11 +621,11 @@ function systemEndpoints(app) {
       try {
         const newFilename = await renameLogoFile(request.file.originalname);
         const existingLogoFilename = await SystemSettings.currentLogoFilename();
         await removeCustomLogo(existingLogoFilename);
 
-        const { success, error } = await SystemSettings.updateSettings({
+        const { success, error } = await SystemSettings._updateSettings({
           logo_filename: newFilename,
         });
 
         return response.status(success ? 200 : 500).json({
           message: success
@@ -655,11 +655,11 @@ function systemEndpoints(app) {
     [validatedRequest, flexUserRoleValid([ROLES.admin, ROLES.manager])],
     async (_request, response) => {
       try {
         const currentLogoFilename = await SystemSettings.currentLogoFilename();
         await removeCustomLogo(currentLogoFilename);
-        const { success, error } = await SystemSettings.updateSettings({
+        const { success, error } = await SystemSettings._updateSettings({
           logo_filename: LOGO_FILENAME,
         });
 
         return response.status(success ? 200 : 500).json({
           message: success
diff --git a/server/models/systemSettings.js b/server/models/systemSettings.js
index f27434ca9..e4c0f9499 100644
--- a/server/models/systemSettings.js
+++ b/server/models/systemSettings.js
@@ -3,12 +3,12 @@ process.env.NODE_ENV === "development"
   : require("dotenv").config();
 
 const prisma = require("../utils/prisma");
 
 const SystemSettings = {
+  protectedFields: ["multi_user_mode"],
   supportedFields: [
-    "multi_user_mode",
     "users_can_delete_workspaces",
     "limit_user_messages",
     "message_limit",
     "logo_filename",
     "telemetry_id",
@@ -285,30 +285,47 @@ const SystemSettings = {
       console.error(error.message);
       return [];
     }
   },
 
+  // Can take generic keys and will pre-filter invalid keys
+  // from the set before sending to the explicit update function
+  // that will then enforce validations as well.
   updateSettings: async function (updates = {}) {
+    const validFields = Object.keys(updates).filter((key) =>
+      this.supportedFields.includes(key)
+    );
+
+    Object.entries(updates).forEach(([key]) => {
+      if (validFields.includes(key)) return;
+      delete updates[key];
+    });
+
+    return this._updateSettings(updates);
+  },
+
+  // Explicit update of settings + key validations.
+  // Only use this method when directly setting a key value
+  // that takes no user input for the keys being modified.
+  _updateSettings: async function (updates = {}) {
     try {
-      const updatePromises = Object.keys(updates)
-        .filter((key) => this.supportedFields.includes(key))
-        .map((key) => {
-          const validatedValue = this.validations.hasOwnProperty(key)
-            ? this.validations[key](updates[key])
-            : updates[key];
-
-          return prisma.system_settings.upsert({
-            where: { label: key },
-            update: {
-              value: validatedValue === null ? null : String(validatedValue),
-            },
-            create: {
-              label: key,
-              value: validatedValue === null ? null : String(validatedValue),
-            },
-          });
+      const updatePromises = Object.keys(updates).map((key) => {
+        const validatedValue = this.validations.hasOwnProperty(key)
+          ? this.validations[key](updates[key])
+          : updates[key];
+
+        return prisma.system_settings.upsert({
+          where: { label: key },
+          update: {
+            value: validatedValue === null ? null : String(validatedValue),
+          },
+          create: {
+            label: key,
+            value: validatedValue === null ? null : String(validatedValue),
+          },
         });
+      });
 
       await Promise.all(updatePromises);
       return { success: true, error: null };
     } catch (error) {
       console.error("FAILED TO UPDATE SYSTEM SETTINGS", error.message);
diff --git a/server/models/telemetry.js b/server/models/telemetry.js
index 7c27aa69c..ac82d56f4 100644
--- a/server/models/telemetry.js
+++ b/server/models/telemetry.js
@@ -65,11 +65,11 @@ const Telemetry = {
     await client.shutdownAsync();
   },
 
   setUid: async function () {
     const newId = v4();
-    await SystemSettings.updateSettings({ [this.label]: newId });
+    await SystemSettings._updateSettings({ [this.label]: newId });
     return newId;
   },
 
   findOrCreateId: async function () {
     let currentId = await this.id();

Vulnerable code:         await SystemSettings.updateSettings({
        await SystemSettings.updateSettings({
        const { success, error } = await SystemSettings.updateSettings({
        const { success, error } = await SystemSettings.updateSettings({
    "multi_user_mode",
      const updatePromises = Object.keys(updates)
        .filter((key) => this.supportedFields.includes(key))
        .map((key) => {
          const validatedValue = this.validations.hasOwnProperty(key)
            ? this.validations[key](updates[key])
            : updates[key];

          return prisma.system_settings.upsert({
            where: { label: key },
            update: {
              value: validatedValue === null ? null : String(validatedValue),
            },
            create: {
              label: key,
              value: validatedValue === null ? null : String(validatedValue),
            },
          });
    await SystemSettings.updateSettings({ [this.label]: newId });
Fixed code:         await SystemSettings._updateSettings({
        await SystemSettings._updateSettings({
        const { success, error } = await SystemSettings._updateSettings({
        const { success, error } = await SystemSettings._updateSettings({
  protectedFields: ["multi_user_mode"],
  // Can take generic keys and will pre-filter invalid keys
  // from the set before sending to the explicit update function
  // that will then enforce validations as well.
    const validFields = Object.keys(updates).filter((key) =>
      this.supportedFields.includes(key)
    );

    Object.entries(updates).forEach(([key]) => {
      if (validFields.includes(key)) return;
      delete updates[key];
    });

    return this._updateSettings(updates);
  },

  // Explicit update of settings + key validations.
  // Only use this method when directly setting a key value
  // that takes no user input for the keys being modified.
  _updateSettings: async function (updates = {}) {
      const updatePromises = Object.keys(updates).map((key) => {
        const validatedValue = this.validations.hasOwnProperty(key)
          ? this.validations[key](updates[key])
          : updates[key];

        return prisma.system_settings.upsert({
          where: { label: key },
          update: {
            value: validatedValue === null ? null : String(validatedValue),
          },
          create: {
            label: key,
            value: validatedValue === null ? null : String(validatedValue),
          },
      });
    await SystemSettings._updateSettings({ [this.label]: newId });