bug: fix lodash prototype pollution in PUT /chat (CWE-1321)

package.json

@@ -31,7 +31,7 @@
     "hbs": "^4.0.4",
     "humanize-ms": "1.0.1",
     "jquery": "^2.2.4",
-    "lodash": "4.17.4",
+    "lodash": "4.17.21",
     "marked": "0.3.5",
     "method-override": "latest",
     "moment": "2.15.1",

routes/index.js

@@ -16,8 +16,6 @@ var fileType = require('file-type');
 var AdmZip = require('adm-zip');
 var fs = require('fs');
 
-// prototype-pollution
-var _ = require('lodash');
 
 exports.index = function (req, res, next) {
   Todo.
@@ -321,10 +319,34 @@ let messages = [];
 let lastId = 1;
 
 function findUser(auth) {
+  if (typeof auth.name !== 'string' || typeof auth.password !== 'string') {
+    return undefined;
+  }
+
   return users.find((u) =>
     u.name === auth.name &&
     u.password === auth.password);
 }
+
+// Only these message fields may be supplied by the client, and only as strings.
+const MESSAGE_FIELDS = ['text', 'icon'];
+
+function pickMessageFields(input) {
+  const picked = {};
+
+  if (input === null || typeof input !== 'object') {
+    return picked;
+  }
+
+  MESSAGE_FIELDS.forEach((field) => {
+    if (Object.prototype.hasOwnProperty.call(input, field) &&
+      typeof input[field] === 'string') {
+      picked[field] = input[field];
+    }
+  });
+
+  return picked;
+}
 ///////////////////////////////////////////////////////////////////////////////
 
 exports.chat = {
@@ -339,24 +361,26 @@ exports.chat = {
       return;
     }
 
-    const message = {
-      // Default message icon. Cen be overwritten by user.
-      icon: '👋',
-    };
-
-    _.merge(message, req.body.message, {
-      id: lastId++,
-      timestamp: Date.now(),
-      userName: user.name,
-    });
+    const message = Object.assign(
+      {
+        // Default message icon. Cen be overwritten by user.
+        icon: '👋',
+      },
+      pickMessageFields(req.body.message),
+      {
+        id: lastId++,
+        timestamp: Date.now(),
+        userName: user.name,
+      });
 
     messages.push(message);
     res.send({ ok: true });
   },
   delete(req, res) {
     const user = findUser(req.body.auth || {});
 
-    if (!user || !user.canDelete) {
+    if (!user || !Object.prototype.hasOwnProperty.call(user, 'canDelete') ||
+      user.canDelete !== true) {
       res.status(403).send({ ok: false, error: 'Access denied' });
       return;
     }

tests/chat.prototype-pollution.test.js

@@ -0,0 +1,95 @@
+const test = require('tap').test;
+const mongoose = require('mongoose');
+
+// routes/index.js resolves the Todo/User models at require time.
+mongoose.model('Todo', new mongoose.Schema({ content: String, updated_at: Date }));
+mongoose.model('User', new mongoose.Schema({ username: String, password: String }));
+
+const routes = require('../routes');
+
+const AUTH = { name: 'user', password: 'pwd' };
+
+function fakeRes() {
+  return {
+    statusCode: 200,
+    body: undefined,
+    status(code) {
+      this.statusCode = code;
+      return this;
+    },
+    send(body) {
+      this.body = body;
+      return this;
+    },
+  };
+}
+
+test('chat.add ignores __proto__ in the message payload', (t) => {
+  const res = fakeRes();
+  routes.chat.add({
+    body: {
+      auth: AUTH,
+      // JSON.parse keeps __proto__ as an own key, exactly like a request body.
+      message: JSON.parse('{"text":"hi","__proto__":{"polluted":true,"canDelete":true}}'),
+    },
+  }, res);
+
+  t.same(res.body, { ok: true });
+  t.equal({}.polluted, undefined, 'Object.prototype is not polluted');
+  t.equal({}.canDelete, undefined, 'Object.prototype.canDelete is not set');
+  t.end();
+});
+
+test('chat.add ignores constructor.prototype in the message payload', (t) => {
+  const res = fakeRes();
+  routes.chat.add({
+    body: {
+      auth: AUTH,
+      message: { constructor: { prototype: { pollutedToo: true } } },
+    },
+  }, res);
+
+  t.same(res.body, { ok: true });
+  t.equal({}.pollutedToo, undefined, 'Object.prototype is not polluted');
+  t.end();
+});
+
+test('chat.add only copies allow-listed string fields', (t) => {
+  const res = fakeRes();
+  routes.chat.add({
+    body: {
+      auth: AUTH,
+      message: { text: 'hello', icon: 'x', id: 999, userName: 'admin', evil: 'nope' },
+    },
+  }, res);
+
+  const getRes = fakeRes();
+  routes.chat.get({}, getRes);
+  const message = getRes.body[getRes.body.length - 1];
+
+  t.equal(message.text, 'hello');
+  t.equal(message.icon, 'x');
+  t.equal(message.userName, 'user', 'userName is assigned server side');
+  t.not(message.id, 999, 'id is assigned server side');
+  t.equal(message.evil, undefined, 'unknown fields are dropped');
+  t.end();
+});
+
+test('chat.delete stays denied for the low privileged user', (t) => {
+  const res = fakeRes();
+  routes.chat.delete({ body: { auth: AUTH, messageId: 1 } }, res);
+
+  t.equal(res.statusCode, 403);
+  t.same(res.body, { ok: false, error: 'Access denied' });
+  t.end();
+});
+
+test('findUser rejects non string credentials', (t) => {
+  const res = fakeRes();
+  routes.chat.add({
+    body: { auth: { name: { $ne: null }, password: { $ne: null } }, message: {} },
+  }, res);
+
+  t.equal(res.statusCode, 403);
+  t.end();
+});