when enabling ip forwarding set the default forward policy to drop

[sanimej]

Related to docker #14041

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]

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

[sanimej]

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

[aboch]

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

[aboch]

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]

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]

@aboch Addressed the comments. PTAL

[aboch]

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)

[sanimej]

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.

[aboch]

Ah that's right. Thanks.

[aboch]

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.

[sanimej]

@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]

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

LGTM