Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-19 Thread Damien Miller
On Wed, 19 Apr 2017, Job Snijders wrote:

> The realisation that a shutdown communication may contain \0 (since NUL is a
> valid UTF-8 char)

\0 isn't a valid UTF-8 character. UTF-8 sets the MSB on code points > 127:
https://en.wikipedia.org/wiki/UTF-8#Description




Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-19 Thread Theo de Raadt
> The realisation that a shutdown communication may contain \0 (since NUL is a
> valid UTF-8 char), led me to alter the proposed changes. A debugging tool like
> tcpdump should display trash too. This 0003 patch avoids the memset/memcpy and
> can deal with trash in the shutdown communication through a new util 
> putlchars().

What kind of trash do you mean?  Any kind of trash?

How about an xterm set-title ANSI sequence, or something worse.

Personally, I think this should use vis.



Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-19 Thread Job Snijders
On Mon, Apr 17, 2017 at 01:56:17PM -0600, Theo de Raadt wrote:
> +   memset(string, 0, 129);
> +   memcpy(string, p+1, shutdown_comm_length);
> +   safeputs(string);
> 
> Please don't copy numbers like that. If this is a string, why not use
> string functions that gaurantee truncation and truncation detection...

The payload of the shutdown communication (which starts at p+1) is not a
C string: the content of the field wich is memcpy'd, is not
null-terminated. Instead the field's size is available through
'shutdown_comm_length'.

The realisation that a shutdown communication may contain \0 (since NUL is a
valid UTF-8 char), led me to alter the proposed changes. A debugging tool like
tcpdump should display trash too. This 0003 patch avoids the memset/memcpy and
can deal with trash in the shutdown communication through a new util 
putlchars().

example output: 

