[BUGFIX] fix iptables rules to make them idempotent (#1214)

* Update iptables rules to make them idempotent

* Add missing return statements

* Fix catch logic error

* Only catch one statement with try

* Remove broken catches (sync code)

* Move to explictly denying flux 172.23.0.0/16 and use DOCKER-USER chain

* Add a bit more to docstring

* Remove erroneous extra function call from testing

* Greatly simplify how default gateway / local subnet is determined

* Lint

* Remove requirement for , will protect all operator networks

* Modify rules slightly to match iptables output, add tests

* Add allow for Flux networks, remove RETURN that docker keeps adding

* Full refactor - see below

This commit now blocks 100% of access to private address space, while
maintaining isolation for each Flux docker network. Now apps can be
sure no other app is snooping their traffic, and operators can be sure
that apps do not have access to ANY private network they are routing.

Tests will all be broken - I'll fix up in next commit.

* Lint

* Update tests

* Move the docker interface fetch up a level to avoid circular

* Add dockerService to serviceManager, fix up tests

* Fix typing for return

* Stub console output (from Flux log) so it doesn't clog up the testing output

* Add missing remove private stanza for softInstallLocally

* Update compatibility with older iptables 1.8.4 - see below

Older iptables (legacy) on ubuntu 20.04 operates slightly
differently than the nf_tables version, the check output command
doesn't return anything.

Some of the output strings are different, so we don't check those
anymore.

Have also added a check to make sure the iptables binary is in the
root users path.

* Add iptables exists check and fix up tests

ZelBack/src/services/appsService.js

@@ -3391,6 +3391,14 @@ async function registerAppLocally(appSpecs, componentSpecs, res) {
         throw new Error(`Flux App network of ${appName} failed to initiate. Range already assigned to different application.`);
       }
       log.info(serviceHelper.ensureString(fluxNet));
+      const fluxNetworkInterfaces = await dockerService.getFluxDockerNetworkPhysicalInterfaceNames();
+      const accessRemoved = await fluxNetworkHelper.removeDockerContainerAccessToNonRoutable(fluxNetworkInterfaces);
+      const accessRemovedRes = {
+        status: accessRemoved ? `Private network access removed for ${appName}` : `Error removing private network access for ${appName}`,
+      };
+      if (res) {
+        res.write(serviceHelper.ensureString(accessRemovedRes));
+      }
       const fluxNetResponse = {
         status: `Docker network of ${appName} initiated.`,
       };
@@ -3764,6 +3772,14 @@ async function softRegisterAppLocally(appSpecs, componentSpecs, res) {
         throw new Error(`Flux App network of ${appName} failed to initiate. Range already assigned to different application`);
       }
       log.info(serviceHelper.ensureString(fluxNet));
+      const fluxNetworkInterfaces = await dockerService.getFluxDockerNetworkPhysicalInterfaceNames();
+      const accessRemoved = await fluxNetworkHelper.removeDockerContainerAccessToNonRoutable(fluxNetworkInterfaces);
+      const accessRemovedRes = {
+        status: accessRemoved ? `Private network access removed for ${appName}` : `Error removing private network access for ${appName}`,
+      };
+      if (res) {
+        res.write(serviceHelper.ensureString(accessRemovedRes));
+      }
       const fluxNetResponse = {
         status: `Docker network of ${appName} initiated.`,
       };

ZelBack/src/services/dockerService.js

@@ -821,6 +821,46 @@ async function createFluxDockerNetwork() {
   return response;
 }
 
+/**
+ *
+ * @returns {Promise<Docker.NetworkInspectInfo[]>}
+ */
+async function getFluxDockerNetworks() {
+  const fluxNetworks = await docker.listNetworks({
+    filters: JSON.stringify({
+      name: ['fluxDockerNetwork'],
+    }),
+  });
+
+  return fluxNetworks;
+}
+
+/**
+ *
+ * @returns {Promise<string[]>}
+ */
+async function getFluxDockerNetworkPhysicalInterfaceNames() {
+  const fluxNetworks = await getFluxDockerNetworks();
+
+  const interfaceNames = fluxNetworks.map((network) => {
+    // the physical interface name is br-<first 12 chars of Id>
+    const intName = `br-${network.Id.slice(0, 12)}`;
+    return intName;
+  });
+
+  return interfaceNames;
+}
+
+/**
+ *
+ * @returns {Promise<string[]>}
+ */
+async function getFluxDockerNetworkSubnets() {
+  const fluxNetworks = await getFluxDockerNetworks();
+  const subnets = fluxNetworks.map((network) => network.IPAM.Config[0].Subnet);
+  return subnets;
+}
+
 /**
  * Creates flux application docker network if doesn't exist
  *
@@ -973,6 +1013,8 @@ module.exports = {
   createFluxDockerNetwork,
   getDockerContainerOnly,
   getDockerContainerByIdOrName,
+  getFluxDockerNetworkPhysicalInterfaceNames,
+  getFluxDockerNetworkSubnets,
   createFluxAppDockerNetwork,
   removeFluxAppDockerNetwork,
   pruneNetworks,

ZelBack/src/services/fluxNetworkHelper.js

@@ -1345,21 +1345,174 @@ async function purgeUFW() {
 }
 
 /**
- * This fix a docker security issue where docker containers can access host network, for example to create port forwarding on hosts
+ * This fix a docker security issue where docker containers can access private node operator networks, for example to create port forwarding on hosts.
+ *
+ * Docker should create a DOCKER-USER chain. If this doesn't exist - we create it, then jump to this chain immediately from the FORWARD CHAIN.
+ * This allows rules to be added via -I (insert) and -A (append) to the DOCKER-USER chain individually, so we can ALWAYS append the
+ * drop traffic rule, and insert the ACCEPT rules. If no matches are found in the DOCKER-USER chain, rule evaluation continues
+ * from the next rule in the FORWARD chain.
+ *
+ * If needed in the future, we can actually create a JUMP from the DOCKER-USER chain to a custom chain. The reason why we MUST use the DOCKER-USER
+ * chain is that whenever docker creates a new network, it re-jumps the DOCKER-USER chain at the head of the FORWARD chain.
+ *
+ * As can be seen in this example:
+ *
+ * Originally, was using the FLUX chain, but you can see docker inserted the br-72d1725e481c network ahead, as well as the JUMP to DOCKER-USER,
+ * which invalidates any rules in the FLUX chain, as there is basically an accept any:
+ *
+ * FORWARD -i br-72d1725e481c ! -o br-72d1725e481c -j ACCEPT
+ *
+ * ```bash
+ * -A INPUT -j ufw-track-input
+ * -A FORWARD -j DOCKER-USER
+ * -A FORWARD -j DOCKER-ISOLATION-STAGE-1
+ * -A FORWARD -o br-72d1725e481c -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
+ * -A FORWARD -o br-72d1725e481c -j DOCKER
+ * -A FORWARD -i br-72d1725e481c ! -o br-72d1725e481c -j ACCEPT
+ * -A FORWARD -i br-72d1725e481c -o br-72d1725e481c -j ACCEPT
+ * -A FORWARD -j FLUX
+ * -A FORWARD -o br-048fde111132 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
+ * -A FORWARD -o br-048fde111132 -j DOCKER
+ * -A FORWARD -i br-048fde111132 ! -o br-048fde111132 -j ACCEPT
+ * -A FORWARD -i br-048fde111132 -o br-048fde111132 -j ACCEPT
+ *```
+ * This means if a user or someone was to delete a single rule, we are able to recover correctly from it.
+ *
+ * The other option - is just to Flush all rules on every run, and reset them all. This is what we are doing now.
+ *
+ * @param {string[]} fluxNetworkInterfaces The network interfaces, br-<12 character string>
+ * @returns  {Promise<Boolean>}
  */
-async function removeDockerContainerAccessToHost() {
+async function removeDockerContainerAccessToNonRoutable(fluxNetworkInterfaces) {
+  const cmdAsync = util.promisify(nodecmd.get);
+
+  const checkIptables = 'sudo iptables --version';
+  const iptablesInstalled = await cmdAsync(checkIptables).catch(() => {
+    log.error('Unable to find iptables binary');
+    return false
+  });
+
+  if (!iptablesInstalled) return false;
+
+  // check if rules have been created, as iptables is NOT idempotent.
+  const checkDockerUserChain = 'sudo iptables -L DOCKER-USER';
+  // iptables 1.8.4 doesn't return anything - so have updated command a little
+  const checkJumpChain = 'sudo iptables -C FORWARD -j DOCKER-USER && echo true';
+
+  const dockerUserChainExists = await cmdAsync(checkDockerUserChain).catch(async () => {
+    try {
+      await cmdAsync('sudo iptables -N DOCKER-USER');
+      log.info('IPTABLES: DOCKER-USER chain created');
+    } catch (err) {
+      log.error('IPTABLES: Error adding DOCKER-USER chain');
+      // if we can't add chain, we can't proceed
+      return new Error();
+    }
+    return null;
+  });
+
+  if (dockerUserChainExists instanceof Error) return false;
+  if (dockerUserChainExists) log.info('IPTABLES: DOCKER-USER chain already created');
+
+  const checkJumpToDockerChain = await cmdAsync(checkJumpChain).catch(async () => {
+    // Ubuntu 20.04 @ iptables 1.8.4 Error: "iptables: No chain/target/match by that name."
+    // Ubuntu 22.04 @ iptables 1.8.7 Error: "iptables: Bad rule (does a matching rule exist in that chain?)."
+    const jumpToFluxChain = 'sudo iptables -I FORWARD -j DOCKER-USER';
+    try {
+      await cmdAsync(jumpToFluxChain);
+      log.info('IPTABLES: New rule in FORWARD inserted to jump to DOCKER-USER chain');
+    } catch (err) {
+      log.error('IPTABLES: Error inserting FORWARD jump to DOCKER-USER chain');
+      // if we can't jump, we need to bail out
+      return new Error();
+    }
+
+    return null;
+  });
+
+  if (checkJumpToDockerChain instanceof Error) return false;
+  if (checkJumpToDockerChain) log.info('IPTABLES: Jump to DOCKER-USER chain already enabled');
+
+  const rfc1918Networks = ['10.0.0.0/8', '172.16.0.0/12', '192.168.0.0/16'];
+  const fluxSrc = '172.23.0.0/16';
+
+  const baseDropCmd = `sudo iptables -A DOCKER-USER -s ${fluxSrc} -d #DST -j DROP`;
+  const baseAllowToFluxNetworksCmd = 'sudo iptables -I DOCKER-USER -i #INT -o #INT -j ACCEPT';
+  const baseAllowEstablishedCmd = `sudo iptables -I DOCKER-USER -s ${fluxSrc} -d #DST -m state --state RELATED,ESTABLISHED -j ACCEPT`;
+  const baseAllowDnsCmd = `sudo iptables -I DOCKER-USER -s ${fluxSrc} -d #DST -p udp --dport 53 -j ACCEPT`;
+
+  const addReturnCmd = 'sudo iptables -A DOCKER-USER -j RETURN';
+  const flushDockerUserCmd = 'sudo iptables -F DOCKER-USER';
+
   try {
-    const cmdAsync = util.promisify(nodecmd.get);
-    const dropAccessToHostNetwork = "sudo iptables -I FORWARD -i docker0 -d $(ip route | grep \"src $(ip addr show dev $(ip route | awk '/default/ {print $5}') | grep \"inet\" | awk 'NR==1{print $2}' | cut -d'/' -f 1)\" | awk '{print $1}') -j DROP";
-    await cmdAsync(dropAccessToHostNetwork).catch((error) => log.error(`Error executing dropAccessToHostNetwork command:${error}`));
-    const giveHostAccessToDockerNetwork = "sudo iptables -I FORWARD -i docker0 -d $(ip route | grep \"src $(ip addr show dev $(ip route | awk '/default/ {print $5}') | grep \"inet\" | awk 'NR==1{print $2}' | cut -d'/' -f 1)\" | awk '{print $1}') -m state --state ESTABLISHED,RELATED -j ACCEPT";
-    await cmdAsync(giveHostAccessToDockerNetwork).catch((error) => log.error(`Error executing giveHostAccessToDockerNetwork command:${error}`));
-    const giveContainerAccessToDNS = "sudo iptables -I FORWARD -i docker0 -p udp -d $(ip route | grep \"src $(ip addr show dev $(ip route | awk '/default/ {print $5}') | grep \"inet\" | awk 'NR==1{print $2}' | cut -d'/' -f 1)\" | awk '{print $1}') --dport 53 -j ACCEPT";
-    await cmdAsync(giveContainerAccessToDNS).catch((error) => log.error(`Error executing giveContainerAccessToDNS command:${error}`));
-    log.info('Access to host from containers removed');
-  } catch (error) {
-    log.error(error);
+    await cmdAsync(flushDockerUserCmd);
+    log.info('IPTABLES: DOCKER-USER table flushed');
+  } catch (err) {
+    log.error(`IPTABLES: Error flushing DOCKER-USER table. ${err}`);
+    return false;
+  }
+
+  // add for legacy apps
+  fluxNetworkInterfaces.push('docker0');
+
+  // eslint-disable-next-line no-restricted-syntax
+  for (const int of fluxNetworkInterfaces) {
+    // if this errors, we need to bail, as if the deny succeedes, we may cut off access
+    const giveFluxNetworkAccess = baseAllowToFluxNetworksCmd.replace(/#INT/g, int);
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      await cmdAsync(giveFluxNetworkAccess);
+      log.info(`IPTABLES: Traffic on Flux interface ${int} accepted`);
+    } catch (err) {
+      log.error(`IPTABLES: Error allowing traffic on Flux interface ${int}. ${err}`);
+      return false;
+    }
+  }
+
+  // eslint-disable-next-line no-restricted-syntax
+  for (const network of rfc1918Networks) {
+    // if any of these error, we need to bail, as if the deny succeedes, we may cut off access
+
+    const giveHostAccessToDockerNetwork = baseAllowEstablishedCmd.replace('#DST', network);
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      await cmdAsync(giveHostAccessToDockerNetwork);
+      log.info(`IPTABLES: Access to Flux containers from ${network} accepted`);
+    } catch (err) {
+      log.error(`IPTABLES: Error allowing access to Flux containers from ${network}. ${err}`);
+      return false;
+    }
+
+    const giveContainerAccessToDNS = baseAllowDnsCmd.replace('#DST', network);
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      await cmdAsync(giveContainerAccessToDNS);
+      log.info(`IPTABLES: DNS access to ${network} from Flux containers accepted`);
+    } catch (err) {
+      log.error(`IPTABLES: Error allowing DNS access to ${network} from Flux containers. ${err}`);
+      return false;
+    }
+
+    // This always gets appended, so the drop is at the end
+    const dropAccessToHostNetwork = baseDropCmd.replace('#DST', network);
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      await cmdAsync(dropAccessToHostNetwork);
+      log.info(`IPTABLES: Access to ${network} from Flux containers removed`);
+    } catch (err) {
+      log.error(`IPTABLES: Error denying access to ${network} from Flux containers. ${err}`);
+      return false;
+    }
   }
