Firewall groups are not ordered on port associations

[Anthony]

Neutron allows multiple firewall groups to be attached to ports:

MariaDB [neutron]> describe firewall_group_port_associations_v2;
+-------------------+-------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+-------------------+-------------+------+-----+---------+-------+
| firewall_group_id | varchar(36) | YES | MUL | NULL | |
| port_id | varchar(36) | YES | UNI | NULL | |
+-------------------+-------------+------+-----+---------+-------+
2 rows in set (0.001 sec)

However, these associations are not ordered.

We have a use-case where we'd like to define a shared firewall group that tenants can apply in their environments, then add their own, private firewall group above the shared group.

For example, suppose we have an admin owned shared group that allows some RFC1918 space, denies all other RFC1918 networks, then passes all other traffic. A tenant should be able to apply this group, then put their own group above it to allow additional RFC1918 space.

Currently, the group associations will shift on association, so the private group can get popped to below the shared group, effectively blocking traffic that should be allowed.

This can also be a security vulnerability, as the shifting of firewall groups and reordering of rules could easily allow traffic that was previously denied.

Firewall group port associations should be ordered so that the combined ruleset on a port is evaluated in the desired order.

[Brian Haley (brian-haley)]

Just to be clear, this is talking about the neutron-fwaas component, correct?

When the v2 of the API was created, it actually talks about this specific issue in the "Multiple Firewall Policies" section:

https://specs.openstack.org/openstack/neutron-specs/specs/newton/fwaas-api-2.0.html

That said, I can see how there's the possibility that something could get dropped based on how things are created.

But how can something be allowed and create a security vulnerability? If you have created a group/rule that allows a packet to pass but the ordering disallows it (by accident), but a restart somehow shifts the order to then allow it, that doesn't seem like a bug. In this case you did want to let the packet through and it just wasn't happening in some circumstances. Can you outline a case where rules were mis-applied? Maybe I'm mis-understanding.

[Anthony (atimmins)]

Yes, this is concerning neutron-fwaas.

Let's take an example where one group looks like this:

openstack firewall group rule create --protocol tcp \
  --destination-ip-address 192.168.0.100 \
  --destination-port 443 \
  --action allow \
  --name webserverA-allow

openstack firewall group rule create --protocol any \
  --destination-ip-address 192.168.0.100 \
  --action deny \
  --name webserverA-deny

openstack firewall group rule create --protocol tcp \
  --destination-ip-address 192.168.0.0/24 \
  --action allow \
  --destination-port 22 \
  --name ssh-allow

openstack firewall group policy create \
  --firewall-rule webserverA-allow \
  --firewall-rule webserverA-deny \
  --firewall-rule ssh-allow \
  testmultiplegroups1

openstack firewall group create \
  --egress-firewall-policy testmultiplegroups1 \
  --name mytestgroup1

Now we'll create a second firewall group to allow tcp 3306 to everything in the subnet. With this group placed below the previous, webserverA (192.168.0.100) would not be included in this allow statement.

openstack firewall group rule create --protocol tcp \
  --destination-ip-address 192.168.0.0/24 \
  --action allow \
  --destination-port 3306 \
  --name mysql-allow

openstack firewall group policy create \
  --firewall-rule mysql-allow \
  testmultiplegroups2

openstack firewall group create \
  --egress-firewall-policy testmultiplegroups2 \
  --name mytestgroup2

The combined rules would be:

  --firewall-rule webserverA-allow
  --firewall-rule webserverA-deny
  --firewall-rule ssh-allow
  --firewall-rule mysql-allow

When the order of the groups is shuffled, the new combined ruleset becomes:

  --firewall-rule mysql-allow
  --firewall-rule webserverA-allow
  --firewall-rule webserverA-deny
  --firewall-rule ssh-allow

TCP 3306 is now unintentionally open to webserverA.

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete
security advisory task has been added while the core security
reviewers for the affected project or projects confirm the bug and
discuss the scope of any vulnerability along with potential
solutions.

Although the OpenStack Vulnerability Management Team doesn't
officially oversee[*] reports for the neutron-fwaas repository,
I've gone ahead and triaged this for now as if we would. That
said, I don't see a compelling reason to hold the discussion of
the presumed defect in private. Based on the comments above, I
don't think the described behavior becoming public knowledge
would put anyone's deployments at risk (if anything, it may help
some users by reminding them to double-check the composition of
their firewall rules).

[*] https://security.openstack.org/repos-overseen.html

[ZhouHeng (zhouhenglc)]

Currently, the master code does not allow one port to be associated with multiple firewall groups, call chain is
update_firewall_group[1] -> update_firewall_group[2] -> _set_ports_for_firewall_group[3] -> raise FirewallGroupPortInUse[4].
The current database design also allows only one associated record per port[5][6]

Which version of the test is this？

[1] https://opendev.org/openstack/neutron-fwaas/src/branch/master/neutron_fwaas/services/firewall/fwaas_plugin_v2.py#L359
[2] https://opendev.org/openstack/neutron-fwaas/src/branch/master/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L985
[3] https://opendev.org/openstack/neutron-fwaas/src/branch/master/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L822
[4] https://opendev.org/openstack/neutron-fwaas/src/branch/master/neutron_fwaas/db/firewall/v2/firewall_db_v2.py#L838
[5] https://opendev.org/openstack/neutron-fwaas/src/branch/master/neutron_fwaas/db/migration/alembic_migrations/versions/queens/expand/f24e0d5e5bff_uniq_firewallgroupportassociation0port.py
[6] https://review.opendev.org/c/openstack/neutron-fwaas/+/512154

