Repository: faucetsdn/chewie

Issue:
If Message-Authenticator doesn't exist we should still check the request_authenticator
https://github.com/faucetsdn/chewie/blob/master/chewie/radius.py#L151


correct logic should be:
```
if message_authenticator:
    validate message_authenticator
if request_authenticator:
    validate request_authenticator
```

In other words, if Message-Authenticator doesn't exist we still check the request_authenticator.

Discussion:

Patch:
diff --git a/chewie/radius.py b/chewie/radius.py
index 31370e5..bded60e 100644
--- a/chewie/radius.py
+++ b/chewie/radius.py
@@ -52,25 +52,29 @@ class Radius:
             MessageParseError: if packed_message cannot be parsed
         """
         try:
-            code, packet_id, length, response_authenticator = struct.unpack("!BBH16s",
-                                                                            packed_message[:RADIUS_HEADER_LENGTH])
+            code, packet_id, length, authenticator = struct.unpack("!BBH16s",
+                                                                   packed_message[:RADIUS_HEADER_LENGTH])
         except struct.error as exception:
             raise MessageParseError('Unable to unpack first 20 bytes of RADIUS header') \
                 from exception
 
-        response_authenticator = binascii.hexlify(response_authenticator)
         if code in PACKET_TYPE_PARSERS.keys():
-            radius_packet = PACKET_TYPE_PARSERS[code](packet_id, response_authenticator,
+            radius_packet = PACKET_TYPE_PARSERS[code](packet_id, authenticator,
                                                       RadiusAttributesList.parse(
                                                           packed_message[RADIUS_HEADER_LENGTH:]))
-            try:
-                request_authenticator = radius_lifecycle.packet_id_to_request_authenticator[packet_id]
-            except KeyError as exception:
-                raise MessageParseError('Unknown RAIDUS packet_id: %s' % packet_id,) \
-                    from exception
+            if code == Radius.ACCESS_REQUEST:
+                request_authenticator = authenticator
+            else:
+                try:
+                    request_authenticator = radius_lifecycle.packet_id_to_request_authenticator[
+                        packet_id]
+                except KeyError as exception:
+                    raise MessageParseError('Unknown RAIDUS packet_id: %s' % packet_id, ) \
+                        from exception
             try:
                 return radius_packet.validate_packet(secret,
-                                                     request_authenticator=request_authenticator)
+                                                     request_authenticator=request_authenticator,
+                                                     code=code)
             except (InvalidMessageAuthenticatorError,
                     InvalidResponseAuthenticatorError) as exception:
                 raise MessageParseError("Unable to parse Radius packet") \
@@ -129,13 +133,14 @@ class RadiusPacket(Radius):
                                               .digest())
 
             for i in range(16):
-                self.packed[i+position] = message_authenticator[i]
+                self.packed[i + position] = message_authenticator[i]
         return self.packed
 
-    def validate_packet(self, secret, request_authenticator=None):
+    def validate_packet(self, secret, request_authenticator=None, code=None):
         """Calculates the Response Authenticator (in Radius Header) and
         MessageAuthenticator (a Radius Attribute) hashes and compares with what was provided.
         Args:
+            code (int): The RADIUS Code (e.g. Access-Challenge)
             secret (str): secret shared between RADIUS and chewie.
             request_authenticator (): the original request authenticator for this
              packet (which is a response)
