#4 encodePapPassword fix and Message-Authenticator feature

open
nobody
None
5
2012-08-14
2012-08-14
No

This patch addresses several problems with the tinyradius library. The version of tinyradius that it bases its changes against is the copy of TinyRadius-1.0.tar.gz available at https://sourceforge.net/projects/tinyradius/, and that is the version to which the line numbers pertain in the descriptions that follow.

(1) The old version of encodePapPassword in AccessRequest.java produced an incorrect answer whenever the input userPass byte array had a length at least 17. Refer to http://tools.ietf.org/html/rfc2865#section-5.2 for the following description.

The incorrect version of the method was effectively performing the operation:

b1 = MD5(S + RA) c(1) = p1 xor b1
b2 = MD5(S + p1) c(2) = p2 xor b2
b3 = MD5(S + p2) c(3) = p3 xor b3
. .
. .
. .
bi = MD5(S + p(i-1)) c(i) = pi xor bi

My version performs the correct operation:

b1 = MD5(S + RA) c(1) = p1 xor b1
b2 = MD5(S + c(1)) c(2) = p2 xor b2
b3 = MD5(S + c(2)) c(3) = p3 xor b3
. .
. .
. .
bi = MD5(S + c(i-1)) c(i) = pi xor bi

(2) The old version of decodePapPassword would throw a NullPointerException in the process of printing the log message and a RadiusException would not be thrown as was intended (see line 258 of AccessRequest.java). Also, the old version of decodePapPassword would throw an ArrayIndexOutOfBoundsException on line 275 of AccessRequest.java if the input encryptedPass did not have a length that was divisible by 16, and my new version throws a RadiusException with a useful message in this case.

(3) Although the tinyradius library defines the Message-Authenticator attribute (introduced in http://tools.ietf.org/html/rfc2869#section-5.14\) on line 72 of dictionary/default_dictionary, the existing RadiusPacket does not support the ability to properly add a Message-Authenticator attribute to an Access-Request packet, i.e. to "sign" the packet. Here is an explanation. The Message-Authenticator attribute is calculated as HMAC-MD5 (Type, Identifier, Length, Request Authenticator, Attributes). The important thing is that the Request Authenticator (the authenticator field of RadiusPacket) is calculated using random bytes (see line 768 of RadiusPacket.java), so each time the authenticator field of RadiusPacket is assigned the result of createRequestAuthenticator(sharedSecret), it gets a different value. Now, the only publicly exposed way to calculate the Request Authenticator is by calling RadiusPacket.encodeRequestPacket(ByteArrayOutputStream out, String sharedSecret). So certainly we can add the Message-Authenticator attribute after making this call because the Request Authenticator has been calculated and the attributes (USER_PASSWORD or CHAP_PASSWORD) have been encoded. But here's the problem: if we make another call to the existing RadiusPacket.encodeRequestPacket, the Request Authenticator authenticator will be set to a new value on line 724 of RadiusPacket.java, and now the Message-Authenticator that was calculated using the previous value of authenticator is no longer valid when the packet is written to the second ByteArrayOutputStream!

So the only way to fix this without breaking existing client code is by adding the ability to sign the packet (add a valid Message-Authenticator) attribute immediately after setting authenticator and encoding the passwords for a request packet, and immediately before setting authenticator for a response packet. I have added the public methods encodeRequestPacket(ByteArrayOutputStream out, String sharedSecret, boolean signPacket) and encodeResponsePacket(ByteArrayOutputStream out, String sharedSecret, RadiusPacket request, boolean signPacket) to RadiusPacket, and the existing versions of these methods without the boolean parameter simply call the new ones with signPacket = false, ensuring the functionality of existing code remains the same.

(4) It is important for a Radius server (resp. client) requiring the Message-Authenticator attribute to not only check that the attribute is present on a request packet (resp. response packet), but to make sure it is correct. So in RadiusPacket I have added the public methods hasValidMessageAuthenticator(String sharedSecret) (for calling when this packet is a request) and hasValidMessageAuthenticator(String sharedSecret, RadiusPacket request) (for calling whenthis packet is a response). An important issue that arises here is that the old implementation of AccessRequest.encodeRequestAttributes(String sharedSecret) removed the password attribute from the middle of the ArrayList of attributes and then added it again at the end. This means that if we tried to re-encode in hasValidMessageAuthenticator we would change the order of the attributes from what they were when the and then have no hope of havingbuf in hasValidMessageAuthenticator(String sharedSecret, RadiusPacket request) hold the correct value. So I changed AccessRequest.encodeRequestAttributes(String sharedSecret) to add the encoded password attribute at the end of the list if it does not already exist, but just to use the RadiusAttribute.setAttributeData method to modify the existing attribute in the list if is already there. This does not have the potential to break anything because
(i) a Radius server or client MUST NOT have any dependencies on the order of attributes, as per http://tools.ietf.org/html/rfc2865#section-5.
(ii) a valid Access-Request packet can only contain at most one of a USER_PASSWORD and CHAP_PASSWORD, as per http://tools.ietf.org/html/rfc2865#section-5.44.
So my new implementation of AccessRequest.encodeRequestAttributes(String sharedSecret) performs the same as the old one except that in the case such an attribute already exists on the packet the attribute will not be moved to the end of the list.

Thanks for reviewing!

--Jesse Short-Gershman

Discussion