From 1a12349c056b52b488591abb1671ad94a6db6526 Mon Sep 17 00:00:00 2001 From: Ahmad Hassan Date: Fri, 30 Sep 2011 15:10:33 +0100 Subject: [PATCH] Verify security group parameters Introduced various sanity checks before adding security group rule into the database. The checks have been implemented both in EC2 and openstack extension code. Implemented the suggestions made in first patch by Brian Fixed the unit tests in security groups Fixed pep8 issues in security group unit tests Fixes bug 869979. Change-Id: I2ac28666e90e7bdeacb7b1c2676c0719cfb9e441 --- nova/api/ec2/cloud.py | 44 ++++++++++++++--- nova/api/openstack/contrib/security_groups.py | 42 ++++++++++++---- nova/exception.py | 2 +- .../openstack/contrib/test_security_groups.py | 41 ++++++++++++++++ nova/tests/test_api.py | 48 ++++++++++++++++++- nova/utils.py | 21 ++++++++ 6 files changed, 181 insertions(+), 17 deletions(-) diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py index 393df287025a..dcf185cf142e 100644 --- a/nova/api/ec2/cloud.py +++ b/nova/api/ec2/cloud.py @@ -23,7 +23,6 @@ datastore. """ import base64 -import netaddr import os import re import shutil @@ -727,22 +726,53 @@ class CloudController(object): elif cidr_ip: # If this fails, it throws an exception. This is what we want. cidr_ip = urllib.unquote(cidr_ip).decode() - netaddr.IPNetwork(cidr_ip) + + if not utils.is_valid_cidr(cidr_ip): + # Raise exception for non-valid address + raise exception.InvalidCidr(cidr=cidr_ip) + values['cidr'] = cidr_ip else: values['cidr'] = '0.0.0.0/0' if ip_protocol and from_port and to_port: - from_port = int(from_port) - to_port = int(to_port) + ip_protocol = str(ip_protocol) + try: + # Verify integer conversions + from_port = int(from_port) + to_port = int(to_port) + except ValueError: + if ip_protocol.upper() == 'ICMP': + raise exception.InvalidInput(reason="Type and" + " Code must be integers for ICMP protocol type") + else: + raise exception.InvalidInput(reason="To and From ports " + "must be integers") if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']: raise exception.InvalidIpProtocol(protocol=ip_protocol) - if ((min(from_port, to_port) < -1) or - (max(from_port, to_port) > 65535)): + + # Verify that from_port must always be less than + # or equal to to_port + if from_port > to_port: raise exception.InvalidPortRange(from_port=from_port, - to_port=to_port) + to_port=to_port, msg="Former value cannot" + " be greater than the later") + + # Verify valid TCP, UDP port ranges + if (ip_protocol.upper() in ['TCP', 'UDP'] and + (from_port < 1 or to_port > 65535)): + raise exception.InvalidPortRange(from_port=from_port, + to_port=to_port, msg="Valid TCP ports should" + " be between 1-65535") + + # Verify ICMP type and code + if (ip_protocol.upper() == "ICMP" and + (from_port < -1 or to_port > 255)): + raise exception.InvalidPortRange(from_port=from_port, + to_port=to_port, msg="For ICMP, the" + " type:code must be valid") values['protocol'] = ip_protocol values['from_port'] = from_port diff --git a/nova/api/openstack/contrib/security_groups.py b/nova/api/openstack/contrib/security_groups.py index 9072a34eeb9c..bb4cd48b2e76 100644 --- a/nova/api/openstack/contrib/security_groups.py +++ b/nova/api/openstack/contrib/security_groups.py @@ -15,7 +15,6 @@ """The security groups extension.""" -import netaddr import urllib from webob import exc import webob @@ -26,6 +25,7 @@ from nova import exception from nova import flags from nova import log as logging from nova import rpc +from nova import utils from nova.api.openstack import common from nova.api.openstack import extensions from nova.api.openstack import wsgi @@ -270,28 +270,54 @@ class SecurityGroupRulesController(SecurityGroupController): # If this fails, it throws an exception. This is what we want. try: cidr = urllib.unquote(cidr).decode() - netaddr.IPNetwork(cidr) except Exception: raise exception.InvalidCidr(cidr=cidr) + + if not utils.is_valid_cidr(cidr): + # Raise exception for non-valid address + raise exception.InvalidCidr(cidr=cidr) + values['cidr'] = cidr else: values['cidr'] = '0.0.0.0/0' if ip_protocol and from_port and to_port: + ip_protocol = str(ip_protocol) try: from_port = int(from_port) to_port = int(to_port) except ValueError: - raise exception.InvalidPortRange(from_port=from_port, - to_port=to_port) - ip_protocol = str(ip_protocol) + if ip_protocol.upper() == 'ICMP': + raise exception.InvalidInput(reason="Type and" + " Code must be integers for ICMP protocol type") + else: + raise exception.InvalidInput(reason="To and From ports " + "must be integers") + if ip_protocol.upper() not in ['TCP', 'UDP', 'ICMP']: raise exception.InvalidIpProtocol(protocol=ip_protocol) - if ((min(from_port, to_port) < -1) or - (max(from_port, to_port) > 65535)): + + # Verify that from_port must always be less than + # or equal to to_port + if from_port > to_port: raise exception.InvalidPortRange(from_port=from_port, - to_port=to_port) + to_port=to_port, msg="Former value cannot" + " be greater than the later") + + # Verify valid TCP, UDP port ranges + if (ip_protocol.upper() in ['TCP', 'UDP'] and + (from_port < 1 or to_port > 65535)): + raise exception.InvalidPortRange(from_port=from_port, + to_port=to_port, msg="Valid TCP ports should" + " be between 1-65535") + + # Verify ICMP type and code + if (ip_protocol.upper() == "ICMP" and + (from_port < -1 or to_port > 255)): + raise exception.InvalidPortRange(from_port=from_port, + to_port=to_port, msg="For ICMP, the" + " type:code must be valid") values['protocol'] = ip_protocol values['from_port'] = from_port diff --git a/nova/exception.py b/nova/exception.py index 129dc775f695..5877dfb74de1 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -229,7 +229,7 @@ class InvalidVolumeType(Invalid): class InvalidPortRange(Invalid): - message = _("Invalid port range %(from_port)s:%(to_port)s.") + message = _("Invalid port range %(from_port)s:%(to_port)s. %(msg)s") class InvalidIpProtocol(Invalid): diff --git a/nova/tests/api/openstack/contrib/test_security_groups.py b/nova/tests/api/openstack/contrib/test_security_groups.py index f55ce4a55f2f..b3e1507e0415 100644 --- a/nova/tests/api/openstack/contrib/test_security_groups.py +++ b/nova/tests/api/openstack/contrib/test_security_groups.py @@ -532,6 +532,47 @@ class TestSecurityGroupRules(test.TestCase): self.assertNotEquals(security_group_rule['id'], 0) self.assertEquals(security_group_rule['parent_group_id'], 2) + def test_create_by_invalid_cidr_json(self): + rules = { + "security_group_rule": { + "ip_protocol": "tcp", + "from_port": "22", + "to_port": "22", + "parent_group_id": 2, + "cidr": "10.2.3.124/2433"}} + rule = security_group_rule_template( + ip_protocol="tcp", + from_port=22, + to_port=22, + parent_group_id=2, + cidr="10.2.3.124/2433") + req = fakes.HTTPRequest.blank('/v1.1/123/os-security-group-rules') + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create, + req, {'security_group_rule': rule}) + + def test_create_by_invalid_tcp_port_json(self): + rule = security_group_rule_template( + ip_protocol="tcp", + from_port=75534, + to_port=22, + parent_group_id=2, + cidr="10.2.3.124/24") + + req = fakes.HTTPRequest.blank('/v1.1/123/os-security-group-rules') + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create, + req, {'security_group_rule': rule}) + + def test_create_by_invalid_icmp_port_json(self): + rule = security_group_rule_template( + ip_protocol="icmp", + from_port=1, + to_port=256, + parent_group_id=2, + cidr="10.2.3.124/24") + req = fakes.HTTPRequest.blank('/v1.1/123/os-security-group-rules') + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create, + req, {'security_group_rule': rule}) + def test_create_add_existing_rules(self): rule = security_group_rule_template(cidr='10.0.0.0/24') diff --git a/nova/tests/test_api.py b/nova/tests/test_api.py index e9f1145ddeb4..2d3d4b6049aa 100644 --- a/nova/tests/test_api.py +++ b/nova/tests/test_api.py @@ -386,6 +386,50 @@ class ApiEc2TestCase(test.TestCase): group.connection = self.ec2 group.authorize('tcp', 80, 81, '0.0.0.0/0') + group.authorize('icmp', -1, -1, '0.0.0.0/0') + group.authorize('udp', 80, 81, '0.0.0.0/0') + # Invalid CIDR address + self.assertRaises(Exception, + group.authorize, 'tcp', 80, 81, '0.0.0.0/0444') + # Missing ports + self.assertRaises(Exception, + group.authorize, 'tcp', '0.0.0.0/0') + # from port cannot be greater than to port + self.assertRaises(Exception, + group.authorize, 'tcp', 100, 1, '0.0.0.0/0') + # For tcp, negative values are not allowed + self.assertRaises(Exception, + group.authorize, 'tcp', -1, 1, '0.0.0.0/0') + # For tcp, valid port range 1-65535 + self.assertRaises(Exception, + group.authorize, 'tcp', 1, 65599, '0.0.0.0/0') + # For icmp, only -1:-1 is allowed for type:code + self.assertRaises(Exception, + group.authorize, 'icmp', -1, 0, '0.0.0.0/0') + # Non valid type:code + self.assertRaises(Exception, + group.authorize, 'icmp', 0, 3, '0.0.0.0/0') + # Invalid Cidr for ICMP type + self.assertRaises(Exception, + group.authorize, 'icmp', -1, -1, '0.0.444.0/4') + # Invalid protocol + self.assertRaises(Exception, + group.authorize, 'xyz', 1, 14, '0.0.0.0/0') + # Invalid port + self.assertRaises(Exception, + group.authorize, 'tcp', " ", "81", '0.0.0.0/0') + # Invalid icmp port + self.assertRaises(Exception, + group.authorize, 'icmp', " ", "81", '0.0.0.0/0') + # Invalid CIDR Address + self.assertRaises(Exception, + group.authorize, 'icmp', -1, -1, '0.0.0.0') + # Invalid CIDR Address + self.assertRaises(Exception, + group.authorize, 'icmp', -1, -1, '0.0.0.0/') + # Invalid Cidr ports + self.assertRaises(Exception, + group.authorize, 'icmp', 1, 256, '0.0.0.0/0') self.expect_http() self.mox.ReplayAll() @@ -394,7 +438,7 @@ class ApiEc2TestCase(test.TestCase): group = [grp for grp in rv if grp.name == security_group_name][0] - self.assertEquals(len(group.rules), 1) + self.assertEquals(len(group.rules), 3) self.assertEquals(int(group.rules[0].from_port), 80) self.assertEquals(int(group.rules[0].to_port), 81) self.assertEquals(len(group.rules[0].grants), 1) @@ -405,6 +449,8 @@ class ApiEc2TestCase(test.TestCase): group.connection = self.ec2 group.revoke('tcp', 80, 81, '0.0.0.0/0') + group.revoke('icmp', -1, -1, '0.0.0.0/0') + group.revoke('udp', 80, 81, '0.0.0.0/0') self.expect_http() self.mox.ReplayAll() diff --git a/nova/utils.py b/nova/utils.py index a30d90ff104e..ad0d5725d8ba 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -37,6 +37,7 @@ import time import types import uuid import pyclbr +import netaddr from xml.sax import saxutils from eventlet import event @@ -908,6 +909,26 @@ def is_valid_ipv4(address): return True +def is_valid_cidr(address): + """Check if the provided ipv4 or ipv6 address is a valid + CIDR address or not""" + try: + # Validate the correct CIDR Address + netaddr.IPNetwork(address) + except netaddr.core.AddrFormatError: + return False + + # Prior validation partially verify /xx part + # Verify it here + ip_segment = address.split('/') + + if (len(ip_segment) <= 1 or + ip_segment[1] == ''): + return False + + return True + + def monkey_patch(): """ If the Flags.monkey_patch set as True, this function patches a decorator