Security groups fail to be set correctly if incorrect case is used for protocol specification

[Robert Clark]

The high level issue is that if a security group rule is specified with the protocol in uppercase (e.g. TCP instead of tcp) on a system using the IpTablesFirewallDriver then the security group rules may fail to be properly applied, leading to security groups that are more open than specified.

The detail of the issue is as follows (Described from the OSAPI perspective, but the problem also exists on EC2)

When a security group rule is specified with the protocol in upper case it is validated (contrig/security_groups.py: _rule_args_to_dict() regardless of case but stored in the database in the supplied case:
    if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']:
                raise exception.InvalidIpProtocol(protocol=ip_protocol)

  …

  values['protocol'] = ip_protocol

When the security group refresh is triggered (virt/firewall.py – instance_rules() the protocol check is case sensitive:

                if protocol in ['udp', 'tcp']:
                    args += self._build_tcp_udp_rule(rule, version)
                elif protocol == 'icmp':
                    args += self._build_icmp_rule(rule, version)
                if rule.cidr:
                    LOG.info('Using cidr %r', rule.cidr)
                    args += ['-s', rule.cidr]
                    fw_rules += [' '.join(args)]

Because the protocol doesn’t match ‘udp’ or ‘tcp’ the protocol part of the rule is skipped, leading to an incomplete and invalid iptables command line.

[Robert Clark (robert-clark)]

@all I suggest that this is a "Normal" level security bug as per: http://wiki.openstack.org/VulnerabilityManagement

Looks like it should be an easy fix, thoughts?

[Vish Ishaya (vishvananda)]

yup appears easy. Suggested patch:

diff --git a/nova/virt/firewall.py b/nova/virt/firewall.py
index 3f53334..62de2c2 100644
--- a/nova/virt/firewall.py
+++ b/nova/virt/firewall.py
@@ -300,7 +300,7 @@ class IptablesFirewallDriver(FirewallDriver):
                 else:
                     fw_rules = ipv6_rules

- protocol = rule.protocol
+ protocol = rule.protocol.lower()
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'

[Phil Day (philip-day)]

For our fix we did the tolower() before writing to the db, in case there were any other similar issues in the code (we know that it works everywhere when the DB contents are lower case)

Phil

-- Sent from my HP Pre

________________________________
On 19 Apr 2012 10:15, Vish Ishaya <email address hidden> wrote:

yup appears easy. Suggested patch:

diff --git a/nova/virt/firewall.py b/nova/virt/firewall.py
index 3f53334..62de2c2 100644
--- a/nova/virt/firewall.py
+++ b/nova/virt/firewall.py
@@ -300,7 +300,7 @@ class IptablesFirewallDriver(FirewallDriver):
                 else:
                     fw_rules = ipv6_rules

- protocol = rule.protocol
+ protocol = rule.protocol.lower()
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'

--
You received this bug notification because you are subscribed to the bug
report.
https://bugs.launchpad.net/bugs/985184

Title:
  Security groups fail to be set correctly if incorrect case is used for
  protocol specification

Status in OpenStack Compute (Nova):
  New

Bug description:
  The high level issue is that if a security group rule is specified
  with the protocol in uppercase (e.g. TCP instead of tcp) on a system
  using the IpTablesFirewallDriver then the security group rules may
  fail to be properly applied, leading to security groups that are more
  open than specified.

  The detail of the issue is as follows (Described from the OSAPI
  perspective, but the problem also exists on EC2)

  When a security group rule is specified with the protocol in upper case it is validated (contrig/security_groups.py: _rule_args_to_dict() regardless of case but stored in the database in the supplied case:
      if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']:
                  raise exception.InvalidIpProtocol(protocol=ip_protocol)

    …

    values['protocol'] = ip_protocol

  When the security group refresh is triggered (virt/firewall.py – instance_rules() the protocol check is case sensitive:

                  if protocol in ['udp', 'tcp']:
                      args += self._build_tcp_udp_rule(rule, version)
                  elif protocol == 'icmp':
                      args += self._build_icmp_rule(rule, version)
                  if rule.cidr:
                      LOG.info('Using cidr %r', rule.cidr)
                      args += ['-s', rule.cidr]
                      fw_rules += [' '.join(args)]

  Because the protocol doesn’t match ‘udp’ or ‘tcp’ the protocol part of
  the rule is skipped, leading to an incomplete and invalid iptables
  command line.

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/985184/+subscriptions

[Vish Ishaya (vishvananda)]

The issue with that solution is it requires a migration to fix any bad data
that might already be in the database. I don't have a problem with lowering
it on both ends though.

[Phil Day (philip-day)]

Fair point, although if you had any bad values in the DB I doubt if they would go un-noticed ;-)

Phil

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Vish Ishaya
Sent: 20 April 2012 04:09
To: Day, Phil
Subject: Re: [Bug 985184] Re: Security groups fail to be set correctly if incorrect case is used for protocol specification

The issue with that solution is it requires a migration to fix any bad data that might already be in the database. I don't have a problem with lowering it on both ends though.

--
You received this bug notification because you are subscribed to the bug report.
https://bugs.launchpad.net/bugs/985184

Title:
  Security groups fail to be set correctly if incorrect case is used for
  protocol specification

Status in OpenStack Compute (Nova):
  New

Bug description:
  The high level issue is that if a security group rule is specified
  with the protocol in uppercase (e.g. TCP instead of tcp) on a system
  using the IpTablesFirewallDriver then the security group rules may
  fail to be properly applied, leading to security groups that are more
  open than specified.

  The detail of the issue is as follows (Described from the OSAPI
  perspective, but the problem also exists on EC2)

  When a security group rule is specified with the protocol in upper case it is validated (contrig/security_groups.py: _rule_args_to_dict() regardless of case but stored in the database in the supplied case:
      if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']:
                  raise exception.InvalidIpProtocol(protocol=ip_protocol)

    …

    values['protocol'] = ip_protocol

  When the security group refresh is triggered (virt/firewall.py – instance_rules() the protocol check is case sensitive:

                  if protocol in ['udp', 'tcp']:
                      args += self._build_tcp_udp_rule(rule, version)
                  elif protocol == 'icmp':
                      args += self._build_icmp_rule(rule, version)
                  if rule.cidr:
                      LOG.info('Using cidr %r', rule.cidr)
                      args += ['-s', rule.cidr]
                      fw_rules += [' '.join(args)]

  Because the protocol doesn’t match ‘udp’ or ‘tcp’ the protocol part of
  the rule is skipped, leading to an incomplete and invalid iptables
  command line.

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/985184/+subscriptions

[Robert Clark (robert-clark)]

Ok, is there agreement on the correct way to patch this?

I'd also like to issue some advice on how to find currently-incorrect DB entries for the protocol type with the release, @thierry would you be happy with that?

[Thierry Carrez (ttx)]

We can definitely add to the security notice some hints (SQL one-liner anyone ?) on sanitizing existing entries. Deployers/distros can then assess if changing existing data should be part of their upgrade process or not.

[Robert Clark (robert-clark)]

Title: Security groups fail to be set correctly
Impact: High
Reporter: HP Cloud Services <email address hidden>
Products: Nova
Affects: All versions

Description:
HP Cloud Services reported a vulnerability in Nova API handling when a security group is created via the API using a protocol defined in the incorrect case i.e 'TCP' rather than 'tcp' causes a later string comparison to fail. This leads to Security Groups not being set correctly.

Once the Nova DB has been polluted with the incorrect case any subsequent modifications to the security group will also fail. When the patch is applied the DB entries must be corrected to resume normal operations.

[Robert Clark (robert-clark)]

Description:
HP Cloud Services reported a vulnerability in Nova API handling. When a security group is created via the EC2 or OS API's that uses a protocol defined in the incorrect case i.e 'TCP' rather than 'tcp' it causes a later string comparison to fail. This leads to Security Groups not being set correctly.

Once the Nova DB has been polluted with the incorrect case any subsequent modifications to the security group will also fail. When the patch is applied the DB entries must be corrected to resume normal operations.

[Robert Clark (robert-clark)]

I've knocked together a little SQL that the Nova guys are checking for me now, I'll add that to the disclosure when it's ready.

@Vish - can we get a decision on the patch please?

[Robert Clark (robert-clark)]

This SQL appears to work for our deployment:

UPDATE security_group_rules SET protocol = 'tcp' where protocol = 'TCP'
UPDATE security_group_rules SET protocol = 'udp' where protocol = 'UDP'
UPDATE security_group_rules SET protocol = 'icmp' where protocol = 'ICMP'

[Russell Bryant (russellb)]

Presumably there could be entries in the database as 'Tcp', 'tCp', 'tcP', etc., as well.

[Russell Bryant (russellb)]

I also wonder if we should classify this as 'Medium' or 'Low' instead of 'High' since this can't really be exploited. It doesn't allow any privilege escalation and doesn't interfere with service for other tenants as far as I can tell.

[Robert Clark (robert-clark)]

Hmm, you're right on both counts of course.

As Security Groups are the primary network facing security control for Nova tenants I'd suggest that this should be a medium.

As for the SQL, is there a more elegant solution than the rather obvious ~35 line SQL statement to cover all the possibilities for the protocol casing - I'm no expert on MySQL, I can see there's a LOWER('str') function, perhaps something like this would work...
UPDATE security_group_rules SET protocol = LOWER(protocol);

I've asked one of the guys on our Nova team to validate.

[Phil Day (philip-day)]

I think it does interfere with other tennats - Nova applies all IPtables rules in one operation, including those that haven't been changed. The basic approach is:

- Save iptables
- Modify the ones that need changing in this saved file
- Reapply the new settings

If a bug like this causes the reapply to fail part way though it could leave other VMs in other groups on the same host exposed.

Phil

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Russell Bryant
Sent: 24 April 2012 15:23
To: Day, Phil
Subject: [Bug 985184] Re: Security groups fail to be set correctly if incorrect case is used for protocol specification

I also wonder if we should classify this as 'Medium' or 'Low' instead of 'High' since this can't really be exploited. It doesn't allow any privilege escalation and doesn't interfere with service for other tenants as far as I can tell.

--
You received this bug notification because you are subscribed to the bug report.
https://bugs.launchpad.net/bugs/985184

Title:
  Security groups fail to be set correctly if incorrect case is used for
  protocol specification

Status in OpenStack Compute (Nova):
  Triaged

Bug description:
  The high level issue is that if a security group rule is specified
  with the protocol in uppercase (e.g. TCP instead of tcp) on a system
  using the IpTablesFirewallDriver then the security group rules may
  fail to be properly applied, leading to security groups that are more
  open than specified.

  The detail of the issue is as follows (Described from the OSAPI
  perspective, but the problem also exists on EC2)

  When a security group rule is specified with the protocol in upper case it is validated (contrig/security_groups.py: _rule_args_to_dict() regardless of case but stored in the database in the supplied case:
      if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']:
                  raise exception.InvalidIpProtocol(protocol=ip_protocol)

    …

    values['protocol'] = ip_protocol

  When the security group refresh is triggered (virt/firewall.py – instance_rules() the protocol check is case sensitive:

                  if protocol in ['udp', 'tcp']:
                      args += self._build_tcp_udp_rule(rule, version)
                  elif protocol == 'icmp':
                      args += self._build_icmp_rule(rule, version)
                  if rule.cidr:
                      LOG.info('Using cidr %r', rule.cidr)
                      args += ['-s', rule.cidr]
                      fw_rules += [' '.join(args)]

  Because the protocol doesn’t match ‘udp’ or ‘tcp’ the protocol part of
  the rule is skipped, leading to an incomplete and invalid iptables
  command line.

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/985184/+subscriptions

[Robert Clark (robert-clark)]

Ok, so I missed Vish's patch, Phil just pointed out that with his fix, changes to the DB aren't required.

This would still leave flaky values in the DB. I guess I'd like to see a fix where:

A) Data enters the DB in the correct state (this makes late queries and as-yet unwritten code - simpler).
B) Security Group code is robust to bad data currently in the DB
C) Advice for people who wish to clean the DB

Thoughts?

[Phil Day (philip-day)]

Sounds good to me

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Robert Clark
Sent: 24 April 2012 17:06
To: Day, Phil
Subject: [Bug 985184] Re: Security groups fail to be set correctly if incorrect case is used for protocol specification

Ok, so I missed Vish's patch, Phil just pointed out that with his fix, changes to the DB aren't required.

This would still leave flaky values in the DB. I guess I'd like to see a fix where:

A) Data enters the DB in the correct state (this makes late queries and as-yet unwritten code - simpler).
B) Security Group code is robust to bad data currently in the DB
C) Advice for people who wish to clean the DB

Thoughts?

--
You received this bug notification because you are subscribed to the bug report.
https://bugs.launchpad.net/bugs/985184

Title:
  Security groups fail to be set correctly if incorrect case is used for
  protocol specification

Status in OpenStack Compute (Nova):
  Triaged

Bug description:
  The high level issue is that if a security group rule is specified
  with the protocol in uppercase (e.g. TCP instead of tcp) on a system
  using the IpTablesFirewallDriver then the security group rules may
  fail to be properly applied, leading to security groups that are more
  open than specified.

  The detail of the issue is as follows (Described from the OSAPI
  perspective, but the problem also exists on EC2)

  When a security group rule is specified with the protocol in upper case it is validated (contrig/security_groups.py: _rule_args_to_dict() regardless of case but stored in the database in the supplied case:
      if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']:
                  raise exception.InvalidIpProtocol(protocol=ip_protocol)

    …

    values['protocol'] = ip_protocol

  When the security group refresh is triggered (virt/firewall.py – instance_rules() the protocol check is case sensitive:

                  if protocol in ['udp', 'tcp']:
                      args += self._build_tcp_udp_rule(rule, version)
                  elif protocol == 'icmp':
                      args += self._build_icmp_rule(rule, version)
                  if rule.cidr:
                      LOG.info('Using cidr %r', rule.cidr)
                      args += ['-s', rule.cidr]
                      fw_rules += [' '.join(args)]

  Because the protocol doesn’t match ‘udp’ or ‘tcp’ the protocol part of
  the rule is skipped, leading to an incomplete and invalid iptables
  command line.

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/985184/+subscriptions

[Vish Ishaya (vishvananda)]

attached diff lowercasing the rule on entering the db and on creating of iptables rules. This should be sufficient, but we can also include instructions on cleaning potentially bad rules out of the db as well.

[Thierry Carrez (ttx)]

Agreed, the advisory should mention how to sanitize existing values in the DB using SQL.

[Thierry Carrez (ttx)]

@Rob: are you handling the process on this one ? If yes, you should:
- seek pre-approval of Vish's patch by asking subscribing select nova-core reviewers and ask them to +1 the attached patch
- define CRD
- once that's done, send the advance notification to downstream stakeholders (Russell should have the up-to-date list)

See whole process at: http://wiki.openstack.org/VulnerabilityManagement

[Robert Clark (robert-clark)]

@ttx - I want to drive this forward, I'll talk to some Nova guys today.

[Robert Clark (robert-clark)]

@soren, @tr3buchet

Can you take a look at this patch and approve if you're happy please?

[Robert Clark (robert-clark)]

@markmc @dprince

Can you take a look at this patch and approve if you're happy please?

[Russell Bryant (russellb)]

Patch looks good to me, +2

[Mark McLoughlin (markmc)]

Vish's patch looks good to me.

I wouldn't bother with instructions on how to fix DB entries in the advisory. It will give the impression it is needed, but Vish's patch means it isn't needed (i.e. we handle DB entries in any case just fine)

As follow ups on master, it might be nice to add a migration and a unit test for this, but neither or required IMHO. The migration wouldn't be backported to essex/diablo anyway.

[Thierry Carrez (ttx)]

@Rob: You have enough preapproval for the patch. You can now define a CRD and push the description to downstream stakeholders. You can ask Russell for up-to-date list.

[Russell Bryant (russellb)]

Latest stakeholders list should be in your inbox.

[Robert Clark (robert-clark)]

Thanks Russell.

So I still think that we should encourage downstream to correct their databases, otherwise this could be something that comes back to bite people later on when there's another code change or something else gets added.

Opinions?

[Russell Bryant (russellb)]

As long as the advisory is incredibly clear that this is an optional step for the paranoid and not actually required, I'm ok with it. :-)

[Thierry Carrez (ttx)]

+1, the devil is in the wording though.

[Robert Clark (robert-clark)]

Title: Security groups fail to be set correctly

Impact: Medium

Reporter: HP Cloud Services <email address hidden>

Products: Nova

Affects: All versions

Description:
HP Cloud Services reported a vulnerability in Nova API handling. When a security group is created via the EC2 or OS API's that uses a protocol defined in the incorrect case i.e 'TCP' rather than 'tcp' it causes a later string comparison to fail. This leads to Security Groups not being set correctly. Once the Nova DB has been polluted with the incorrect case any subsequent modifications to the security group will also fail.

Proposed patch:
See attached diff. This proposed patch will be merged to Nova master and stable/diablo/essex branches on public disclosure date.

Database considerations:
The attached diff will make Nova resilient to any protocol case inconsistencies that may be in the Nova DB. Downstream stakeholders may want to consider sanitising their database by forcing all protocol entries to lower case, hardening their DB against any failures of future code that may expect the data to be lower case.

Proposed public disclosure date/time:
Suggestions ?
Please do not make the issue public (or release public patches) before this coordinated embargo date.

Regards,

--
$VMT_COORDINATOR_NAME
OpenStack Vulnerability Management Team

[Thierry Carrez (ttx)]

Looks good to me. If pushed down before wednesday, I'd set the coordinated disclosure date to Tuesday next week, 1500 UTC.

[Thierry Carrez (ttx)]

@Rob: was this pushed down ? Let us know if you want us to take it from there.

[Robert Clark (robert-clark)]

Had major connectivity issues the last few days.

Will email downstream with this tomorrow.

What should the release date be ?

[Thierry Carrez (ttx)]

Hmm, Thursday is a bit short (especially with Monday being a holiday in some parts of the world), Friday is usually a bad idea... Monday is always an issue... I'd say next Tuesday, 1500 UTC.

[Robert Clark (robert-clark)]

Tuesday is also a public holiday in the UK, perhaps Wednesday?

[Thierry Carrez (ttx)]

Sure! The rule is a minimum of 4 business days, and avoid Mondays and Fridays, as well as obvious holidays.

[Thierry Carrez (ttx)]

Sent downstream with proposed public disclosure date/time:
Wednesday 6th June 1000 UTC

[Robert Clark (robert-clark)]

We've had no requests to delay the public disclosure.

The next step is to get the fix pushed to gerrit, how do I arrange that?

[Thierry Carrez (ttx)]

You can follow http://wiki.openstack.org/GerritJenkinsGithub -- alternatively I can push the change to Gerrit for you.

[Russell Bryant (russellb)]

Rob, I can push it too. Please let me know if you'd like me to do so. A security advisory will need to be sent out after the patches are in, too.

[Russell Bryant (russellb)]

Patches for master, stable/essex, and stable/diablo.

The stable/essex backport required no changes. I had to make additional changes to the stable/diablo patch, so that one could use some review.

[Vish Ishaya (vishvananda)]

diablo patch looks good to me.

[Russell Bryant (russellb)]

Thanks! I'll proceed with trying to get these all merged.

[Russell Bryant (russellb)]

The patches are on gerrit now, so this bug should be made public. I don't seem to have permission to change that anymore though ...

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/8238
Committed: http://github.com/openstack/nova/commit/9f9e9da777161426a6f8cb4314b78e09beac2978
Submitter: Jenkins
Branch: stable/essex

commit 9f9e9da777161426a6f8cb4314b78e09beac2978
Author: Vishvananda Ishaya <email address hidden>
Date: Wed Jun 6 13:25:04 2012 -0400

    Fix up protocol case handling for security groups.

    Fix bug 985184.

    When creating security group rules, any case for the protocol was
    accepted as input, such as TCP, Tcp, tcp, etc., and was stored in the
    database as specified. However, unless specified as all lowercase, the
    code to apply the rules would break and result in some rules not being
    applied.

    (cherry picked from commit ff06c7c885dc94ed7c828e8cdbb8b5d850a7e654)

    Change-Id: If737104f492aacd3688f04d78eb9bc993ffa33fc

[Russell Bryant (russellb)]

Advisory sent to the list.

[Russell Bryant (russellb)]

It looks like this patch caused a regression: https://bugs.launchpad.net/nova/+bug/1010514

[Thierry Carrez (ttx)]

stable/diablo under review @ https://review.openstack.org/#/c/8239/

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/8239
Committed: http://github.com/openstack/nova/commit/2ecbf5cc0042f5a9378678811fde007603b1ee45
Submitter: Jenkins
Branch: stable/diablo

commit 2ecbf5cc0042f5a9378678811fde007603b1ee45
Author: Vishvananda Ishaya <email address hidden>
Date: Wed Jun 6 13:25:04 2012 -0400

    Fix up protocol case handling for security groups.

    Fix bug 985184.

    When creating security group rules, any case for the protocol was
    accepted as input, such as TCP, Tcp, tcp, etc., and was stored in the
    database as specified. However, unless specified as all lowercase, the
    code to apply the rules would break and result in some rules not being
    applied.

    (cherry picked from commit ff06c7c885dc94ed7c828e8cdbb8b5d850a7e654)

    Also includes backport of thix fix:
        https://review.openstack.org/#/c/8392

    Change-Id: I36af1db29c2bd97627d614df21b5da07db29a8ab

[Steve Beattie (sbeattie)]

Dave, this was fixed for Ubuntu precise in http://www.ubuntu.com/usn/usn-1466-1/ (2012.1-0ubuntu2.2). Thanks.

[Adam Gandelman (gandelman-a)]

Please find the attached test log from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Nova has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Stable review: https://review.openstack.org/8238

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

[Adam Gandelman (gandelman-a)]

Test coverage log.

[Clint Byrum (clint-fewbar)]

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.