when enabling ip forwarding set the default forward policy to drop by sanimej · Pull Request #1526 · moby/libnetwork

sanimej · 2016-10-28T21:15:05Z

If docker daemon enables ip_forwarding on a host set the policy in the FORWARD chain to DROP. This blocks the vulnerability explained in the above mentioned issue. Setting the policy is done from the bridge driver to keep it consistent with the current handling for ip_forward flag.

Signed-off-by: Santhosh Manohar santhosh@docker.com

aboch · 2016-10-28T21:28:33Z

drivers/bridge/setup_ip_forwarding.go

 			return fmt.Errorf("Setup IP forwarding failed: %v", err)
 		}
+		if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil {
+			return fmt.Errorf("setting default policy failed: %v", err)


SetDefaultPolicy already returns a comprehensive error string. This one is not adding anything new to that.
I would just return err here.

yeah.. I was also thinking about just returning the error here. will change it.

aboch · 2016-10-28T21:29:10Z

drivers/bridge/setup_ip_forwarding.go

 			return fmt.Errorf("Setup IP forwarding failed: %v", err)
 		}
+		if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil {
+			return fmt.Errorf("setting default policy failed: %v", err)


In case of failure to set the policy to drop, we must reset ip_forwarding to 0

aboch · 2016-10-28T21:33:13Z

We will now touch the iptables (changing the default policy) even if daemon was started with --iptables=false.
I guess it is fine, given we are addressing a vulnerability.
We'll need to document this behavior.

sanimej · 2016-11-01T16:48:53Z

We will now touch the iptables (changing the default policy) even if daemon was started with --iptables=false. - On second thought I think its probably better to honor the iptabels=false config. Whoever sets it wants full control of the iptables and its their responsibility to also set the default policies correctly.

sanimej · 2016-11-01T21:46:01Z

@aboch Addressed the comments. PTAL

aboch · 2016-11-01T22:15:39Z

drivers/bridge/bridge.go

 		return &ErrInvalidDriverConfig{}
 	}