BGP (NOTIFICATION: error Cease, subcode #2, Shutdown Communication 
(length: 52): "This is a test of the sh\000\000\000\000wn communication 
system.") (DF) [tos 0xc0] (ttl 255, id 40416, len 126)

Kind regards,

Job


diff --git a/usr.sbin/tcpdump/print-bgp.c b/usr.sbin/tcpdump/print-bgp.c
index d028d671893..e25fdbd930a 100644
--- a/usr.sbin/tcpdump/print-bgp.c
+++ b/usr.sbin/tcpdump/print-bgp.c
@@ -228,9 +228,13 @@ static const char *bgpnotify_minor_update[] = {
 
 /* RFC 4486 */
 #define BGP_NOTIFY_MINOR_CEASE_MAXPRFX  1
+/* draft-ietf-idr-shutdown-07 */
+#define BGP_NOTIFY_MINOR_CEASE_SHUT2
+#define BGP_NOTIFY_MINOR_CEASE_RESET   4
+#define BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN  128
 static const char *bgpnotify_minor_cease[] = {
-   NULL, "Maximum Number of Prefixes Reached", "Administratively Shutdown",
-   "Peer De-configured", "Administratively Reset", "Connection Rejected",
+   NULL, "Maximum Number of Prefixes Reached", "Administrative Shutdown",
+   "Peer De-configured", "Administrative Reset", "Connection Rejected",
"Other Configuration Change", "Connection Collision Resolution",
"Out of Resources",
 };
@@ -302,6 +306,21 @@ static const char *afnumber[] = AFNUM_NAME_STR;
sizeof(afnumber)/sizeof(afnumber[0]), (x)))
 
 
+static void
+print_hex(const u_char *p, u_int len)
+{
+   while (len--)
+   printf("%02x", *p++);
+}
+
+static void
+putlchars(const u_char *str, u_int len)
+{
+   while (len--)
+   safeputchar(*str++);
+}
+
+
 static const char *
 num_or_str(const char **table, size_t siz, int value)
 {
@@ -996,6 +1015,8 @@ bgp_notification_print(const u_char *dat, int length)
u_int16_t af;
u_int8_t safi;
const u_char *p;
+   uint8_t shutdown_comm_length;
+   uint8_t remainder_offset;
 
TCHECK2(dat[0], BGP_NOTIFICATION_SIZE);
memcpy(, dat, BGP_NOTIFICATION_SIZE);
@@ -1026,9 +1047,54 @@ bgp_notification_print(const u_char *dat, int length)
 
printf(" Max Prefixes: %u", EXTRACT_32BITS(p+3));
}
+
+   /*
+* draft-ietf-idr-shutdown describes a method to send a
+* message intended for human consumption regarding the
+* Administrative Shutdown or Reset event. This is called
+* the "Shutdown Communication". The communication is
+* UTF-8 encoded and may be no longer than 128 bytes.
+*/
+
+   if ((bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_SHUT ||
+   bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_RESET) &&
+   (length >= BGP_NOTIFICATION_SIZE + 1)) {
+   p = dat + BGP_NOTIFICATION_SIZE;
+   TCHECK2(*p, 1);
+   shutdown_comm_length = *(p);
+   remainder_offset = 0;
+   /* if we received garbage, make sure we hexdump it all 
*/
+   if (shutdown_comm_length >
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN ||
+   shutdown_comm_length > (length - 
BGP_NOTIFICATION_SIZE) + 1)
+   printf(", invalid Shutdown Communication 
length");
+   else if (shutdown_comm_length == 0) {
+   printf(", empty Shutdown Communication");
+   remainder_offset += 1;
+   }
+   /* a proper shutdown communication */
+   else {
+   TCHECK2(*(p+1), shutdown_comm_length);
+   printf(", Shutdown Communication (length: %u): 
\"",
+   shutdown_comm_length);
+   putlchars(p+1, shutdown_comm_length);
+   printf("\"");
+   remainder_offset += 

Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-17 Thread Theo de Raadt
+   memset(string, 0, 129);
+   memcpy(string, p+1, shutdown_comm_length);
+   safeputs(string);

Please don't copy numbers like that.  If this is a string, why not
use string functions that gaurantee truncation and truncation detection...



Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-17 Thread Job Snijders
Hi all,

Daan Keuper (Computest) was kind enough to review the diff, he pointed
out the following:

safeputs() expects a null-terminated string. Since shutdown_comm_length
won't exceed BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN (128), the
following will ensure a null-terminated string is passed to safeputs().

> + char string[128];
^^^
> + memset(string, 0, 128);
  ^^^
> + memcpy(string, p+1, shutdown_comm_length);
> + safeputs(string);

128 should be 129. oops!

I'd like to defer to more experienced programmers on the aesthetics of
using "129" rather then "BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN + 1",
or perhaps even an entirely other approach to ensure the string is
null-terminated.

Also added an additional check "shutdown_comm_length > (length -
BGP_NOTIFICATION_SIZE)" which is useful to prevent in case the shutdown
communication claims to be longer then the BGP message actually is.

Kind regards,

Job


diff --git a/usr.sbin/tcpdump/print-bgp.c b/usr.sbin/tcpdump/print-bgp.c
index d028d671893..0886304926e 100644
--- a/usr.sbin/tcpdump/print-bgp.c
+++ b/usr.sbin/tcpdump/print-bgp.c
@@ -228,9 +228,13 @@ static const char *bgpnotify_minor_update[] = {
 
 /* RFC 4486 */
 #define BGP_NOTIFY_MINOR_CEASE_MAXPRFX  1
+/* draft-ietf-idr-shutdown-07 */
+#define BGP_NOTIFY_MINOR_CEASE_SHUT2
+#define BGP_NOTIFY_MINOR_CEASE_RESET   4
+#define BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN  128
 static const char *bgpnotify_minor_cease[] = {
-   NULL, "Maximum Number of Prefixes Reached", "Administratively Shutdown",
-   "Peer De-configured", "Administratively Reset", "Connection Rejected",
+   NULL, "Maximum Number of Prefixes Reached", "Administrative Shutdown",
+   "Peer De-configured", "Administrative Reset", "Connection Rejected",
"Other Configuration Change", "Connection Collision Resolution",
"Out of Resources",
 };
@@ -302,6 +306,13 @@ static const char *afnumber[] = AFNUM_NAME_STR;
sizeof(afnumber)/sizeof(afnumber[0]), (x)))
 
 
+static void
+print_hex(const u_char *p, u_int len)
+{
+   while (len--)
+   printf("%02x", *p++);
+}
+
 static const char *
 num_or_str(const char **table, size_t siz, int value)
 {
@@ -996,6 +1007,9 @@ bgp_notification_print(const u_char *dat, int length)
u_int16_t af;
u_int8_t safi;
const u_char *p;
+   uint8_t shutdown_comm_length;
+   uint8_t remainder_offset;
+   char string[129];
 
TCHECK2(dat[0], BGP_NOTIFICATION_SIZE);
memcpy(, dat, BGP_NOTIFICATION_SIZE);
@@ -1026,9 +1040,56 @@ bgp_notification_print(const u_char *dat, int length)
 
printf(" Max Prefixes: %u", EXTRACT_32BITS(p+3));
}
+
+   /*
+* draft-ietf-idr-shutdown describes a method to send a
+* message intended for human consumption regarding the
+* Administrative Shutdown or Reset event. This is called
+* the "Shutdown Communication". The communication is
+* UTF-8 encoded and may be no longer than 128 bytes.
+*/
+
+   if ((bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_SHUT ||
+   bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_RESET) &&
+   (length >= BGP_NOTIFICATION_SIZE + 1)) {
+   p = dat + BGP_NOTIFICATION_SIZE;
+   TCHECK2(*p, 1);
+   shutdown_comm_length = *(p);
+   remainder_offset = 0;
+   /* seems we received garbage, hexdump it all */
+   if (shutdown_comm_length >
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN ||
+   shutdown_comm_length > (length - 
BGP_NOTIFICATION_SIZE))
+   printf(", invalid Shutdown Communication 
length");
+   else if (shutdown_comm_length == 0) {
+   printf(", empty Shutdown Communication");
+   remainder_offset += 1;
+   }
+   /* a proper shutdown communication */
+   else {
+   TCHECK2(*(p+1), shutdown_comm_length);
+   printf(", Shutdown Communication (length: %u): 
\"",
+   shutdown_comm_length);
+   memset(string, 0, 129);
+   memcpy(string, p+1, shutdown_comm_length);
+   safeputs(string);
+   printf("\"");
+   remainder_offset += shutdown_comm_length + 1;
+   }
+   /* if there is trailing data, hexdump it */
+

Re: tcpdump: decode BGP Administrative Shutdown Communication

2017-04-17 Thread Job Snijders
Hi OpenBSD,

bgpd(8) as shipped in OpenBSD 6.1 supports draft-ietf-idr-shutdown-07.
The below patch adds support to tcpdump(8) to decode such shutdown
communication.

This is an improved version of the patch proposal I sent in January.

Kind regards,

Job



diff --git a/usr.sbin/tcpdump/print-bgp.c b/usr.sbin/tcpdump/print-bgp.c
index d028d671893..2871b92909f 100644
--- a/usr.sbin/tcpdump/print-bgp.c
+++ b/usr.sbin/tcpdump/print-bgp.c
@@ -228,9 +228,13 @@ static const char *bgpnotify_minor_update[] = {
 
 /* RFC 4486 */
 #define BGP_NOTIFY_MINOR_CEASE_MAXPRFX  1
+/* draft-ietf-idr-shutdown-07 */
+#define BGP_NOTIFY_MINOR_CEASE_SHUT2
+#define BGP_NOTIFY_MINOR_CEASE_RESET   4
+#define BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN  128
 static const char *bgpnotify_minor_cease[] = {
-   NULL, "Maximum Number of Prefixes Reached", "Administratively Shutdown",
-   "Peer De-configured", "Administratively Reset", "Connection Rejected",
+   NULL, "Maximum Number of Prefixes Reached", "Administrative Shutdown",
+   "Peer De-configured", "Administrative Reset", "Connection Rejected",
"Other Configuration Change", "Connection Collision Resolution",
"Out of Resources",
 };
@@ -302,6 +306,13 @@ static const char *afnumber[] = AFNUM_NAME_STR;
sizeof(afnumber)/sizeof(afnumber[0]), (x)))
 
 
+static void
+print_hex(const u_char *p, u_int len)
+{
+   while (len--)
+   printf("%02x", *p++);
+}
+
 static const char *
 num_or_str(const char **table, size_t siz, int value)
 {
@@ -996,6 +1007,9 @@ bgp_notification_print(const u_char *dat, int length)
u_int16_t af;
u_int8_t safi;
const u_char *p;
+   uint8_t shutdown_comm_length;
+   uint8_t remainder_offset;
+   char string[128];
 
TCHECK2(dat[0], BGP_NOTIFICATION_SIZE);
memcpy(, dat, BGP_NOTIFICATION_SIZE);
@@ -1026,9 +1040,55 @@ bgp_notification_print(const u_char *dat, int length)
 
printf(" Max Prefixes: %u", EXTRACT_32BITS(p+3));
}
+
+   /*
+* draft-ietf-idr-shutdown describes a method to send a
+* message intended for human consumption regarding the
+* Administrative Shutdown or Reset event. This is called
+* the "Shutdown Communication". The communication is
+* UTF-8 encoded and may be no longer than 128 bytes.
+*/
+
+   if ((bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_SHUT ||
+   bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_RESET) &&
+   (length >= BGP_NOTIFICATION_SIZE + 1)) {
+   p = dat + BGP_NOTIFICATION_SIZE;
+   TCHECK2(*p, 1);
+   shutdown_comm_length = *(p);
+   remainder_offset = 0;
+   /* seems we received garbage, hexdump it all */
+   if (shutdown_comm_length >
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN)
+   printf(", invalid Shutdown Communication 
length");
+   else if (shutdown_comm_length == 0) {
+   printf(", empty Shutdown Communication");
+   remainder_offset += 1;
+   }
+   /* a proper shutdown communication */
+   else {
+   TCHECK2(*(p+1), shutdown_comm_length);
+   printf(", Shutdown Communication (length: %u): 
\"",
+   shutdown_comm_length);
+   memset(string, 0, 128);
+   memcpy(string, p+1, shutdown_comm_length);
+   safeputs(string);
+   printf("\"");
+   remainder_offset += shutdown_comm_length + 1;
+   }
+   /* if there is trailing data, hexdump it */
+   if (length -
+   (remainder_offset + BGP_NOTIFICATION_SIZE) > 0) {
+   printf(", Data: (length: %u) ",
+   length - (remainder_offset +
+   BGP_NOTIFICATION_SIZE));
+   print_hex(p + remainder_offset, length -
+   (remainder_offset + BGP_NOTIFICATION_SIZE));
+   }
+   }
}
 
return;
+
 trunc:
printf("[|BGP]");
 }



tcpdump: decode BGP Administrative Shutdown Communication

2017-01-07 Thread Job Snijders
This patch adds support to tcpdump(8) to decode BGP Administrative
Shutdown Communications in human readable form.

The draft-ietf-idr-shutdown 
(https://tools.ietf.org/html/draft-ietf-idr-shutdown)
specification documents a mechanism to transmit a short freeform UTF-8
message as part of a BGP Cease NOTIFICATION message to inform the
neighbor why the BGP session is being shutdown.

This modified tcpdump(8) is used in the development of the
draft-ietf-idr-shutdown feature for OpenBGPD.

example, snipped from 'tcpdump -t -n -v -s 1500 host 165.254.255.17 and port 
179':

BGP (NOTIFICATION: error Cease, subcode #2, Shutdown Communication
(len 124): "NTT will perform maintenance on this router. This is
tracked in TICKET-1-24824294. Contact n...@ntt.net for more
information.")

UTF-8 stuff like 濾 is printed like this:

BGP (NOTIFICATION: error Cease, subcode #2, Shutdown Communication
(len 26): "and here is a unicorn \360\237\246\204")

Index: usr.sbin/tcpdump/print-bgp.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-bgp.c,v
retrieving revision 1.20
diff -u -p -r1.20 print-bgp.c
--- usr.sbin/tcpdump/print-bgp.c27 Oct 2016 08:21:58 -  1.20
+++ usr.sbin/tcpdump/print-bgp.c7 Jan 2017 11:15:01 -
@@ -228,6 +228,9 @@ static const char *bgpnotify_minor_updat
 
 /* RFC 4486 */
 #define BGP_NOTIFY_MINOR_CEASE_MAXPRFX  1
+/* draft-ietf-idr-shutdown */
+#define BGP_NOTIFY_MINOR_CEASE_ADMIN   2
+#define BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN  128
 static const char *bgpnotify_minor_cease[] = {
NULL, "Maximum Number of Prefixes Reached", "Administratively Shutdown",
"Peer De-configured", "Administratively Reset", "Connection Rejected",
@@ -302,6 +305,13 @@ static const char *afnumber[] = AFNUM_NA
sizeof(afnumber)/sizeof(afnumber[0]), (x)))
 
 
+static void
+print_hex(const u_char *p, u_int len)
+{
+   while (len--)
+   printf("%02x", *p++);
+}
+
 static const char *
 num_or_str(const char **table, size_t siz, int value)
 {
@@ -996,6 +1006,9 @@ bgp_notification_print(const u_char *dat
u_int16_t af;
u_int8_t safi;
const u_char *p;
+   uint8_t shutdown_comm_length;
+   uint8_t remainder_offset;
+   char string[128];
 
TCHECK2(dat[0], BGP_NOTIFICATION_SIZE);
memcpy(, dat, BGP_NOTIFICATION_SIZE);
@@ -1026,9 +1039,57 @@ bgp_notification_print(const u_char *dat
 
printf(" Max Prefixes: %u", EXTRACT_32BITS(p+3));
}
+
+   /*
+* draft-ietf-idr-shutdown describes a method to send a
+* message intended for human consumption regarding the
+* Administrative Shutdown event. This is called the
+* "Administrative Shutdown Communication". The message
+* is UTF-8 encoded and may be no longer than 128 bytes.
+*/
+
+   if (bgpn.bgpn_minor == BGP_NOTIFY_MINOR_CEASE_ADMIN &&
+   length >= 1) {
+   p = dat + BGP_NOTIFICATION_SIZE;
+   TCHECK2(*p, 1);
+   shutdown_comm_length = *(p);
+   remainder_offset = 0;
+   /* seems we received garbage, hexdump it all */
+   if (shutdown_comm_length >
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN)
+   printf(", invalid Shutdown Communication (len 
%i > %i)",
+   shutdown_comm_length,
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN);
+   else if (shutdown_comm_length == 0) {
+   printf(", empty Shutdown Communication");
+   remainder_offset += 1;
+   }
+   /* a proper shutdown communication */
+   else if (shutdown_comm_length <=
+   BGP_NOTIFY_MINOR_CEASE_ADMIN_SHUTDOWN_LEN) {
+   TCHECK2(*p, shutdown_comm_length);
+   printf(", Shutdown Communication (len %u): \"",
+   shutdown_comm_length);
+   memset(string, 0, 128);
+   memcpy(string, p+1, shutdown_comm_length);
+   safeputs(string);
+   printf("\"");
+   remainder_offset += shutdown_comm_length + 1;
+   }
+   /* if there is trailing data, hexdump it */
+   if (length -
+   (remainder_offset + BGP_NOTIFICATION_SIZE) > 0) {
+   printf(", Data: (len %u) ",
+   length - (remainder_offset +
+