@@ -146,42 +151,55 @@ class RadiusPacket(Radius):
         """
         # Copy this packet so we can modify the 'Authenticator' and 'Message-Authenticator'
         radius_packet = copy.deepcopy(self)
-        # get the Original Message Authenticator
 
-        message_authenticator = radius_packet.attributes.find(MessageAuthenticator.DESCRIPTION)
-        if not message_authenticator or not request_authenticator:
-            # no validation required.
-            return self
         if not secret:
             raise ValueError("secret cannot be None for hashing")
 
-        response_authenticator = radius_packet.authenticator
-        radius_packet.authenticator = request_authenticator
-        radius_packet.pack()
-        calculated_response_authenticator = binascii.hexlify(hashlib.md5(radius_packet.packed +
-                                                             bytearray(secret, 'utf-8')).digest())
-        if calculated_response_authenticator != response_authenticator:
-            raise InvalidResponseAuthenticatorError(
-                "Original ResponseAuthenticator: '%s', does not match calculated: '%s' %s" % (
-                    response_authenticator,
-                    calculated_response_authenticator,
-                    binascii.hexlify(radius_packet.packed)))
-
-        original_ma = message_authenticator.data_type.bytes_data
-        # Replace the Original Message Authenticator
-        message_authenticator.data_type.bytes_data = bytes.fromhex("00000000000000000000000000000000")
-        radius_packet.pack()
-
-        # calculate new hash message authenticator
-        new_ma = hmac.new(secret.encode(), radius_packet.packed, 'md5').digest()
-
-        # compare old and new message authenticator
-        if original_ma != new_ma:
-            raise InvalidMessageAuthenticatorError(
-                "Original Message-Authenticator: '%s', does not match calculated: '%s'" %
-                (binascii.hexlify(original_ma), binascii.hexlify(new_ma)))
+        self.validate_response_authenticator(radius_packet, request_authenticator, secret, code)
+
+        self.validate_message_authenticator(radius_packet, secret, request_authenticator)
         return self
 
+    @staticmethod
+    def validate_response_authenticator(radius_packet, request_authenticator, secret, code):
+        if request_authenticator and code in [Radius.ACCESS_REJECT,
+                                              Radius.ACCESS_ACCEPT,
+                                              Radius.ACCESS_CHALLENGE]:
+            response_authenticator = radius_packet.authenticator
+            radius_packet.authenticator = request_authenticator
+            radius_packet.pack()
+            calculated_response_authenticator = hashlib.md5(radius_packet.packed +
+                                                            bytearray(secret, 'utf-8')).digest()
+            if calculated_response_authenticator != response_authenticator:
+                raise InvalidResponseAuthenticatorError(
+                    "Original ResponseAuthenticator: '%s', does not match calculated: '%s' %s" % (
+                        response_authenticator,
+                        calculated_response_authenticator,
+                        binascii.hexlify(radius_packet.packed)))
+
+    @staticmethod
+    def validate_message_authenticator(radius_packet, secret, request_authenticator):
+        message_authenticator = radius_packet.attributes.find(MessageAuthenticator.DESCRIPTION)
+        if message_authenticator:
+
+            radius_packet.authenticator = request_authenticator
+
+            original_ma = message_authenticator.data_type.bytes_data
+            # Replace the Original Message Authenticator
+            message_authenticator.data_type.bytes_data = bytes.fromhex(
+                "00000000000000000000000000000000")
+
+            radius_packet.pack()
+
+            # calculate new hash message authenticator
+            new_ma = hmac.new(secret.encode(), radius_packet.packed, 'md5').digest()
+
+            # compare old and new message authenticator
+            if original_ma != new_ma:
+                raise InvalidMessageAuthenticatorError(
+                    "Original Message-Authenticator: '%s', does not match calculated: '%s'" %
+                    (binascii.hexlify(original_ma), binascii.hexlify(new_ma)))
+
 
 @register_packet_type_parser
 class RadiusAccessRequest(RadiusPacket):
@@ -283,7 +301,7 @@ class RadiusAttributesList:
                 type_, attr_length = struct.unpack("!BB",
                                                    attributes_data[pos:pos + Attribute.HEADER_SIZE])
             except struct.error as exception:
-                raise MessageParseError('Unable to unpack first 2 bytes of attribute header')\
+                raise MessageParseError('Unable to unpack first 2 bytes of attribute header') \
                     from exception
             data = attributes_data[pos + Attribute.HEADER_SIZE: pos + attr_length]
             pos += attr_length


Test patch:
diff --git a/test/test_radius.py b/test/test_radius.py
index 73b5fdb..bc52864 100644
--- a/test/test_radius.py
+++ b/test/test_radius.py
@@ -23,7 +23,7 @@ class RadiusTestCase(unittest.TestCase):
         message = Radius.parse(packed_message, secret="SECRET",
                                radius_lifecycle=namedtuple('RadiusLifecycle', 'packet_id_to_request_authenticator')({0: None}))
         self.assertEqual(message.packet_id, 0)
-        self.assertEqual(message.authenticator, b"982a0ba06d3557f0dbc8ba6e823822f1")
+        self.assertEqual(binascii.hexlify(message.authenticator), b"982a0ba06d3557f0dbc8ba6e823822f1")
         msg_attr = message.attributes
         self.assertEqual(len(msg_attr.attributes), 10)
         self.assertEqual(msg_attr.find(UserName.DESCRIPTION).data_type.data(), 'host1user')
@@ -52,7 +52,7 @@ class RadiusTestCase(unittest.TestCase):
                                  1: bytes.fromhex("a0b4ace0b367114b1a16d76e2bfed5d8")
                                }))
         self.assertEqual(message.packet_id, 1)
-        self.assertEqual(message.authenticator, b"02970aff2ef0700780f70848e90d2410")
+        self.assertEqual(binascii.hexlify(message.authenticator), b"02970aff2ef0700780f70848e90d2410")
         msg_attr = message.attributes
         self.assertEqual(len(msg_attr.attributes), 4)
         eap_msg = msg_attr.find(EAPMessage.DESCRIPTION).data_type.data()
@@ -136,7 +136,7 @@ class RadiusTestCase(unittest.TestCase):
                                  0: bytes.fromhex("982a0ba06d3557f0dbc8ba6e823822f1")
                                }))
         self.assertEqual(message.packet_id, 0)
-        self.assertEqual(message.authenticator, b"56d9280d3e4fed327eb31cf1823f8c24")
+        self.assertEqual(binascii.hexlify(message.authenticator), b"56d9280d3e4fed327eb31cf1823f8c24")
         msg_attr = message.attributes
         self.assertEqual(len(msg_attr.attributes), 3)
         eap_msg = msg_attr.find(EAPMessage.DESCRIPTION).data_type.data()
@@ -158,7 +158,7 @@ class RadiusTestCase(unittest.TestCase):
                                  6: bytes.fromhex("0d64ffb8bc76d457d337e5f5692534aa")
                                }))
         self.assertEqual(message.packet_id, 6)
-        self.assertEqual(message.authenticator, b"54dbc73332c00c0347fc4b462d1811a7")
+        self.assertEqual(binascii.hexlify(message.authenticator), b"54dbc73332c00c0347fc4b462d1811a7")
         msg_attr = message.attributes
         self.assertEqual(len(msg_attr.attributes), 3)
         eap_msg = msg_attr.find(EAPMessage.DESCRIPTION).data_type.data()