From 5aecd94e243ee55cffbb9aa47e7adfc437401de3 Mon Sep 17 00:00:00 2001 From: Rich Bayliss Date: Wed, 12 Aug 2020 12:16:55 +0100 Subject: [PATCH] bug: Firewall not blocking supervisor access from outside the device Change-type: patch Signed-off-by: Rich Bayliss --- src/lib/firewall.ts | 23 +++-- test/29-firewall.spec.ts | 184 +++++++++++++++++++++++++++++------- test/lib/mocked-iptables.ts | 51 +++++++--- 3 files changed, 202 insertions(+), 56 deletions(-) diff --git a/src/lib/firewall.ts b/src/lib/firewall.ts index 3e9b937e..550d6c0b 100644 --- a/src/lib/firewall.ts +++ b/src/lib/firewall.ts @@ -77,11 +77,6 @@ const standardPolicy: iptables.Rule[] = [ matches: ['-m state', '--state ESTABLISHED,RELATED'], target: 'ACCEPT', }, - { - comment: 'Reject everything else', - action: iptables.RuleAction.Append, - target: 'REJECT', - }, ]; let supervisorAccessRules: iptables.Rule[] = []; @@ -105,6 +100,15 @@ function updateSupervisorAccessRules( target: 'ACCEPT', }), ); + + // now block access to the port for any interface, since the above should have allowed legitimate traffic... + supervisorAccessRules.push({ + comment: 'Supervisor API', + action: iptables.RuleAction.Append, + proto: 'tcp', + matches: [`--dport ${port}`], + target: 'REJECT', + }); } async function runningHostBoundServices(): Promise { @@ -162,7 +166,7 @@ export async function applyFirewallMode(mode: string) { mode === 'off' || (mode === 'auto' && !isServicesInHostNetworkMode) ? { comment: `Firewall disabled (${mode})`, - action: iptables.RuleAction.Insert, + action: iptables.RuleAction.Append, target: 'RETURN', } : []; @@ -181,7 +185,12 @@ export async function applyFirewallMode(mode: string) { .addRule(supervisorAccessRules) .addRule(standardServices) .addRule(standardPolicy) - .addRule(returnIfOff), + .addRule(returnIfOff) + .addRule({ + comment: 'Reject everything else', + action: iptables.RuleAction.Append, + target: 'REJECT', + }), ) .forChain('INPUT', (chain) => chain diff --git a/test/29-firewall.spec.ts b/test/29-firewall.spec.ts index 32714d55..425ec0b9 100644 --- a/test/29-firewall.spec.ts +++ b/test/29-firewall.spec.ts @@ -12,7 +12,7 @@ import * as iptablesMock from './lib/mocked-iptables'; import * as targetStateCache from '../src/device-state/target-state-cache'; import constants = require('../src/lib/constants'); -import { RuleAction } from '../src/lib/iptables'; +import { RuleAction, Rule } from '../src/lib/iptables'; import { log } from '../src/lib/supervisor-console'; describe('Host Firewall', function () { @@ -80,7 +80,6 @@ describe('Host Firewall', function () { // expect that we jump to the firewall chain... expectRule({ - action: RuleAction.Append, target: 'BALENA-FIREWALL', chain: 'INPUT', family: 4, @@ -88,8 +87,6 @@ describe('Host Firewall', function () { // expect to return... expectRule({ - action: RuleAction.Insert, - table: 'filter', chain: 'BALENA-FIREWALL', target: 'RETURN', family: 4, @@ -114,13 +111,22 @@ describe('Host Firewall', function () { }); // expect to return... - expectRule({ - action: RuleAction.Insert, + const returnRuleIdx = expectRule({ table: 'filter', chain: 'BALENA-FIREWALL', target: 'RETURN', family: 4, }); + + // ... just before we reject everything + const rejectRuleIdx = expectRule({ + chain: 'BALENA-FIREWALL', + target: 'REJECT', + matches: iptablesMock.RuleProperty.NotSet, + family: 4, + }); + + expect(returnRuleIdx).to.be.lessThan(rejectRuleIdx); }, ); }); @@ -132,7 +138,7 @@ describe('Host Firewall', function () { await config.set({ firewallMode: 'on' }); await hasAppliedRules; - // expect that we DO have a rule to use the chain... + // expect that we jump to the firewall chain... expectRule({ action: RuleAction.Append, target: 'BALENA-FIREWALL', @@ -140,13 +146,10 @@ describe('Host Firewall', function () { family: 4, }); - // expect to not return... + // expect to not return for any reason... expectNoRule({ - action: RuleAction.Insert, - table: 'filter', chain: 'BALENA-FIREWALL', target: 'RETURN', - family: 4, }); }, ); @@ -186,7 +189,7 @@ describe('Host Firewall', function () { await config.set({ firewallMode: 'auto' }); await hasAppliedRules; - // expect that we DO have a rule to use the chain... + // expect that we jump to the firewall chain... expectRule({ action: RuleAction.Append, target: 'BALENA-FIREWALL', @@ -196,8 +199,6 @@ describe('Host Firewall', function () { // expect to return... expectRule({ - action: RuleAction.Insert, - table: 'filter', chain: 'BALENA-FIREWALL', target: 'RETURN', family: 4, @@ -241,7 +242,7 @@ describe('Host Firewall', function () { await config.set({ firewallMode: 'auto' }); await hasAppliedRules; - // expect that we DO have a rule to use the chain... + // expect that we jump to the firewall chain... expectRule({ action: RuleAction.Append, target: 'BALENA-FIREWALL', @@ -251,8 +252,6 @@ describe('Host Firewall', function () { // expect to return... expectNoRule({ - action: RuleAction.Insert, - table: 'filter', chain: 'BALENA-FIREWALL', target: 'RETURN', family: 4, @@ -279,21 +278,116 @@ describe('Host Firewall', function () { }); describe('Service rules', () => { + const rejectAllRule = { + target: 'REJECT', + chain: 'BALENA-FIREWALL', + matches: iptablesMock.RuleProperty.NotSet, + }; + + const checkForRules = ( + rules: Array> | iptablesMock.Testable, + expectRule: (rule: iptablesMock.Testable) => number, + ) => { + rules = _.castArray(rules); + rules.forEach((rule) => { + const ruleIdx = expectRule(rule); + + // make sure we reject AFTER the rule... + const rejectAllRuleIdx = expectRule(rejectAllRule); + expect(ruleIdx).is.lessThan(rejectAllRuleIdx); + }); + }; + it('should have a rule to allow DNS traffic from the balena0 interface', async () => { await iptablesMock.whilstMocked( async ({ hasAppliedRules, expectRule }) => { - // set the firewall to be in auto mode... + // set the firewall to be on... await config.set({ firewallMode: 'on' }); await hasAppliedRules; - // expect that we have a rule to allow DNS access... - expectRule({ - action: RuleAction.Append, - target: 'ACCEPT', - chain: 'BALENA-FIREWALL', - family: 4, - proto: 'udp', - matches: ['--dport 53', '-i balena0'], + [4, 6].forEach((family: 4 | 6) => { + // expect that we have a rule to allow DNS access... + checkForRules( + { + family, + target: 'ACCEPT', + chain: 'BALENA-FIREWALL', + proto: 'udp', + matches: ['--dport 53', '-i balena0'], + }, + expectRule, + ); + }); + }, + ); + }); + + it('should have a rule to allow SSH traffic any interface', async () => { + await iptablesMock.whilstMocked( + async ({ hasAppliedRules, expectRule }) => { + // set the firewall to be on... + await config.set({ firewallMode: 'on' }); + await hasAppliedRules; + + [4, 6].forEach((family: 4 | 6) => { + // expect that we have a rule to allow SSH access... + checkForRules( + { + family, + target: 'ACCEPT', + chain: 'BALENA-FIREWALL', + proto: 'tcp', + matches: ['--dport 22222'], + }, + expectRule, + ); + }); + }, + ); + }); + + it('should have a rule to allow Multicast traffic any interface', async () => { + await iptablesMock.whilstMocked( + async ({ hasAppliedRules, expectRule }) => { + // set the firewall to be on... + await config.set({ firewallMode: 'on' }); + await hasAppliedRules; + + [4, 6].forEach((family: 4 | 6) => { + // expect that we have a rule to allow multicast... + checkForRules( + { + family, + target: 'ACCEPT', + chain: 'BALENA-FIREWALL', + matches: ['-m addrtype', '--dst-type MULTICAST'], + }, + expectRule, + ); + }); + }, + ); + }); + + it('should have a rule to allow balenaEngine traffic any interface', async () => { + await iptablesMock.whilstMocked( + async ({ hasAppliedRules, expectRule }) => { + // set the firewall to be on... + await config.set({ firewallMode: 'on' }); + await hasAppliedRules; + + [4, 6].forEach((family: 4 | 6) => { + // expect that we have a rule to allow balenaEngine access... + checkForRules( + { + family, + target: 'ACCEPT', + chain: 'BALENA-FIREWALL', + proto: 'tcp', + matches: ['--dport 2375'], + }, + expectRule, + ); }); }, ); @@ -308,10 +402,9 @@ describe('Host Firewall', function () { await config.set({ localMode: true }); await hasAppliedRules; - // make sure we have a rule to allow traffic on ANY interface [4, 6].forEach((family: 4 | 6) => { - expectRule({ - action: RuleAction.Append, + // make sure we have a rule to allow traffic on ANY interface + const allowRuleIdx = expectRule({ proto: 'tcp', matches: [`--dport ${listenPort}`], target: 'ACCEPT', @@ -319,10 +412,24 @@ describe('Host Firewall', function () { table: 'filter', family, }); + + // make sure we have a rule to block traffic on ANY interface also + const rejectRuleIdx = expectRule({ + proto: 'tcp', + matches: [`--dport ${listenPort}`], + target: 'REJECT', + chain: 'BALENA-FIREWALL', + table: 'filter', + family, + }); + + // we should always reject AFTER we allow + expect(allowRuleIdx).to.be.lessThan(rejectRuleIdx); }); }, ); }); + it('should allow limited access in non-localmode', async function () { await iptablesMock.whilstMocked( async ({ hasAppliedRules, expectRule, expectNoRule }) => { @@ -332,9 +439,7 @@ describe('Host Firewall', function () { // ensure we have no unrestricted rule... expectNoRule({ - action: RuleAction.Append, chain: 'BALENA-FIREWALL', - table: 'filter', proto: 'tcp', matches: [`--dport ${listenPort}`], target: 'ACCEPT', @@ -342,12 +447,11 @@ describe('Host Firewall', function () { }); // ensure we do have a restricted rule for each interface... + let allowRuleIdx = -1; constants.allowedInterfaces.forEach((intf) => { [4, 6].forEach((family: 4 | 6) => { - expectRule({ - action: RuleAction.Append, + allowRuleIdx = expectRule({ chain: 'BALENA-FIREWALL', - table: 'filter', proto: 'tcp', matches: [`--dport ${listenPort}`, `-i ${intf}`], target: 'ACCEPT', @@ -355,6 +459,18 @@ describe('Host Firewall', function () { }); }); }); + + // make sure we have a rule to block traffic on ANY interface also + const rejectRuleIdx = expectRule({ + proto: 'tcp', + matches: [`--dport ${listenPort}`], + target: 'REJECT', + chain: 'BALENA-FIREWALL', + table: 'filter', + }); + + // we should always reject AFTER we allow + expect(allowRuleIdx).to.be.lessThan(rejectRuleIdx); }, ); }); diff --git a/test/lib/mocked-iptables.ts b/test/lib/mocked-iptables.ts index 77b6e9ab..c867e1c4 100644 --- a/test/lib/mocked-iptables.ts +++ b/test/lib/mocked-iptables.ts @@ -8,6 +8,12 @@ import * as iptables from '../../src/lib/iptables'; import { EventEmitter } from 'events'; import { Writable } from 'stream'; +export enum RuleProperty { + NotSet, +} + +export type Testable = { [P in keyof T]?: T[P] | RuleProperty | undefined }; + class FakeRuleAdaptor { private rules: iptables.Rule[]; @@ -40,14 +46,21 @@ class FakeRuleAdaptor { } private isSameRule( - partial: Partial, + testable: Testable, rule: iptables.Rule, ): boolean { - const props = Object.getOwnPropertyNames(partial); + const props = Object.getOwnPropertyNames(testable); for (const prop of props) { + if ( + _.get(testable, prop) === RuleProperty.NotSet && + _.get(rule, prop) === undefined + ) { + return true; + } + if ( _.get(rule, prop) === undefined || - !_.isEqual(_.get(rule, prop), _.get(partial, prop)) + !_.isEqual(_.get(rule, prop), _.get(testable, prop)) ) { return false; } @@ -56,17 +69,25 @@ class FakeRuleAdaptor { return true; } - public expectRule(testRule: Partial) { - return expect( - _.some(this.rules, (r) => this.isSameRule(testRule, r)), - ).to.eq( - true, - `Rule has not been applied: ${JSON.stringify( - testRule, - )}\n\n${JSON.stringify(this.rules, null, 2)}`, - ); + public expectRule(testRule: Testable) { + const matchingIndex = (() => { + for (let i = 0; i < this.rules.length; i++) { + if (this.isSameRule(testRule, this.rules[i])) { + return i; + } + } + return -1; + })(); + + if (matchingIndex < 0) { + console.log({ testRule, rules: this.rules }); + } + + expect(matchingIndex).to.be.greaterThan(-1, `Rule has not been applied`); + + return matchingIndex; } - public expectNoRule(testRule: Partial) { + public expectNoRule(testRule: Testable) { return expect( _.some(this.rules, (r) => this.isSameRule(testRule, r)), ).to.eq( @@ -95,8 +116,8 @@ iptables.getDefaultRuleAdaptor = () => { export interface MockedState { hasAppliedRules: Promise; - expectRule: (rule: iptables.Rule) => void; - expectNoRule: (rule: iptables.Rule) => void; + expectRule: (rule: Testable) => number; + expectNoRule: (rule: Testable) => void; clearHistory: () => void; }