Fix command-injection in network monitor (use execFile) and add tests

sensor/NetworkMonitorSensor.js

@@ -18,7 +18,7 @@ const log = require('../net2/logger.js')(__filename);
 
 const Sensor = require('./Sensor.js').Sensor;
 
-const exec = require('child-process-promise').exec;
+const networkMonitorCommand = require('../util/NetworkMonitorCommand.js');
 const f = require('../net2/Firewalla.js');
 const fc = require('../net2/config.js');
 const extensionManager = require('./ExtensionManager.js')
@@ -334,11 +334,10 @@ class NetworkMonitorSensor extends Sensor {
 
         rtid = intf.rtid;
       }
-      const result = await exec(`sudo ping -i ${cfg.sampleTick} ${rtid ? `-m ${rtid}` : ""} -c ${cfg.sampleCount} -W 1 -4 -n ${target}| awk '/time=/ && !/DUP!/ {print $7}' | cut -d= -f2`).catch((err) => {
+      const data = await networkMonitorCommand.ping(target, cfg.sampleTick, cfg.sampleCount, rtid).catch((err) => {
         log.error(`ping failed on ${target}:`,err.message);
-        return null;
+        return [];
       } );
-      const data = (result && result.stdout) ?  result.stdout.trim().split(/\n/).map(e => parseFloat(e)) : [];
       return { "status": "OK", "data": await this.recordSampleDataInRedis(MONITOR_PING, target, timeSlot, data, cfg, opts)};
     } catch (err) {
       log.error("failed to sample PING:",err.message);
@@ -372,13 +371,11 @@ class NetworkMonitorSensor extends Sensor {
         }
       }
       for (let i=0;i<cfg.sampleCount;i++) {
-        const result = await exec(`dig @${target} ${bindIP ? `-b ${bindIP}` : ""} +tries=1 +timeout=${cfg.sampleTick} ${cfg.lookupName} | awk '/Query time:/ {print $4}'`).catch((err) => {
+        const queryTime = await networkMonitorCommand.dns(target, bindIP, cfg.sampleTick, cfg.lookupName).catch((err) => {
           log.error(`dig failed on ${target}:`,err.message);
           return null;
         } );
-        if (result && result.stdout) {
-          data.push(parseInt(result.stdout.trim()));
-        }
+        if (queryTime !== null) data.push(queryTime);
       }
       return { "status": "OK", "data": await this.recordSampleDataInRedis(MONITOR_DNS, target, timeSlot, data, cfg, opts)};
     } catch (err) {
@@ -454,13 +451,11 @@ class NetworkMonitorSensor extends Sensor {
       }
       for (let i=0;i<cfg.sampleCount;i++) {
         try {
-          const result = await exec(`curl -sk -m 10 ${bindIP ? `--interface ${bindIP}` : ""} -w '%{time_total}\n' '${target}' | tail -1`).catch((err) => {
+          const queryTime = await networkMonitorCommand.http(target, bindIP).catch((err) => {
             log.error(`curl failed on ${target}:`,err.message);
             return null;
           } );
-          if (result && result.stdout) {
-            data.push(parseFloat(result.stdout.trim()));
-          }
+          if (queryTime !== null && Number.isFinite(queryTime)) data.push(queryTime);
         } catch (err2) {
           log.error("curl command failed:",err2);
         }

test/test_network_monitor_security.js

@@ -0,0 +1,60 @@
+/*    Copyright 2016-2026 Firewalla Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the GNU Affero General Public License, version 3.
+ */
+
+'use strict';
+
+const { expect } = require('chai');
+const proxyquire = require('proxyquire').noPreserveCache();
+
+describe('NetworkMonitorSensor command execution', function() {
+  let calls;
+  let command;
+
+  beforeEach(function() {
+    calls = [];
+    command = proxyquire('../util/NetworkMonitorCommand.js', {
+      'child-process-promise': {
+        execFile: async (file, args) => {
+          calls.push({ file, args });
+          if (file === 'sudo') return { stdout: '64 bytes from host: time=1.25 ms\n' };
+          if (file === 'dig') return { stdout: ';; Query time: 7 msec\n' };
+          return { stdout: '0.125000\n' };
+        }
+      }
+    });
+  });
+
+  it('passes ping targets as literal arguments instead of shell commands', async function() {
+    const target = '1.1.1.1; touch /tmp/network-monitor-injected';
+    const result = await command.ping(target, 1, 1, 0);
+
+    expect(calls).to.deep.equal([{
+      file: 'sudo',
+      args: ['ping', '-i', '1', '-c', '1', '-W', '1', '-4', '-n', target]
+    }]);
+    expect(result).to.deep.equal([1.25]);
+  });
+
+  it('passes DNS names and HTTP URLs as literal arguments', async function() {
+    const dnsTarget = '8.8.8.8; touch /tmp/network-monitor-injected';
+    const lookupName = 'example.com; touch /tmp/network-monitor-injected';
+    const httpTarget = "https://example.com/'; touch /tmp/network-monitor-injected; #";
+
+    const dnsResult = await command.dns(dnsTarget, null, 1, lookupName);
+    const httpResult = await command.http(httpTarget, null);
+
+    expect(calls[0]).to.deep.equal({
+      file: 'dig',
+      args: [`@${dnsTarget}`, '+tries=1', '+timeout=1', lookupName]
+    });
+    expect(calls[1]).to.deep.equal({
+      file: 'curl',
+      args: ['-s', '-k', '-m', '10', '-o', '/dev/null', '-w', '%{time_total}\n', '--', httpTarget]
+    });
+    expect(dnsResult).to.equal(7);
+    expect(httpResult).to.equal(0.125);
+  });
+});

util/NetworkMonitorCommand.js

@@ -0,0 +1,39 @@
+/*    Copyright 2016-2026 Firewalla Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the GNU Affero General Public License, version 3.
+ */
+
+'use strict';
+
+const { execFile } = require('child-process-promise');
+
+async function ping(target, sampleTick, sampleCount, rtid) {
+  const args = ['ping', '-i', String(sampleTick)];
+  if (rtid) args.push('-m', String(rtid));
+  args.push('-c', String(sampleCount), '-W', '1', '-4', '-n', String(target));
+  const result = await execFile('sudo', args);
+  return result.stdout.split(/\n/)
+    .filter(line => /time=/.test(line) && !/DUP!/.test(line))
+    .map(line => Number((line.match(/time[=<]([0-9.]+)/) || [])[1]))
+    .filter(Number.isFinite);
+}
+
+async function dns(target, bindIP, sampleTick, lookupName) {
+  const args = [`@${target}`];
+  if (bindIP) args.push('-b', String(bindIP));
+  args.push('+tries=1', `+timeout=${sampleTick}`, String(lookupName));
+  const result = await execFile('dig', args);
+  const match = result.stdout.match(/Query time:\s+(\d+)\s+msec/);
+  return match ? Number(match[1]) : null;
+}
+
+async function http(target, bindIP) {
+  const args = ['-s', '-k', '-m', '10'];
+  if (bindIP) args.push('--interface', String(bindIP));
+  args.push('-o', '/dev/null', '-w', '%{time_total}\n', '--', String(target));
+  const result = await execFile('curl', args);
+  return Number(result.stdout.trim());
+}
+
+module.exports = { ping, dns, http };