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

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

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'

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

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.

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

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?

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.

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.

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.

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?

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'

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

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.

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.

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

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?

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

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.

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

@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

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

@soren, @tr3buchet

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

@markmc @dprince

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

Patch looks good to me, +2

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.

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

Latest stakeholders list should be in your inbox.

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?

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. :-)

+1, the devil is in the wording though.

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

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

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

Had major connectivity issues the last few days.

Will email downstream with this tomorrow.

What should the release date be ?

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.

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

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

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

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?

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

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.

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.

diablo patch looks good to me.

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

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 ...

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

Advisory sent to the list.

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

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

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

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

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.

Test coverage log.

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.