+
+  try {
+    await cmdAsync(addReturnCmd);
+    log.info('IPTABLES: DOCKER-USER explicit return to FORWARD chain added');
+  } catch (err) {
+    log.error(`IPTABLES: Error adding explicit return to Forward chain. ${err}`);
+    return false;
+  }
+  return true;
 }
 
 const lruRateOptions = {
@@ -1492,5 +1645,5 @@ module.exports = {
   isPortUserBlocked,
   allowNodeToBindPrivilegedPorts,
   installNetcat,
-  removeDockerContainerAccessToHost,
+  removeDockerContainerAccessToNonRoutable,
 };

ZelBack/src/services/serviceHelper.js

@@ -200,6 +200,42 @@ function commandStringToArray(command) {
   return splitargs(command);
 }
 
+/**
+ *
+ * @param {*} ip ip address to check
+ * @returns {Boolean}
+ */
+function validIpv4Address(ip) {
+  // first octet must start with 1-9, then next 3 can be 0.
+  const ipv4Regex = /^[1-9]\d{0,2}\.(\d{0,3}\.){2}\d{0,3}$/;
+
+  if (!ipv4Regex.test(ip)) return false;
+
+  const octets = ip.split('.');
+  const isValid = octets.every((octet) => parseInt(octet, 10) < 256);
+  return isValid;
+}
+
+/**
+ * To confirm if ip is in subnet
+ * @param {string} ip
+ * @param {string} subnet
+ * @returns {Boolean}
+ */
+function ipInSubnet(ip, subnet) {
+  const [network, mask] = subnet.split('/');
+
+  if (!validIpv4Address(ip) || !validIpv4Address(network)) return false;
+
+  // eslint-disable-next-line no-bitwise
+  const ipAsInt = Number(ip.split('.').reduce((ipInt, octet) => (ipInt << 8) + parseInt(octet || 0, 10), 0));
+  // eslint-disable-next-line no-bitwise
+  const networkAsInt = Number(network.split('.').reduce((ipInt, octet) => (ipInt << 8) + parseInt(octet || 0, 10), 0));
+  const maskAsInt = parseInt('1'.repeat(mask) + '0'.repeat(32 - mask), 2);
+  // eslint-disable-next-line no-bitwise
+  return (ipAsInt & maskAsInt) === (networkAsInt & maskAsInt);
+}
+
 module.exports = {
   ensureBoolean,
   ensureNumber,
@@ -212,4 +248,6 @@ module.exports = {
   isDecimalLimit,
   dockerBufferToString,
   commandStringToArray,
+  validIpv4Address,
+  ipInSubnet,
 };

ZelBack/src/services/serviceManager.js

@@ -14,6 +14,7 @@ const geolocationService = require('./geolocationService');
 const upnpService = require('./upnpService');
 const syncthingService = require('./syncthingService');
 const pgpService = require('./pgpService');
+const dockerService = require('./dockerService');
 
 const apiPort = userconfig.initial.apiport || config.server.apiport;
 const development = userconfig.initial.development || false;
@@ -91,10 +92,11 @@ async function startFluxFunctions() {
     setTimeout(() => {
       fluxCommunicationUtils.constantlyUpdateDeterministicFluxList(); // updates deterministic flux list for communication every 2 minutes, so we always trigger cache and have up to date value
     }, 15 * 1000);
-    setTimeout(() => {
+    setTimeout(async () => {
       log.info('Rechecking firewall app rules');
       fluxNetworkHelper.purgeUFW();
-      fluxNetworkHelper.removeDockerContainerAccessToHost();
+      const fluxNetworkInterfaces = await dockerService.getFluxDockerNetworkPhysicalInterfaceNames();
+      fluxNetworkHelper.removeDockerContainerAccessToNonRoutable(fluxNetworkInterfaces);
       appsService.testAppMount(); // test if our node can mount a volume
     }, 30 * 1000);
     setTimeout(() => {