group and simplify iptables INPUT rules #3072

singuliere · 2018-02-27T11:50:52Z

Status

Ready for review

Description of Changes

Fixes #1236

Testing

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances.
New installs.

Checklist

If you made changes to the app code:

Unit and functional tests pass on the development VM

If you made changes to the system configuration:

Configuration tests pass

If you made changes to documentation:

Doc linting passed locally

emkll

Thank you @singuliere for your pull request! This makes the rules quite a bit simpler indeed :)

It appears that the tests (testinfra/app/test_network.py) and test variables have been deleted (testinfra/vars/app-prod.yml, testinfra/vars/mon-prod.yml). These are important, specifically because iptables rules are different in staging and production.
To fix these tests, you will need to to grab the output of iptables-save on both production app and monitor servers. Let me know if you need some help with that.

Otherwise, I have a couple of small questions/comments inline. Thanks again!

emkll · 2018-03-06T18:58:35Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4

@@ -4,9 +4,12 @@
 :OUTPUT DROP [0:0]
 :LOGNDROP - [0:0]

+-A INPUT         -m state --state ESTABLISHED -j ACCEPT   -m comment --comment "Allow traffic back"


Is there a specific reason as to why this rule was moved to the top (vs. the rule here)? There are also a few extra spaces after INPUT and ACCEPT

I see this rule omits -p tcp, thus allowing all established UDP traffic inbound. While is it less restrictive than the previous iteration, I don't immediately see a problem with this.

why this rule was moved to the top

Because it is generic. It is not limited to tor traffic, contrary to what the comment claims.

emkll · 2018-03-06T19:09:54Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4

 # Prod ssh connections happen through an authenticated tor hidden service
 # The ssh connection is proxied on the server by the tor client to
 # the ssh dameon listening on the local loopback.
+-A INPUT -i lo -p tcp --dport 22 -m state --state NEW -j ACCEPT -m comment --comment "Allow tor connection from local loopback to connect to ssh"


Could you please clarify your reasoning behind this rule? It is my understanding that for ssh over tor should be handled here

this rule is for the INPUT chain

Makes sense as the rule on L115 was removed :)

emkll · 2018-03-06T20:02:00Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4


 # Block all other traffic by application users
 # Load before generic loopback rules
 -A OUTPUT -m owner --uid-owner www-data -j LOGNDROP -m comment --comment "Drop all other traffic by the securedrop user"

 {% endif %}

-# Block all other outbound access for users in the ssh group
-# Load before generic loopback rules
-A OUTPUT -m owner --gid-owner ssh -j LOGNDROP -m comment --comment "Drop all other outbound traffic for ssh user"


Why was this line removed?

there are no users in the ssh group

ageis · 2018-03-10T19:47:27Z

Thanks @singuliere.

Entirely tangential comments that probably deserve their own issue... but about the way SecureDrop manages iptables rules in general. It's not ideal... James Dolan (RIP) left a comment in his commit that the whole process should be redesigned (see head of install_files/ansible-base/roles/restrict-direct-access/tasks'iptables.yml)... To begin with, we apply the rules with an ifup-d wrapper, e.g. when a network link becomes active. I can think of ways this could be bypassed. A slight improvement (to the way we apply them) would be to manage them with a systemd service instead. You could accomplish it all in one unit:

[Unit]
Description=Manage iptables rules
Before=network.target
Before=shutdown.target
DefaultDependencies=yes

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStartPre=/bin/mkdir -p /etc/network/iptables/
ExecStart=/sbin/iptables-restore /etc/network/iptables/rules_v4
ExecStop=/sbin/iptables-save /etc/network/iptables/rules_v4
ExecReload=/sbin/iptables-save /etc/network/iptables/rules_v4

[Install]
WantedBy=multi-user.target

Rules are restored on boot, before there's even networking, and saved every time the host gets shut down. More secure, but also stateful — remembers alterations. This doesn't address the needed bypass in staging/during installation though. We also have issue #845 left open. It would serve well to audit and test the rules to ensure they do what they're supposed to do, or perhaps consider other approaches, such as a firewall management daemon like ufw or nufw/UFWI or strongwall? Just wanted to attempt to kickstart a discussion somewhere, since this piece of the project hasn't seen much love.

ghost · 2018-03-12T15:03:26Z

@singuliere gentle ping?

codecov-io · 2018-03-15T13:34:05Z

Codecov Report

Merging #3072 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #3072   +/-   ##
=======================================
  Coverage     84.6%   84.6%           
=======================================
  Files           34      34           
  Lines         2027    2027           
  Branches       221     221           
=======================================
  Hits          1715    1715           
  Misses         258     258           
  Partials        54      54

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e43c9c...f575cd3. Read the comment docs.

The rule: -A INPUT -p tcp -m state --state ESTABLISHED,RELATED -j ACCEPT shadows all other TCP rules. The rule: -A INPUT -i lo -p all -j ACCEPT shadows all other TCP/UDP rules for lo, regardless of their state. They are replaced by one rule, for both udp and tcp: -A INPUT -m state --state ESTABLISHED -j ACCEPT There is no need to account for the RELATED state because no protocol for which it would be useful is allowed (ICMP, ftp etc.). The remaining INPUT rules are left in place only when they explicitly permit NEW connections on a given port, others are removed. A new INPUT rule is added to allow new connections to SSH from the user running the tor daemon.

emkll

Thanks for the clarifications @singuliere. From my perspective, the rules look sound, however the removal of tests is a significant regression. Please let us know if you intend on fixing these or if you would like some help.

