Merge "Verify security group parameters"

6 files changed, 181 insertions, 17 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index de9b9e660..a2f9ecdae 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, 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)
+                      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 9072a34ee..bb4cd48b2 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, 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)
+                      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 d749d89a0..bf277f53a 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 f55ce4a55..b3e1507e0 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 e9f1145dd..2d3d4b604 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 a30d90ff1..ad0d5725d 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