2003-04-22 Havoc Pennington <hp@redhat.com>

	* test/data/valid-messages/opposite-endian.message: fix test
	to use proper type for rply field

        * test/data/invalid-messages: add tests for below validation

	* dbus/dbus-message.c (decode_header_data): validate field types,
	and validate that named fields are valid names
	(decode_name_field): consider messages in the
	org.freedesktop.Local. namespace to be invalid.

	* dbus/dbus-string.c (_dbus_string_validate_name): new

12 files changed, 319 insertions, 89 deletions

diff --git a/ChangeLog b/ChangeLog
index d9a2f6cb..7818e3f9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2003-04-22  Havoc Pennington  <hp@redhat.com>
+
+	* test/data/valid-messages/opposite-endian.message: fix test
+	to use proper type for rply field
+
+        * test/data/invalid-messages: add tests for below validation
+	
+	* dbus/dbus-message.c (decode_header_data): validate field types,
+	and validate that named fields are valid names
+	(decode_name_field): consider messages in the
+	org.freedesktop.Local. namespace to be invalid.
+
+	* dbus/dbus-string.c (_dbus_string_validate_name): new
+
 2003-04-19  Havoc Pennington  <hp@pobox.com>
 
 	* bus/driver.c (bus_driver_handle_hello): check limits and 
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 7a89da3e..eb331662 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -267,7 +267,7 @@ _dbus_connection_queue_received_message_link (DBusConnection  *connection,
   _dbus_list_append_link (&connection->incoming_messages,
                           link);
   message = link->data;
-  
+
   /* If this is a reply we're waiting on, remove timeout for it */
   reply_serial = dbus_message_get_reply_serial (message);
   if (reply_serial != -1)
diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index ee42947d..af949fb4 100644
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -2910,8 +2910,9 @@ append_array_type (DBusMessageRealIter *real,
       existing_element_type = iter_get_array_type (real, array_type_pos);
       if (existing_element_type != element_type)
 	{
-	  _dbus_warn ("Appending array of %d, when expecting array of %d\n",
-		      element_type, existing_element_type);
+	  _dbus_warn ("Appending array of %s, when expecting array of %s\n",
+		      _dbus_type_to_string (element_type),
+                      _dbus_type_to_string (existing_element_type));
 	  _dbus_string_set_length (&real->message->body, real->pos);
 	  return FALSE;
 	}
@@ -3627,7 +3628,76 @@ _dbus_message_loader_get_buffer (DBusMessageLoader  *loader,
 #define DBUS_HEADER_FIELD_SENDER_AS_UINT32  \
   FOUR_CHARS_TO_UINT32 ('s', 'n', 'd', 'r')
 
-/* FIXME impose max length on name, srvc, sndr */
+static dbus_bool_t
+decode_string_field (const DBusString   *data,
+                     HeaderField         fields[FIELD_LAST],
+                     int                 pos,
+                     int                 type,
+                     int                 field,
+                     const char         *field_name)
+{
+  DBusString tmp;
+  int string_data_pos;
+  
+  if (fields[field].offset >= 0)
+    {
+      _dbus_verbose ("%s field provided twice\n",
+                     field_name);
+      return FALSE;
+    }
+
+  if (type != DBUS_TYPE_STRING)
+    {
+      _dbus_verbose ("%s field has wrong type\n", field_name);
+      return FALSE;
+    }
+
+  /* skip padding after typecode, skip string length;
+   * we assume that the string arg has already been validated
+   * for sanity and UTF-8
+   */
+  string_data_pos = _DBUS_ALIGN_VALUE (pos, 4) + 4;
+  _dbus_assert (string_data_pos < _dbus_string_get_length (data));
+  
+  _dbus_string_init_const (&tmp,
+                           _dbus_string_get_const_data (data) + string_data_pos);
+
+  if (field == FIELD_NAME)
+    {
+      if (!_dbus_string_validate_name (&tmp, 0, _dbus_string_get_length (&tmp)))
+        {
+          _dbus_verbose ("%s field has invalid content \"%s\"\n",
+                         field_name, _dbus_string_get_const_data (&tmp));
+          return FALSE;
+        }
+      
+      if (_dbus_string_starts_with_c_str (&tmp,
+                                          DBUS_NAMESPACE_LOCAL_MESSAGE))
+        {
+          _dbus_verbose ("Message is in the local namespace\n");
+          return FALSE;
+        }
+    }
+  else
+    {
+      if (!_dbus_string_validate_service (&tmp, 0, _dbus_string_get_length (&tmp)))
+        {
+          _dbus_verbose ("%s field has invalid content \"%s\"\n",
+                         field_name, _dbus_string_get_const_data (&tmp));
+          return FALSE;
+        }
+    }
+  
+  fields[field].offset = _DBUS_ALIGN_VALUE (pos, 4);
+  
+#if 0
+  _dbus_verbose ("Found field %s name at offset %d\n",
+                 field_name, fields[field].offset);
+#endif
+
+  return TRUE;
+}
+
 static dbus_bool_t
 decode_header_data (const DBusString   *data,
 		    int		        header_len,
@@ -3672,51 +3742,46 @@ decode_header_data (const DBusString   *data,
       pos += 4;
 
       _dbus_assert (_DBUS_ALIGN_ADDRESS (field, 4) == field);
+      
+      if (!_dbus_marshal_validate_type (data, pos, &type, &pos))
+	{
+          _dbus_verbose ("Failed to validate type of named header field\n");
+	  return FALSE;
+	}
+      
+      if (!_dbus_marshal_validate_arg (data, byte_order, 0, type, -1, pos, &new_pos))
+        {
+          _dbus_verbose ("Failed to validate argument to named header field\n");
+          return FALSE;
+        }
 
+      if (new_pos > header_len)
+        {
+          _dbus_verbose ("Named header field tries to extend beyond header length\n");
+          return FALSE;
+        }
+      
       switch (DBUS_UINT32_FROM_BE (*(int*)field))
         {
         case DBUS_HEADER_FIELD_SERVICE_AS_UINT32:
-          if (fields[FIELD_SERVICE].offset >= 0)
-            {
-              _dbus_verbose ("%s field provided twice\n",
-                             DBUS_HEADER_FIELD_SERVICE);
-              return FALSE;
-            }
-          
-          fields[FIELD_SERVICE].offset = _DBUS_ALIGN_VALUE (pos + 1, 4);
-#if 0
-          _dbus_verbose ("Found service name at offset %d\n",
-                         fields[FIELD_SERVICE].offset);
-#endif
+          if (!decode_string_field (data, fields, pos, type,
+                                    FIELD_SERVICE,
+                                    DBUS_HEADER_FIELD_SERVICE))
+            return FALSE;
           break;
 
         case DBUS_HEADER_FIELD_NAME_AS_UINT32:
-          if (fields[FIELD_NAME].offset >= 0)
-            {              
-              _dbus_verbose ("%s field provided twice\n",
-                             DBUS_HEADER_FIELD_NAME);
-              return FALSE;
-            }
-          
-          fields[FIELD_NAME].offset = _DBUS_ALIGN_VALUE (pos + 1, 4);
-
-#if 0
-          _dbus_verbose ("Found message name at offset %d\n",
-                         fields[FIELD_NAME].offset);
-#endif
+          if (!decode_string_field (data, fields, pos, type,
+                                    FIELD_NAME,
+                                    DBUS_HEADER_FIELD_NAME))
+            return FALSE;
           break;
-	case DBUS_HEADER_FIELD_SENDER_AS_UINT32:
-          if (fields[FIELD_SENDER].offset >= 0)
-            {
-              _dbus_verbose ("%s field provided twice\n",
-                             DBUS_HEADER_FIELD_SENDER);
-              return FALSE;
-            }
-          
-          fields[FIELD_SENDER].offset = _DBUS_ALIGN_VALUE (pos + 1, 4);
 
-          _dbus_verbose ("Found sender name at offset %d\n",
-                         fields[FIELD_NAME].offset);
+	case DBUS_HEADER_FIELD_SENDER_AS_UINT32:
+          if (!decode_string_field (data, fields, pos, type,
+                                    FIELD_SENDER,
+                                    DBUS_HEADER_FIELD_SENDER))
+            return FALSE;
 	  break;
           
 	case DBUS_HEADER_FIELD_REPLY_AS_UINT32:
@@ -3726,8 +3791,14 @@ decode_header_data (const DBusString   *data,
                              DBUS_HEADER_FIELD_REPLY);
               return FALSE;
             }
+
+          if (type != DBUS_TYPE_UINT32)
+            {
+              _dbus_verbose ("%s field has wrong type\n", DBUS_HEADER_FIELD_REPLY);
+              return FALSE;
+            }
           
-          fields[FIELD_REPLY_SERIAL].offset = _DBUS_ALIGN_VALUE (pos + 1, 4);
+          fields[FIELD_REPLY_SERIAL].offset = _DBUS_ALIGN_VALUE (pos, 4);
 
           _dbus_verbose ("Found reply serial at offset %d\n",
                          fields[FIELD_REPLY_SERIAL].offset);
@@ -3737,24 +3808,6 @@ decode_header_data (const DBusString   *data,
 	  _dbus_verbose ("Ignoring an unknown header field: %c%c%c%c at offset %d\n",
 			 field[0], field[1], field[2], field[3], pos);
 	}
-
-      if (!_dbus_marshal_validate_type (data, pos, &type, &pos))
-	{
-          _dbus_verbose ("Failed to validate type of named header field\n");
-	  return FALSE;
-	}
-      
-      if (!_dbus_marshal_validate_arg (data, byte_order, 0, type, -1, pos, &new_pos))
-        {
-          _dbus_verbose ("Failed to validate argument to named header field\n");
-          return FALSE;
-        }
-
-      if (new_pos > header_len)
-        {
-          _dbus_verbose ("Named header field tries to extend beyond header length\n");
-          return FALSE;
-        }
       
       pos = new_pos;
     }
@@ -3772,12 +3825,13 @@ decode_header_data (const DBusString   *data,
         }
     }
 
- if (fields[FIELD_NAME].offset < 0)
-   {
-     _dbus_verbose ("No %s field provided\n",
-                    DBUS_HEADER_FIELD_NAME);
-     return FALSE;
-   }
+  /* Name field is mandatory */
+  if (fields[FIELD_NAME].offset < 0)
+    {
+      _dbus_verbose ("No %s field provided\n",
+                     DBUS_HEADER_FIELD_NAME);
+      return FALSE;
+    }
   
   if (message_padding)
     *message_padding = header_len - pos;  
@@ -5076,7 +5130,7 @@ _dbus_message_test (const char *test_data_dir)
   _dbus_assert (sizeof (DBusMessageRealIter) <= sizeof (DBusMessageIter));
 
   /* Test the vararg functions */
-  message = dbus_message_new ("org.freedesktop.DBus.Test", "testMessage");
+  message = dbus_message_new ("org.freedesktop.DBus.Test", "test.Message");
   _dbus_message_set_serial (message, 1);
   dbus_message_append_args (message,
 			    DBUS_TYPE_INT32, -0x12345678,
@@ -5121,7 +5175,7 @@ _dbus_message_test (const char *test_data_dir)
   dbus_message_unref (message);
   dbus_message_unref (copy);
   
-  message = dbus_message_new ("org.freedesktop.DBus.Test", "testMessage");
+  message = dbus_message_new ("org.freedesktop.DBus.Test", "test.Message");
   _dbus_message_set_serial (message, 1);
   dbus_message_set_reply_serial (message, 0x12345678);
 
diff --git a/dbus/dbus-protocol.h b/dbus/dbus-protocol.h
index 651969c4..314a9934 100644
--- a/dbus/dbus-protocol.h
+++ b/dbus/dbus-protocol.h
@@ -53,7 +53,10 @@ extern "C" {
 #define DBUS_TYPE_DICT          10
 
 #define DBUS_TYPE_LAST DBUS_TYPE_DICT
- 
+
+/* Max length in bytes of a service or message name */
+#define DBUS_MAXIMUM_NAME_LENGTH 256
+
 /* Header flags */
 #define DBUS_HEADER_FLAG_ERROR 0x1
   
@@ -92,7 +95,12 @@ extern "C" {
 #define DBUS_MESSAGE_SERVICE_DELETED       "org.freedesktop.DBus.ServiceDeleted"
 #define DBUS_MESSAGE_SERVICE_LOST          "org.freedesktop.DBus.ServiceLost"
 
-#define DBUS_MESSAGE_LOCAL_DISCONNECT      "org.freedesktop.Local.Disconnect"
+
+/* This namespace is reserved for locally-synthesized messages, you can't
+ * send messages that have this namespace.
+ */
+#define DBUS_NAMESPACE_LOCAL_MESSAGE       "org.freedesktop.Local."
+#define DBUS_MESSAGE_LOCAL_DISCONNECT      DBUS_NAMESPACE_LOCAL_MESSAGE"Disconnect"
   
 #ifdef __cplusplus
 }
diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c
index 71fc5fcc..8abc74ac 100644
--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -28,6 +28,7 @@
 #include "dbus-marshal.h"
 #define DBUS_CAN_USE_DBUS_STRING_PRIVATE 1
 #include "dbus-string-private.h"
+#include "dbus-protocol.h"
 
 /**
  * @defgroup DBusString string class
@@ -2642,6 +2643,125 @@ _dbus_string_validate_nul (const DBusString *str,
 }
 
 /**
+ * Checks that the given range of the string is a valid message name
+ * in the D-BUS protocol. This includes a length restriction, etc.,
+ * see the specification. It does not validate UTF-8, that has to be
+ * done separately for now.
+ *
+ * @todo this is inconsistent with most of DBusString in that
+ * it allows a start,len range that isn't in the string.
+ * 
+ * @param str the string
+ * @param start first byte index to check
+ * @param len number of bytes to check
+ * @returns #TRUE if the byte range exists and is a valid name
+ */
+dbus_bool_t
+_dbus_string_validate_name (const DBusString  *str,
+                            int                start,
+                            int                len)
+{
+  const unsigned char *s;
+  const unsigned char *end;
+  dbus_bool_t saw_dot;
+  
+  DBUS_CONST_STRING_PREAMBLE (str);
+  _dbus_assert (start >= 0);
+  _dbus_assert (len >= 0);
+  _dbus_assert (start <= real->len);
+  
+  if (len > real->len - start)
+    return FALSE;
+
+  if (len > DBUS_MAXIMUM_NAME_LENGTH)
+    return FALSE;
+
+  if (len == 0)
+    return FALSE;
+
+  saw_dot = FALSE;
+  s = real->str + start;
+  end = s + len;
+  while (s != end)
+    {
+      if (*s == '.')
+        {
+          saw_dot = TRUE;
+          break;
+        }
+      
+      ++s;
+    }
+
+  if (!saw_dot)
+    return FALSE;
+  
+  return TRUE;
+}
+
+
+/**
+ * Checks that the given range of the string is a valid service name
+ * in the D-BUS protocol. This includes a length restriction, etc.,
+ * see the specification. It does not validate UTF-8, that has to be
+ * done separately for now.
+ *
+ * @todo this is inconsistent with most of DBusString in that
+ * it allows a start,len range that isn't in the string.
+ * 
+ * @param str the string
+ * @param start first byte index to check
+ * @param len number of bytes to check
+ * @returns #TRUE if the byte range exists and is a valid name
+ */
+dbus_bool_t
+_dbus_string_validate_service (const DBusString  *str,
+                               int                start,
+                               int                len)
+{
+  const unsigned char *s;
+  const unsigned char *end;
+  dbus_bool_t saw_dot;
+  dbus_bool_t is_base_service;
+  
+  DBUS_CONST_STRING_PREAMBLE (str);
+  _dbus_assert (start >= 0);
+  _dbus_assert (len >= 0);
+  _dbus_assert (start <= real->len);
+  
+  if (len > real->len - start)
+    return FALSE;
+
+  if (len > DBUS_MAXIMUM_NAME_LENGTH)
+    return FALSE;
+
+  if (len == 0)
+    return FALSE;
+
+  is_base_service = _dbus_string_get_byte (str, start) == ':';
+  if (is_base_service)
+    return TRUE; /* can have any content */
+
+  /* non-base-service must have the '.' indicating a namespace */
+  
+  saw_dot = FALSE;
+  s = real->str + start;
+  end = s + len;
+  while (s != end)
+    {
+      if (*s == '.')
+        {
+          saw_dot = TRUE;
+          break;
+        }
+      
+      ++s;
+    }
+  
+  return saw_dot;
+}
+
+/**
  * Clears all allocated bytes in the string to zero.
  *
  * @param str the string
diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h
index 065c9caa..b9b298ae 100644
--- a/dbus/dbus-string.h
+++ b/dbus/dbus-string.h
@@ -210,6 +210,12 @@ dbus_bool_t   _dbus_string_validate_utf8         (const DBusString  *str,
 dbus_bool_t   _dbus_string_validate_nul          (const DBusString  *str,
                                                   int                start,
                                                   int                len);
+dbus_bool_t   _dbus_string_validate_name         (const DBusString  *str,
+                                                  int                start,
+                                                  int                len);
+dbus_bool_t   _dbus_string_validate_service      (const DBusString  *str,
+                                                  int                start,
+                                                  int                len);
 void          _dbus_string_zero                  (DBusString        *str);
 
 
diff --git a/doc/TODO b/doc/TODO
index a02804a1..2cd7e44d 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -1,17 +1,4 @@
 
- - Service and message names should be more carefully restricted;
-   they should have a max length, may not be an empty string, 
-   and perhaps should not be allowed to be a glob such as "*" since
-   the config file could conveniently use such notation. 
-
-   Suggest requiring length > 0, length < max, 
-   name contains at least one ".", no initial ".", and valid UTF-8. 
-   That would prohibit plain "*" but not "foo.bar.baz.operator*"
-
-   For maximum convenience from all programming languages, we could go
-   further and just categorically ban nearly all non-alphanumeric
-   characters.
-
  - Message matching rules (so broadcasts can be filtered) need sorting
    out.
 
@@ -74,6 +61,3 @@
 
  - We have a limit on the number of messages a connection can send, but 
    not on how many can be buffered for a given connection.
-
- - other apps can send you a fake DBUS_MESSAGE_LOCAL_DISCONNECT; need to 
-   check for that and disallow it.
diff --git a/test/break-loader.c b/test/break-loader.c
index 1ddaec40..ebe2606b 100644
--- a/test/break-loader.c
+++ b/test/break-loader.c
@@ -499,6 +499,7 @@ find_breaks_based_on (const DBusString   *filename,
       goto failed;
     }
 
+  printf ("        changing one random byte 100 times\n");
   i = 0;
   while (i < 100)
     {
@@ -508,6 +509,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        changing length 50 times\n");
   i = 0;
   while (i < 50)
     {
@@ -516,7 +518,8 @@ find_breaks_based_on (const DBusString   *filename,
 
       ++i;
     }
-  
+
+  printf ("        removing one byte 50 times\n");
   i = 0;
   while (i < 50)
     {
@@ -526,6 +529,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        adding one byte 50 times\n");
   i = 0;
   while (i < 50)
     {
@@ -535,6 +539,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        changing ints to boundary values 50 times\n");
   i = 0;
   while (i < 50)
     {
@@ -544,6 +549,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        changing typecodes 50 times\n");
   i = 0;
   while (i < 50)
     {
@@ -552,7 +558,8 @@ find_breaks_based_on (const DBusString   *filename,
 
       ++i;
     }
-  
+
+  printf ("        changing message length 15 times\n");
   i = 0;
   while (i < 15)
     {
@@ -562,6 +569,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        randomly making 2 of above modifications 42 times\n");
   i = 0;
   while (i < 42)
     {
@@ -571,6 +579,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        randomly making 3 of above modifications 42 times\n");
   i = 0;
   while (i < 42)
     {
@@ -580,6 +589,7 @@ find_breaks_based_on (const DBusString   *filename,
       ++i;
     }
 
+  printf ("        randomly making 4 of above modifications 42 times\n");
   i = 0;
   while (i < 42)
     {
diff --git a/test/data/invalid-messages/local-namespace.message b/test/data/invalid-messages/local-namespace.message
new file mode 100644
index 00000000..ceb3053d
--- /dev/null
+++ b/test/data/invalid-messages/local-namespace.message
@@ -0,0 +1,12 @@
+## a message that is in the org.freedesktop.Local. namespace and thus
+## invalid
+
+## VALID_HEADER includes a LENGTH Header and LENGTH Body
+VALID_HEADER
+FIELD_NAME name
+TYPE STRING
+STRING 'org.freedesktop.Local.Disconnect'
+ALIGN 8
+END_LENGTH Header
+START_LENGTH Body
+END_LENGTH Body
diff --git a/test/data/invalid-messages/no-dot-in-name.message b/test/data/invalid-messages/no-dot-in-name.message
new file mode 100644
index 00000000..4cde0d1f
--- /dev/null
+++ b/test/data/invalid-messages/no-dot-in-name.message
@@ -0,0 +1,11 @@
+## a message with dotless name
+
+## VALID_HEADER includes a LENGTH Header and LENGTH Body
+VALID_HEADER
+FIELD_NAME name
+TYPE STRING
+STRING 'NoNamespaceHere'
+ALIGN 8
+END_LENGTH Header
+START_LENGTH Body
+END_LENGTH Body
diff --git a/test/data/invalid-messages/overlong-name.message b/test/data/invalid-messages/overlong-name.message
new file mode 100644
index 00000000..0fdc7bc9
--- /dev/null
+++ b/test/data/invalid-messages/overlong-name.message
@@ -0,0 +1,11 @@
+## a message with too-long name field
+
+## VALID_HEADER includes a LENGTH Header and LENGTH Body
+VALID_HEADER
+FIELD_NAME name
+TYPE STRING
+STRING 'org.foo.bar.this.is.really.long 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200'
+ALIGN 8
+END_LENGTH Header
+START_LENGTH Body
+END_LENGTH Body
diff --git a/test/data/valid-messages/opposite-endian.message b/test/data/valid-messages/opposite-endian.message
index dba8918d..f8975b8b 100644
--- a/test/data/valid-messages/opposite-endian.message
+++ b/test/data/valid-messages/opposite-endian.message
@@ -6,8 +6,8 @@ OPPOSITE_ENDIAN
 VALID_HEADER
 
 FIELD_NAME rply
-TYPE INT32
-INT32 10000
+TYPE UINT32
+UINT32 10000
 
 FIELD_NAME name
 TYPE STRING