[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freeradius-devel
Subject:    tunnel attributes bug
From:       Takahiro Wagatsuma <waga () sic ! shibaura-it ! ac ! jp>
Date:       2002-01-25 9:41:21
[Download RAW message or body]

Hello,

I had met a little problem which drops the first octets of
tunnel attributes string.
  NG  Tunnel-Client-Endpoint:0 = "02.18.126.159"
  OK  Tunnel-Client-Endpoint:0 = "202.18.126.159"

And I have found a bug in rad_send() and rad_decode().

According to RFC2868, the TAG for tunnel attributes with integer value
are defined 0x01 through 0x1F, or others must be zero.
The TAG for tunnel attributes with string value are defined 0x01
through 0x1F, or others must recognized as the first octet of string.

In case of rad_send(), if the pair is PW_TYPE_STRING and has invalid tag,
then TAG is set to 0x00.
But receiver ( or client) recognize it as INVALID TAG and recognize it
as the first octet string. which is the string "\0.....".
I think if the TAG is invalid, rad_send() should ignore and set TAG as
the first octet of string.

In case of rad_decode(), if the type of attribute is string and has 
invalid tag, it should recognize as the first octet of string.
And also if the attribute is "Tunnel-Password", and has invalid tag,
the TAG should be ingored.


-3.1.  Tunnel-Type
-3.2.  Tunnel-Medium-Type
-3.8.  Tunnel-Preference
-
-   Tag
-      The Tag field is one octet in length and is intended to provide a
-      means of grouping attributes in the same packet which refer to the
-      same tunnel.  Valid values for this field are 0x01 through 0x1F,
-      inclusive.  If the Tag field is unused, it MUST be zero (0x00).


-3.3.  Tunnel-Client-Endpoint
-3.4.  Tunnel-Server-Endpoint
-3.6.  Tunnel-Private-Group-ID
-3.7.  Tunnel-Assignment-ID
-3.9.  Tunnel-Client-Auth-ID
-3.10.  Tunnel-Server-Auth-ID
-
-   Tag
-      The Tag field is one octet in length and is intended to provide a
-      means of grouping attributes in the same packet which refer to the
-      same tunnel.  If the value of the Tag field is greater than 0x00
-      and less than or equal to 0x1F, it SHOULD be interpreted as
-      indicating which tunnel (of several alternatives) this attribute
-      pertains.  If the Tag field is greater than 0x1F, it SHOULD be
-      interpreted as the first byte of the following String field.


-3.5.  Tunnel-Password
-
-   Tag
-      The Tag field is one octet in length and is intended to provide a
-      means of grouping attributes in the same packet which refer to the
-      same tunnel.  Valid values for this field are 0x01 through 0x1F,
-      inclusive.  If the value of the Tag field is greater than 0x00 and
-      less than or equal to 0x1F, it SHOULD be interpreted as indicating
-      which tunnel (of several alternatives) this attribute pertains;
-      otherwise, the Tag field SHOULD be ignored.



--- radius.c.orig	Fri Jan 25 17:33:38 2002
+++ radius.c	Fri Jan 25 17:46:48 2002
@@ -315,22 +315,20 @@
 
 				  /*
 				   *    Set the TAG at the beginning of the string if tagged.
 				   *    If tag value is not valid for tagged attribute, make 
 				   *    it 0x00 per RFC 2868.  -cparker
 				   */
 				  if(reply->flags.has_tag) {
 				          len++;
 					  if(TAG_VALID(reply->flags.tag)) {
 					          *ptr++ = reply->flags.tag;
-					  } else {
-					          *ptr++ = 0x00;
 					  }
 				  }
 				 
 				  /*
 				   *	Ensure we don't go too far.
 				   *	The 'length' of the attribute
 				   *	may be 0..255, minus whatever
 				   *	octets are used in the attribute
 				   *	header.
 				   */
@@ -1096,12 +1094,25 @@
 				pair->length = strlen(pair->strvalue);
 			} else if (pair->flags.has_tag &&
 				   pair->type == PW_TYPE_STRING) {
-			        if(TAG_VALID(*ptr)) 
-				       pair->flags.tag = *ptr;
-				else
+				if(TAG_VALID(*ptr)) {
+				       pair->flags.tag = *ptr++;
+					pair->length--;
+				} else if(pair->flags.encrypt == 2) {
+					/*
+					 * from RFC2868 - 3.5.  Tunnel-Password
+					 * If the value of the Tag field is greater than
+					 * 0x00 and less than or equal to 0x1F, it SHOULD
+					 * be interpreted as indicating which tunnel
+					 * (of several alternatives) this attribute pertains;
+					 * otherwise, the Tag field SHOULD be ignored.
+					 */
+					pair->flags.tag = 0x00;
+					ptr++;
+					pair->length--;
+				} else {
 				       pair->flags.tag = 0x00;
-				pair->length--;
-				memcpy(pair->strvalue, ptr + 1, 
+				}
+				memcpy(pair->strvalue, ptr, 
 				       pair->length);
 			} else {
 				/* attrlen always < MAX_STRING_LEN */
--- end of patch

- 
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
[prev in list] [next in list] [prev in thread] [next in thread]