Fix OS command injection in POST /create (package install feature finding)

routes/index.js

@@ -8,7 +8,7 @@ var ms = require('ms');
 var streamBuffers = require('stream-buffers');
 var readline = require('readline');
 var moment = require('moment');
-var exec = require('child_process').exec;
+var execFile = require('child_process').execFile;
 var validator = require('validator');
 
 // zip-slip
@@ -149,21 +149,35 @@ function parse(todo) {
   return t;
 }
 
+// Characters outside this set (whitespace and every shell metacharacter) are never
+// accepted in a URL handed to the image tooling.
+var SAFE_URL_CHARS = /^[A-Za-z0-9._~:/?#@%+=-]+$/;
+var MAX_URL_LENGTH = 2048;
+
+function isInspectableImageUrl(url) {
+  return typeof url === 'string' && url.length <= MAX_URL_LENGTH &&
+    SAFE_URL_CHARS.test(url) &&
+    validator.isURL(url, { protocols: ['http', 'https'], require_protocol: true });
+}
+
 exports.create = function (req, res, next) {
   // console.log('req.body: ' + JSON.stringify(req.body));
 
   var item = req.body.content;
-  var imgRegex = /\!\[alt text\]\((http.*)\s\".*/;
+  var imgRegex = /\!\[alt text\]\((http[^)\s]*)\s\".*/;
   if (typeof (item) == 'string' && item.match(imgRegex)) {
     var url = item.match(imgRegex)[1];
     console.log('found img: ' + url);
 
-    exec('identify ' + url, function (err, stdout, stderr) {
-      console.log(err);
-      if (err !== null) {
-        console.log('Error (' + err + '):' + stderr);
-      }
-    });
+    if (isInspectableImageUrl(url)) {
+      execFile('identify', [url], function (err, stdout, stderr) {
+        if (err !== null) {
+          console.log('Error (' + err + '):' + stderr);
+        }
+      });
+    } else {
+      console.log('skipping identify for rejected img url');
+    }
 
   } else {
     item = parse(item);

tests/create-command-injection.spec.js

@@ -0,0 +1,61 @@
+var test = require('tap').test;
+var path = require('path');
+var mongoose = require('mongoose');
+var childProcess = require('child_process');
+
+var spawned = [];
+childProcess.exec = function () {
+  spawned.push({ api: 'exec', args: Array.prototype.slice.call(arguments, 0, 1) });
+};
+childProcess.execFile = function (file, args, cb) {
+  spawned.push({ api: 'execFile', file: file, args: args });
+  if (cb) cb(null, '', '');
+};
+
+if (!mongoose.models.Todo) {
+  mongoose.model('Todo', new mongoose.Schema({ content: String, updated_at: Date }));
+}
+if (!mongoose.models.User) {
+  mongoose.model('User', new mongoose.Schema({ username: String, password: String }));
+}
+mongoose.Model.prototype.save = function (cb) {
+  cb(null, { content: Buffer.from(String(this.content || '')) }, 1);
+};
+
+var routes = require(path.join(__dirname, '..', 'routes', 'index.js'));
+
+function submit(content) {
+  spawned = [];
+  var res = { setHeader: function () {}, status: function () { return this; }, send: function () {} };
+  routes.create({ body: { content: content } }, res, function (err) { throw err; });
+  return spawned;
+}
+
+var injections = [
+  '![alt text](http://x/;id ")',
+  '![alt text](http://x/`id` ")',
+  '![alt text](http://x/$(id) ")',
+  '![alt text](http://x/|id ")',
+  '![alt text](http://x/ && touch /tmp/pwn ")',
+  '![alt text](http://x/\nid ")',
+  '![alt text](file:///etc/passwd ")',
+];
+
+test('POST /create never invokes a shell for todo image urls', function (t) {
+  injections.forEach(function (content) {
+    var calls = submit(content);
+    t.equal(calls.filter(function (c) { return c.api === 'exec'; }).length, 0,
+      'no shell for ' + JSON.stringify(content));
+    t.equal(calls.length, 0, 'no child process at all for ' + JSON.stringify(content));
+  });
+  t.end();
+});
+
+test('POST /create still inspects a well-formed image url without a shell', function (t) {
+  var calls = submit('![alt text](http://example.com/snyk.png "logo")');
+  t.equal(calls.length, 1, 'one child process');
+  t.equal(calls[0].api, 'execFile', 'uses execFile');
+  t.equal(calls[0].file, 'identify', 'runs identify');
+  t.same(calls[0].args, ['http://example.com/snyk.png'], 'url passed as a single argument');
+  t.end();
+});