[Jeremy Stanley (fungi)]

As Brian points out, the described behavior is as designed (if
perhaps surprising to users). Per the linked specification:

> When there are multiple firewall groups associated with a specific
> Neutron port, there is no position or priority between the
> different firewall groups. Some deterministic behavior must be
> defined in order to resolve the action to be taken when some
> firewall groups determine an “allow” action while other firewall
> groups determine a “deny” action.
>
> This spec defines that packets will be allowed if any one of the
> firewall groups associated with that Neutron port allows the
> packet. This behavior is similar to the case of multiple Security
> Groups associated with the same VM port.

Given this, short of redesigning the API, we're presumably looking
at possible improvements to the documentation in order to make the
described pitfall more obvious to end users. If this is correct, I
propose we switch this to a normal public bug in order to continue
discussion with the broader community on ways to better describe
how this interface works.

[Anthony (atimmins)]

> This spec defines that packets will be allowed if any one of the
> firewall groups associated with that Neutron port allows the
> packet. This behavior is similar to the case of multiple Security
> Groups associated with the same VM port.

This works with security groups on VM ports because there are no "deny" actions. When we have "deny" actions in firewall policies, and there are multiple policies applied to a port, if one policy allows the traffic and another policy denies the traffic, there is a conflict. It now becomes random whether or not that traffic will be allowed or denied.

It is possible to assign multiple groups to ports during firewall group creation by not specifying a port in the group creation command.

[Jeremy Stanley (fungi)]

On 2022-06-20 13:29:33 -0000 (-0000), Anthony wrote:
[...]
> This works with security groups on VM ports because there are no
> "deny" actions. When we have "deny" actions in firewall policies,
> and there are multiple policies applied to a port, if one policy
> allows the traffic and another policy denies the traffic, there is
> a conflict. It now becomes random whether or not that traffic will
> be allowed or denied.
[...]

Agreed. I'm arguing that, per the spec, that traffic should always
be allowed because at least one firewall rule group allows it. The
fact that it's sometimes denied by a deny rule in one of the groups
seems like the actual bug here, but I can see how that inconsistency
could lead a user to assume the blocking was intentional behavior
and then start relying on it.

And yes, with my security administrator hat on, I agree that even
ignoring this particular inconsistency, it's a confusing design to
not have deterministic ordering from the application of multiple
rulesets. As far as determining whether there's a security
vulnerability in the software, however (and whether it's severe
enough to warrant continued discussion in secret), we need to
consider the behavior the software is intended to have rather than
the behavior users might want it to have.
--
Jeremy Stanley

[Anthony (atimmins)]

The documentation states that the intended behavior is that "packets will be allowed if any one of the firewall groups associated with that Neutron port allows the packet." This is not a true statement, as I demonstrated in my configuration example above. If one group allows the packet, but another group explicitly denies the packet, the packet will be denied depending on the ordering of the groups on the port. Therefore, I'd argue that the software is not working as intended.

The ability to assign multiple groups to a port is a desired feature. It would allow administrators to apply a default top-level group to projects, and allow users to add their own groups below it to extend access to their project. However, the current behavior prevents this because the groups are re-ordered unintentionally.

Adding ordering/positioning to attached groups seems to be the best path forward.

[Jeremy Stanley (fungi)]

Yes, I agree that there's a bug. What I was trying to point out is that, based on the specification for how it's intended to work, the bug is that sometimes packets are getting blocked when they should be passed. Because of this, I think the bug is one we should be able to discuss and solve in public rather than keeping it secret and trying to identify solutions and review patches for it entirely in private.

[Anthony (atimmins)]

The inverse is also true, that packets could be passed when they should be blocked. If I am explicitly blocking a packet in group 1, but it would be passed by a broader statement in group 2, and the order of those groups flips, I am now passing that packet. This is the basis for considering this bug to be a vulnerability.

The argument is whether or not publicizing the details will be more of a benefit to the community than a risk.

[Jeremy Stanley (fungi)]

Re-quoting the relevant part of the spec:

> packets will be allowed if any one of the firewall groups
> associated with that Neutron port allows the packet

Per your description, packets are sometimes blocked and
sometimes allowed depending on the group order, even though the
specification says they should always be allowed. A such, any
backportable solution to this bug on stable branches (if even
possible) will involve making sure the behavior matches the
specification. That the nature of the bug caused you to expect
things to be blocked even though the specification says they
should not be certainly counts as a security risk, and is
something which could suggest it's worthwhile to revisit this
design in future development of the service, but OpenStack is
committed to open design methodologies and avoids redesigning
services in secret.

That said, the OpenStack Vulnerability Management Team does
not officially oversee reports of suspected vulnerabilities
for the neutron-fwaas project, so I'll step back at this point
and let the developers for it decide how they wish to proceed.

[Lajos Katona (lajos-katona)]

Thanks for the detailed analysis of this issue, @atimmins if you would like to improve the current behaviour please open a bug report with RFE in subject, and we can discuss that on Neutron Drivers meeting: https://meetings.opendev.org/#Neutron_drivers_Meeting

[Anthony (atimmins)]

Thanks, @lajos-katona! I have filed a new bug report per your request here: https://bugs.launchpad.net/neutron/+bug/1979816

[Jeremy Stanley (fungi)]

I would also recommend the FWAAS maintainers switch this earlier bug to public, especially since the new bug report makes a direct reference to it, so there is no point in keeping it secret any further (if there ever was).

[Lajos Katona (lajos-katona)]

Agree, I hope we can discuss today the RFE based on this bug (#1979816)