-	if config.EnableIPForwarding {


Given the new logic is to change the FORWARD chain default policy only if config.EnableIPTables==true, I am thinking we do not need to move this code, neither make any change in SetupForwarding().
We only need to add in SetupIPChain(configuration)

if config.EnableIPForwarding { if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { return fmt.Errorf("%v. IP forwarding was set to 1", err) } }

Also this will guarantee the action will take place when the firewall is reloaded:
iptables.OnReloaded(..., setupIPChains)

config.EnableIPForwarding is set based on the daemon flag and not by checking the sysctl ip_forward. By doing the way you are suggesting we will call SetDefaultPolicy even if ip_forward is already enabled; ie: ip_forward might be enabled and the default policy set be ACCEPT. Docker changing that to DROP is incorrect.

Ah that's right. Thanks.

aboch · 2016-11-02T18:06:23Z

After some more thinking of this and talk offline, I see the FORWARD chain policy will be set to DROP only if the ip forwarding is being enabled during this running instance.

If the daemon was already running and we upgrade it to the version containing this fix, or if the user has it enabled via other means, the policy will not be set to DROP.

I believe this is for assuring we do not disrupt current behavior on the host.

I would be more on the conservative side and always set the policy to DROP in SetupIPChain() if the ip_forward flag read from OS is 1. Reason is that given we cannot know for sure existing forward flag set to 1 is intentional, I think it is better to always change the policy.

But I understand this is opinable.

Signed-off-by: Santhosh Manohar <santhosh@docker.com>

sanimej · 2016-11-04T03:56:45Z

@aboch When firewalld is enabled, if firewalld is restarted the default policy has to be set again. Added a function to OnReloaded to handle this.

mavenugo · 2016-11-10T18:11:47Z

@sanimej after much discussion, I think this is a reasonable approach considering the backward compatibility. Lets document this clearly in docker/docker.

LGTM

full diff: moby/libnetwork@92d1fbe...96bcc0d changes included: - moby/libnetwork#2429 Updating IPAM config with results from HNS create network call - addresses moby#38358 - moby/libnetwork#2450 Always configure iptables forward policy - related to moby#14041 and moby/libnetwork#1526 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: moby/libnetwork@92d1fbe...96bcc0d changes included: - moby/libnetwork#2429 Updating IPAM config with results from HNS create network call - addresses moby/moby#38358 - moby/libnetwork#2450 Always configure iptables forward policy - related to moby/moby#14041 and moby/libnetwork#1526 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 75477f0b3c77f2108a6b5586dbc246c52b479941 Component: engine

full diff: moby/libnetwork@92d1fbe...96bcc0d changes included: - moby/libnetwork#2429 Updating IPAM config with results from HNS create network call - addresses moby#38358 - moby/libnetwork#2450 Always configure iptables forward policy - related to moby#14041 and moby/libnetwork#1526 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 75477f0) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: moby/libnetwork@92d1fbe...96bcc0d changes included: - moby/libnetwork#2429 Updating IPAM config with results from HNS create network call - addresses moby/moby#38358 - moby/libnetwork#2450 Always configure iptables forward policy - related to moby/moby#14041 and moby/libnetwork#1526 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 75477f0b3c77f2108a6b5586dbc246c52b479941) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 559be42fc26048f4069de64f84202803a113413a Component: engine

full diff: moby/libnetwork@92d1fbe...96bcc0d changes included: - moby/libnetwork#2429 Updating IPAM config with results from HNS create network call - addresses moby#38358 - moby/libnetwork#2450 Always configure iptables forward policy - related to moby#14041 and moby/libnetwork#1526 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>

GordonTheTurtle added the status/0-triage label Oct 28, 2016

aboch reviewed Oct 28, 2016

View reviewed changes

aboch suggested changes Oct 28, 2016

View reviewed changes

sanimej force-pushed the policy branch from 2c4442b to f96ea38 Compare November 1, 2016 21:37

sanimej force-pushed the policy branch from f96ea38 to e462c77 Compare November 1, 2016 21:49

aboch suggested changes Nov 1, 2016

View reviewed changes

aboch approved these changes Nov 2, 2016

View reviewed changes

If enabling ip forwarding set the default forward policy to drop

a493ddb

Signed-off-by: Santhosh Manohar <santhosh@docker.com>

sanimej force-pushed the policy branch from e462c77 to a493ddb Compare November 4, 2016 03:54

mavenugo merged commit 1861587 into moby:master Nov 10, 2016

rn mentioned this pull request Dec 29, 2016

How to access containers by internal IPs 172.x.x.x docker/for-win#221

Closed

wclr mentioned this pull request Jan 12, 2017

Add ip-forward-policy to deamon configuration moby/moby#30093

Closed

TheNodi mentioned this pull request Sep 12, 2019

Always configure iptables forward policy #2450

Merged

thaJeztah mentioned this pull request Sep 20, 2019

bump libnetwork to 96bcc0dae898308ed659c5095526788a602f4726 moby/moby#39967

Merged

thaJeztah mentioned this pull request Sep 20, 2019

[19.03 backport] bump libnetwork to 96bcc0dae898308ed659c5095526788a602f4726 docker-archive/engine#372

Merged

thaJeztah mentioned this pull request Mar 6, 2020

docker-ce package makes existing KVM guests unusable docker/for-linux#949

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

when enabling ip forwarding set the default forward policy to drop#1526

when enabling ip forwarding set the default forward policy to drop#1526
mavenugo merged 1 commit intomoby:masterfrom
sanimej:policy

sanimej commented Oct 28, 2016

Uh oh!

aboch Oct 28, 2016

Uh oh!

sanimej Oct 28, 2016

Uh oh!

aboch Oct 28, 2016

Uh oh!

aboch commented Oct 28, 2016

Uh oh!

sanimej commented Nov 1, 2016

Uh oh!

sanimej commented Nov 1, 2016

Uh oh!

aboch Nov 1, 2016

Uh oh!

sanimej Nov 2, 2016

Uh oh!

aboch Nov 2, 2016

Uh oh!

aboch commented Nov 2, 2016

Uh oh!

sanimej commented Nov 4, 2016

Uh oh!

mavenugo commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sanimej commented Oct 28, 2016

Uh oh!

aboch Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

sanimej Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

aboch Oct 28, 2016

Choose a reason for hiding this comment

Uh oh!

aboch commented Oct 28, 2016

Uh oh!

sanimej commented Nov 1, 2016

Uh oh!

sanimej commented Nov 1, 2016

Uh oh!

aboch Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

sanimej Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

aboch Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

aboch commented Nov 2, 2016

Uh oh!

sanimej commented Nov 4, 2016

Uh oh!

mavenugo commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants