Re: tcpdump kills terminal by dumping RADIUS traffic

2009-12-19 Thread Stuart Henderson
On 2009-12-17, Denis Doroshenko denis.doroshe...@gmail.com wrote:
 RFC2865 WRT Class field content says the following:

   The String field is one or more octets.

So the RD_STRING is correct,

   string1-253 octets containing binary data (values 0 through
 255 decimal, inclusive).  Strings of length zero (0)
 MUST NOT be sent; omit the entire attribute instead.

and the same problem could occur with other strings. How about running
them through strvisx() instead?

Index: print-radius.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-radius.c,v
retrieving revision 1.8
diff -u -p -r1.8 print-radius.c
--- print-radius.c  23 May 2006 21:57:15 -  1.8
+++ print-radius.c  19 Dec 2009 13:04:46 -
@@ -32,7 +32,9 @@
 #include arpa/inet.h
 
 #include stdio.h
+#include stdlib.h
 #include string.h
+#include vis.h
 
 /* RADIUS support for tcpdump, Thomas Ptacek t...@enteract.com */
 
@@ -206,6 +208,7 @@ static void r_print_address(int code, in
 
 static void r_print_string(int code, int len, const u_char *data) {
char string[128];
+   char vis[128*4];
 
if(!len) {
fputs( ?, stdout);
@@ -218,7 +221,8 @@ static void r_print_string(int code, int
memset(string, 0, 128);
memcpy(string, data, len);
 
-   fprintf(stdout,  %s, string);
+   strvisx(vis, string, len, 0);
+   fprintf(stdout,  %s, vis);
 }
 
 static void r_print_hex(int code, int len, const u_char *data) {



Re: tcpdump kills terminal by dumping RADIUS traffic

2009-12-19 Thread Claudio Jeker
On Sat, Dec 19, 2009 at 01:06:02PM +, Stuart Henderson wrote:
 On 2009-12-17, Denis Doroshenko denis.doroshe...@gmail.com wrote:
  RFC2865 WRT Class field content says the following:
 
The String field is one or more octets.
 
 So the RD_STRING is correct,
 
string1-253 octets containing binary data (values 0 through
  255 decimal, inclusive).  Strings of length zero (0)
  MUST NOT be sent; omit the entire attribute instead.
 
 and the same problem could occur with other strings. How about running
 them through strvisx() instead?
 

I had the same idea. So I like this idea.

 Index: print-radius.c
 ===
 RCS file: /cvs/src/usr.sbin/tcpdump/print-radius.c,v
 retrieving revision 1.8
 diff -u -p -r1.8 print-radius.c
 --- print-radius.c23 May 2006 21:57:15 -  1.8
 +++ print-radius.c19 Dec 2009 13:04:46 -
 @@ -32,7 +32,9 @@
  #include arpa/inet.h
  
  #include stdio.h
 +#include stdlib.h
  #include string.h
 +#include vis.h
  
  /* RADIUS support for tcpdump, Thomas Ptacek t...@enteract.com */
  
 @@ -206,6 +208,7 @@ static void r_print_address(int code, in
  
  static void r_print_string(int code, int len, const u_char *data) {
   char string[128];
 + char vis[128*4];
  
   if(!len) {
   fputs( ?, stdout);
 @@ -218,7 +221,8 @@ static void r_print_string(int code, int
   memset(string, 0, 128);
   memcpy(string, data, len);
  
 - fprintf(stdout,  %s, string);
 + strvisx(vis, string, len, 0);
 + fprintf(stdout,  %s, vis);

I think this can be simplified further. The memset() and memcpy() can be
skipped since you can call strvisx() directly with data.

  }
  
  static void r_print_hex(int code, int len, const u_char *data) {
 

-- 
:wq Claudio



Re: tcpdump kills terminal by dumping RADIUS traffic

2009-12-19 Thread Denis Doroshenko
On 12/19/09, Stuart Henderson s...@spacehopper.org wrote:
 On 2009-12-17, Denis Doroshenko denis.doroshe...@gmail.com wrote:
   RFC2865 WRT Class field content says the following:
  
 The String field is one or more octets.


 So the RD_STRING is correct,

If you take STRING in RD_STRING points to string in the RFC, then
that may be the case. The RFC defines the following types:

  text  1-253 octets containing UTF-8 encoded 10646 [7]
characters.  Text of length zero (0) MUST NOT be sent;
omit the entire attribute instead.

  string1-253 octets containing binary data (values 0 through
255 decimal, inclusive).  Strings of length zero (0)
MUST NOT be sent; omit the entire attribute instead.

  address   32 bit value, most significant octet first.

  integer   32 bit unsigned value, most significant octet first.

  time  32 bit unsigned value, most significant octet first --
seconds since 00:00:00 UTC, January 1, 1970.  The
standard Attributes do not use this data type but it is
presented here for possible use in future attributes.

So there's no type that is straight printable. For me, when and
attribute type described as octets containing binary data, better be
printed as hexadecimal. But it is IMHO.

 and the same problem could occur with other strings. How about running
  them through strvisx() instead?

You are completely correct the same may occur with other strings while
they would be perfectly in accordance with the RFC.

  Index: print-radius.c
  ===
  RCS file: /cvs/src/usr.sbin/tcpdump/print-radius.c,v

 retrieving revision 1.8
  diff -u -p -r1.8 print-radius.c
  --- print-radius.c  23 May 2006 21:57:15 -  1.8

 +++ print-radius.c  19 Dec 2009 13:04:46 -
  @@ -32,7 +32,9 @@
   #include arpa/inet.h

   #include stdio.h
  +#include stdlib.h
   #include string.h
  +#include vis.h

   /* RADIUS support for tcpdump, Thomas Ptacek t...@enteract.com */

  @@ -206,6 +208,7 @@ static void r_print_address(int code, in

   static void r_print_string(int code, int len, const u_char *data) {
 char string[128];
  +   char vis[128*4];

Just curious here, why128*4? Do you try to align stack here? As per
vis(3) the buffer should be 127*4+1, isn;t it a task of a compiler to
place the variables in the stack no matter what their sizes are?

Then again, the RFC defines maximum length of the string type is 253.
Is it okay to print 127 first octets of it? May be for -v mode we
could print all of it?

 if(!len) {
 fputs( ?, stdout);
  @@ -218,7 +221,8 @@ static void r_print_string(int code, int
 memset(string, 0, 128);
 memcpy(string, data, len);

  -   fprintf(stdout,  %s, string);
  +   strvisx(vis, string, len, 0);

Do we still need that string and memset/memcpy? Why not removing
variable string and here just doing strvisx(vis, data, len, 0)?

  +   fprintf(stdout,  %s, vis);
   }

   static void r_print_hex(int code, int len, const u_char *data) {



tcpdump kills terminal by dumping RADIUS traffic

2009-12-17 Thread Denis Doroshenko
Hi!

It is happening for quite long time already, as I have to deal with
RADIUS traffic, it came to the point where I can't bear it no more.
All the traffic I see contains raw binary Class fields.

RFC2865 WRT Class field content says the following:

  The String field is one or more octets.  The actual format of the
  information is site or application specific, and a robust
  implementation SHOULD support the field as undistinguished octets.

  The codification of the range of allowed usage of this field is
  outside the scope of this specification.

Also the field is of a string type, and the type described by the same RFC as:

  string1-253 octets containing binary data (values 0 through
255 decimal, inclusive).  Strings of length zero (0)
MUST NOT be sent; omit the entire attribute instead.

From my experience with RADIUS traffic, it looks like it is an
implementation dependent identifier used to relate Access-Accept and
Accounting-Request. Thus it may have any value that is generated and
accepted by the server.

Default tcpdump has print-radius.c, which treats Class field as text
(it is like this since the very beginning dated 1997). So tcpdump
makes a copy of the value of maximum length of 127 octets and then
fprintfs it as %s. With binary data going out like this it does some
weird things to PuTTy, so that it shows all kinds of graphics symbols
instead of ASCII characters.

Does anybody experience this too?

So if the RFC says it is a binary field, may be we need to change
tcpdump to make it treat the field as hex? I found myself being unable
to  live without a little change like:

Index: print-radius.c
===
RCS file: /home/cyxob/cvs/src/usr.sbin/tcpdump/print-radius.c,v
retrieving revision 1.8
diff -u -p -r1.8 print-radius.c
--- print-radius.c  23 May 2006 21:57:15 -  1.8
+++ print-radius.c  15 Dec 2009 11:42:44 -
@@ -125,7 +125,7 @@ static struct radius_atable radius_atts[
 { RADIUS_ATT_FRAMED_ROUTE, RD_STRING,  F-Rt, { NULL } },
 { RADIUS_ATT_FRAMED_IPX,   RD_ADDRESS, F-IPX,{ NULL } },
 { RADIUS_ATT_CHALLENGE_STATE,  RD_STRING,  CState,   { NULL } },
-{ RADIUS_ATT_CLASS,RD_STRING,  Class,{ NULL } },
+{ RADIUS_ATT_CLASS,RD_HEX, Class,{ NULL } },
 { RADIUS_ATT_VENDOR_SPECIFIC,  RD_HEX, Vendor,   { NULL } },
 { RADIUS_ATT_SESSION_TIMEOUT,  RD_INT, S-TO, { NULL } },
 { RADIUS_ATT_IDLE_TIMEOUT, RD_INT, I-TO, { NULL } },