As stated previously, running sudo iptables-save on a running server should provide you with enough information to amend the test variables. Once the test_network.py tests are restored for app and mon, you can run the testing suite locally by doing ./testinfra/tests-py {app,mon}-staging (see: https://docs.securedrop.org/en/latest/development/testing_configuration_tests.html)

emkll · 2018-03-15T21:52:11Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4


 # Block all other traffic by application users
 # Load before generic loopback rules
 -A OUTPUT -m owner --uid-owner www-data -j LOGNDROP -m comment --comment "Drop all other traffic by the securedrop user"

 {% endif %}

-# Block all other outbound access for users in the ssh group
-# Load before generic loopback rules
-A OUTPUT -m owner --gid-owner ssh -j LOGNDROP -m comment --comment "Drop all other outbound traffic for ssh user"


emkll · 2018-03-15T21:52:32Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4

 # Prod ssh connections happen through an authenticated tor hidden service
 # The ssh connection is proxied on the server by the tor client to
 # the ssh dameon listening on the local loopback.
+-A INPUT -i lo -p tcp --dport 22 -m state --state NEW -j ACCEPT -m comment --comment "Allow tor connection from local loopback to connect to ssh"


Makes sense as the rule on L115 was removed :)

ghost · 2018-03-15T23:01:21Z

@emkll @singuliere IMHO it would make more sense to verify the intended side effect of the iptables rules (e.g. a packet sent to 8080 from the wrong user or interface is actually dropped & logged). There is very little value (if any) in dumping the rules to verify they are identical to the one that were just loaded. At best it verifies iptables-save/restore works as intended. My 2cts ;-)

conorsch · 2018-03-15T23:04:25Z

it would make more sense to verify the intended side effect of the iptables rules (e.g. a packet sent to 8080 from the wrong user or interface is actually dropped & logged)

👍 Could not agree more, @dachary. The current config tests, particularly on the host-level firewall side of things, merely vet the intended implementation, rather than the desired system end state. Once we move to externalizing the tests, e.g. using client outside the app server as described in #2875, we should also revisit the config tests and make sure that we're validating our hardening by banging on the server from the outside.

singuliere · 2018-03-16T16:58:02Z

I will write tests

ghost · 2018-03-26T11:33:03Z

@singuliere ping?

singuliere · 2018-04-03T14:06:54Z

I will rebase

ghost · 2018-05-20T07:36:42Z

This needs rebasing.

eloquence · 2018-05-31T18:39:56Z

Hey @singuliere , do you still have time to work on this? If not, we can rebase/re-do the change. Thanks for your initial stab at it!

singuliere · 2018-06-11T13:02:00Z

Closing because I do not have time to work on this.

singuliere requested review from conorsch and msheiny as code owners February 27, 2018 11:50

singuliere force-pushed the issue-1236-iptables branch from 1ab4200 to 442de94 Compare February 27, 2018 11:51

ghost added the ops/deployment label Feb 27, 2018

singuliere force-pushed the issue-1236-iptables branch 3 times, most recently from 27b953e to d08af69 Compare March 3, 2018 12:55

redshiftzero requested a review from emkll March 6, 2018 17:57

emkll suggested changes Mar 6, 2018

View reviewed changes

singuliere force-pushed the issue-1236-iptables branch 3 times, most recently from 47e1e04 to 6a3ad27 Compare March 15, 2018 12:50

singuliere added 3 commits March 15, 2018 21:04

remove restriction on empty ssh user group

1e193dc

remove redundant iptables comparison

f575cd3

singuliere force-pushed the issue-1236-iptables branch from 7e3ca5a to f575cd3 Compare March 15, 2018 20:10

emkll reviewed Mar 15, 2018

View reviewed changes

singuliere closed this Jun 11, 2018

eloquence mentioned this pull request Dec 2, 2020

Support for Ubuntu 20.04 (Focal) #4768

Closed

53 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group and simplify iptables INPUT rules #3072

group and simplify iptables INPUT rules #3072

singuliere commented Feb 27, 2018

emkll left a comment

emkll Mar 6, 2018

emkll Mar 6, 2018

singuliere Mar 15, 2018

emkll Mar 6, 2018

singuliere Mar 15, 2018

emkll Mar 15, 2018

emkll Mar 6, 2018

singuliere Mar 15, 2018

emkll Mar 15, 2018

ageis commented Mar 10, 2018

ghost commented Mar 12, 2018

codecov-io commented Mar 15, 2018 •

edited

Loading

emkll left a comment

emkll Mar 15, 2018

emkll Mar 15, 2018

ghost commented Mar 15, 2018

conorsch commented Mar 15, 2018

singuliere commented Mar 16, 2018

ghost commented Mar 26, 2018

singuliere commented Apr 3, 2018

ghost commented May 20, 2018

eloquence commented May 31, 2018

singuliere commented Jun 11, 2018

group and simplify iptables INPUT rules #3072

group and simplify iptables INPUT rules #3072

Conversation

singuliere commented Feb 27, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the app code:

If you made changes to the system configuration:

If you made changes to documentation:

emkll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ageis commented Mar 10, 2018

ghost commented Mar 12, 2018

codecov-io commented Mar 15, 2018 • edited Loading

Codecov Report

emkll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 15, 2018

conorsch commented Mar 15, 2018

singuliere commented Mar 16, 2018

ghost commented Mar 26, 2018

singuliere commented Apr 3, 2018

ghost commented May 20, 2018

eloquence commented May 31, 2018

singuliere commented Jun 11, 2018

codecov-io commented Mar 15, 2018 •

edited

Loading