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_SHUT                    2
+#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(&bgpn, 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 += 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]");
 }

Reply via email to