VerifyReply broken when multiple instances of same attribute are not adjacent on reply

[spencermaxfield]

In packet.py, the VerifyReply function incorrectly calculates the expected authenticator when multiple values for a single attribute type are present on the reply and those attributes are not "next to" each other. For example, if a reply has two Class attributes (which is valid per the RADIUS RFC) with a Framed-Protocol attribute between them, VerifyReply will return False even though the reply is valid.

This bug was introduced by the change here to add support for the message authenticator: fbbfb74

Specifically, the change to use reply._PktEncodeAttributes() instead of rawreply[20:] is the cause of the bug. _PktEncodeAttributes() changes the order of the attributes, causing the verification of the reply to fail despite the reply being valid. The solution is to revert back to using rawreply[20:].

Here is a script that demonstrates this. It builds out a reply packet with two Class attributes that have a Framed-Protocol attribute between them (this is done "by hand" since pyrad will keep the Class attributes together).

import struct
import hashlib
from io import StringIO

from pyrad import packet, dictionary


# Create a dictionary with the attributes used in this reduction
fp = StringIO( """
ATTRIBUTE    Framed-Protocol        7    integer
ATTRIBUTE    Class            25    octets
""")
dictionary = dictionary.Dictionary(fp)
fp.close()

access_request_authenticator = b'a' * 16
identifier = 1
secret = b'secret'

# Create a request
access_request = packet.AuthPacket(secret=secret, id=identifier, authenticator=access_request_authenticator, dict=dictionary)

# Build out the raw bytes for a response to the request. Include two Class attributes.
class_key = 25
class_one_value = b'classone'
class_one_attribute = struct.pack('!BB', class_key, (len(class_one_value) + 2)) + class_one_value

class_two_value = b'classtwo'
class_two_attribute = struct.pack('!BB', class_key, (len(class_two_value) + 2)) + class_two_value

framed_protocol_key = 7
framed_protocol_value = b'\x00\x00\x00\x01'  # PPP
framed_protocol_attribute = struct.pack('!BB', framed_protocol_key, (len(framed_protocol_value) + 2)) + framed_protocol_value

# Put an attribute between the Class attributes. This part is key, and is what
# causes VerifyReply to break
attribute_value_pairs = class_one_attribute + framed_protocol_attribute + class_two_attribute

# Finish constructing the bytes for the response
response_code = packet.AccessAccept
header = struct.pack('!BBH', response_code, identifier, (20 + len(attribute_value_pairs)))
response_authenticator = hashlib.md5(header + access_request_authenticator + attribute_value_pairs + secret).digest()
raw_response = header + response_authenticator + attribute_value_pairs

# Verify that the response is valid.
response_packet = packet.AuthPacket(packet=raw_response, dict=dictionary)
valid_reply = access_request.VerifyReply(response_packet, raw_response)
print("Is the reply valid? - {}".format(valid_reply))
print(b"Attribute order on raw packet: " + attribute_value_pairs)
print(b"Attribute order from _PktEncodeAttributes: " + response_packet._PktEncodeAttributes())

Against pyrad 2.2, the output of this script is the following (note that the call to _PktEncodeAttributes changed moved the classtwo attribute value to be adjacent to the classone attribute value):

Is the reply valid? - False
b'Attribute order on raw packet: \x19\nclassone\x07\x06\x00\x00\x00\x01\x19\nclasstwo'
b'Attribute order from _PktEncodeAttributes: \x19\nclassone\x19\nclasstwo\x07\x06\x00\x00\x00\x01'

If in my script I change the ordering of attribute_value_pairs to not have the Framed-Protocol attribute between them, then the existing pyrad 2.2. code works.

# This change to the script causes the bug to not be encountered
attribute_value_pairs = class_one_attribute + class_two_attribute + framed_protocol_attribute

Applications can and do send multiple instances of an attribute with attributes between them (e.g. Microsoft's Network Policy Server does this when acting as a RADIUS server), though, so that case needs to work.

If I revert VerifyReply to use rawreply[20:] instead, then the script outputs that the reply is valid, as expected.

Is the reply valid? - True
b'Attribute order on raw packet: \x19\nclassone\x07\x06\x00\x00\x00\x01\x19\nclasstwo'
b'Attribute order from _PktEncodeAttributes: \x19\nclassone\x19\nclasstwo\x07\x06\x00\x00\x00\x01'

Additionally, VerifyReply has the following if-block:

        if rawreply is None:
            rawreply = reply.ReplyPacket()

This is also problematic because ReplyPacket() makes use of _PktEncodeAttributes. Therefore if someone calls VerifyReply with a rawreply=None (the default), they will still encounter this bug, even after no longer calling _PktEncodeAttributes directly from VerifyReply. I'm not sure if this is the best solution, but it seems to me that VerifyReply needs to be modified to only take in the rawreply.

[Istvan91]

Hi,

thanks for the detailed bug report! You're right this is indeed a problem.

This is also problematic because ReplyPacket() makes use of _PktEncodeAttributes. Therefore if someone calls VerifyReply with a rawreply=None (the default), they will still encounter this bug, even after no longer calling _PktEncodeAttributes directly from VerifyReply. I'm not sure if this is the best solution, but it seems to me that VerifyReply needs to be modified to only take in the rawreply.

I think the API itself is not good. I'll fix the function, but the function should have only taken a rawreply in the first place and return a Packet object on success, some Exception on failure.
Received packets should have a .raw_packet attribute. Technically that could be used if it exists instead of calling _PktEncodeAttributes. But then (actually already now) we've got a mess: what if the raw_packet from the function argument is different from the .raw_packet attribute?

Just taking the raw reply would be also some kind of solution, but then you'd loose the relation between the created Packet object and the raw_reply. It is not too bad, but still an (unnecessary) point of failure.