[Emanuele Caratti <wiz@xxxxxx>]

On Sat, Sep 20, 2003 at 02:44:09AM -0700, Guy Harris wrote:

> > I've still to complete the accounting part,
> Presumably that's why it gets a
> 
> 	packet-tacacs.h:417: warning: `tacplus_acct_flags' defined but not used
> 
> warning - the accounting part dissector will presumably use that
> variable.

Exactly :) May be I can keep that variable in my cvs version until I
complete the accounting part...

> I just checked in an additional change to arrange that the buffer
> allocated for the decrypted data be freed when the tvbuff to which it
> refers is freed (along the lines of what other decrypting dissectors
> do).

Ok, I found also a leek in md5_xor... It's possible to leave the function
without freeing md5_buff.

Attached you'll find the merge of my latest version and the one in current CVS tree. 

I've replaced the buff=g_malloc + tvb_get_nstringz0 with buff=tvb_get_string.

Now the md5_xor function doesn't return value and use the CLEANUP call

Different RAS have different password with the same tacacs+ server, so it's
possible to have the need of different tacpluskey.... 
Where can I look for some hint about implementing this feature ?
It's possible to have a kind of popup to add the password to the tcp stream ?


-- 
Ciao,
    Emanuele

--- mytacacs/packet-tacacs.c	2003-09-21 11:06:24.000000000 +0200
+++ /.1/cvs-mirror/ethereal/packet-tacacs.c	2003-09-21 09:41:25.000000000 +0200
@@ -47,7 +47,7 @@
 #include "crypt-md5.h"
 #include "packet-tacacs.h"
 
-static void md5_xor( u_char *data, char *key, int data_len, u_char *session_id, u_char version, u_char seq_no );
+static int md5_xor( u_char *data, char *key, int data_len, guint32 session_id, u_char version, u_char seq_no );
 
 static int proto_tacacs = -1;
 static int hf_tacacs_version = -1;
@@ -329,16 +329,15 @@
 tacplus_tvb_setup( tvbuff_t *tvb, tvbuff_t **dst_tvb, packet_info *pinfo, guint32 len, guint8 version )
 {
 	guint8	*buff;
-	u_char session_id[4];
-
-/* session_id is in NETWORK Byte Order, and is used as byte array in the md5_xor */
-
-	tvb_memcpy(tvb, (guint8*)session_id, 4,4); 
+	guint32 tmp_sess;
+	
+	buff=g_malloc(len);
 
-	buff = tvb_memdup(tvb, TAC_PLUS_HDR_SIZE, len);
 
+	tvb_memcpy(tvb,(guint8*)&tmp_sess,4,4);
+	tvb_memcpy(tvb, buff, TAC_PLUS_HDR_SIZE, len);
 
-	md5_xor( buff, tacplus_key, len, session_id,version, tvb_get_guint8(tvb,2) );
+	md5_xor( buff, tacplus_key, len, tmp_sess,version, tvb_get_guint8(tvb,2) );
 
 	/* Allocate a new tvbuff, referring to the decrypted data. */
 	*dst_tvb = tvb_new_real_data( buff, len, len );
@@ -556,7 +555,8 @@
 	val=tvb_get_ntohs( tvb, AUTHEN_C_USER_LEN_OFF ); 
 	proto_tree_add_text( tree, tvb, AUTHEN_C_USER_LEN_OFF, 2 , "User length: %d", val );
 	if( val ){
-		buff=tvb_get_string( tvb, var_off, val );
+		buff=g_malloc(val+1);
+		tvb_get_nstringz0( tvb, var_off, val+1, buff );
 		proto_tree_add_text( tree, tvb, var_off, val, "User: %s", buff );
 		var_off+=val;
 		g_free(buff);
@@ -594,7 +594,8 @@
 	proto_tree_add_text( tree, tvb, AUTHEN_R_SRV_MSG_LEN_OFF, 2 ,
 				"Server message length: %d", val );
 	if( val ) {
-		buff=tvb_get_string(tvb, var_off, val );
+		buff=g_malloc(val+1);
+		tvb_get_nstringz0(tvb, var_off, val+1, buff);
 		proto_tree_add_text(tree, tvb, var_off, val, "Server message: %s", buff );
 		var_off+=val;
 		g_free(buff);
@@ -703,7 +704,8 @@
 	proto_tree_add_text( tree, tvb, ACCT_R_SRV_MSG_LEN_OFF, 2 ,
 				"Server message length: %d", val );
 	if( val ) {
-		buff=tvb_get_string( tvb, var_off, val );
+		buff=(guint8*)g_malloc(val+1);
+		tvb_get_nstringz0( tvb, var_off, val+1, buff );
 		proto_tree_add_text( tree, tvb, var_off,
 				val, "Server message: %s", buff );
 		var_off+=val;
@@ -715,7 +717,8 @@
 	proto_tree_add_text( tree, tvb, ACCT_R_DATA_LEN_OFF, 2 ,
 				"Data length: %d", val );
 	if( val ) {
-		buff= tvb_get_string( tvb, var_off, val );
+		buff=(guint8*)g_malloc(val+1);
+		tvb_get_nstringz0( tvb, var_off, val+1, buff );
 		proto_tree_add_text( tree, tvb, var_off,
 				val, "Data: %s", buff );
 		g_free(buff);
@@ -914,8 +917,9 @@
 	proto_register_field_array(proto_tacplus, hf, array_length(hf));
 	proto_register_subtree_array(ett, array_length(ett));
 	tacplus_module = prefs_register_protocol (proto_tacplus, NULL);
-	prefs_register_string_preference ( tacplus_module, "key",
-	"TACACS+ Encryption Key", "TACACS+ Encryption Key", &tacplus_key );
+	prefs_register_string_preference(tacplus_module, "key",
+	    "TACACS+ Encryption Key", "TACACS+ Encryption Key",
+	    &tacplus_key);
 }
 
 void
@@ -928,11 +932,10 @@
 	dissector_add("tcp.port", TCP_PORT_TACACS, tacplus_handle);
 }
 
-
 #define MD5_LEN 16
-
-static void
-md5_xor( u_char *data, char *key, int data_len, u_char *session_id, u_char version, u_char seq_no )
+	
+static int
+md5_xor( u_char *data, char *key, int data_len, guint32 session_id, u_char version, u_char seq_no )
 {
 	int i,j,md5_len;
 	md5_byte_t *md5_buff;
@@ -940,21 +943,20 @@
 	md5_byte_t *mdp;
 	md5_state_t mdcontext;
 
-	md5_len = 4 /* sizeof(session_id) */ + strlen(key)
+	md5_len = sizeof(session_id) + strlen(key)
 			+ sizeof(version) + sizeof(seq_no);
 	
 	md5_buff = (md5_byte_t*)g_malloc(md5_len+MD5_LEN);
 
-	CLEANUP_PUSH( g_free, md5_buff );
-
 	mdp = md5_buff;
-	*(guint32*)mdp = *(guint32*)session_id;
-	mdp += 4 ;
+	bcopy(&session_id, mdp, sizeof(session_id));
+	mdp += sizeof(session_id);
 	bcopy(key, mdp, strlen(key));
 	mdp += strlen(key);
-	*mdp++ = version;
-	*mdp++ = seq_no;
-
+	bcopy(&version, mdp, sizeof(version));
+	mdp += sizeof(version);
+	bcopy(&seq_no, mdp, sizeof(seq_no));
+	mdp += sizeof(seq_no);
 
 	md5_init(&mdcontext);
 	md5_append(&mdcontext, md5_buff, md5_len);
@@ -963,10 +965,8 @@
 	for (i = 0; i < data_len; i += 16) {
 
 		for (j = 0; j < 16; j++) {
-			if ((i + j) >= data_len)  {
-				i = data_len+1; /* To exit from the external loop  */
-				break;
-			}
+			if ((i + j) >= data_len) 
+				return 0;
 			data[i + j] ^= hash[j];
 		}
 		bcopy( hash, mdp , MD5_LEN );
@@ -974,5 +974,6 @@
 		md5_append(&mdcontext, md5_buff, md5_len);
 		md5_finish(&mdcontext,hash);
 	}
-	CLEANUP_CALL_AND_POP;
+	g_free( md5_buff );
+	return 0;
 }