2003-10-14 Havoc Pennington <hp@redhat.com>

	* bus/bus.c (bus_context_check_security_policy): revamp this to
	work more sanely with new policy-based requested reply setup

	* bus/connection.c (bus_transaction_send_from_driver): set bus
	driver messages as no reply

	* bus/policy.c (bus_client_policy_check_can_receive): handle a
	requested_reply attribute on allow/deny rules

	* bus/system.conf: add <allow requested_reply="true"/>

	* bus/driver.c (bus_driver_handle_message): fix check for replies
	sent to the bus driver, which was backward. How did this ever work
	at all though? I think I'm missing something.

	* dbus/dbus-message.c (decode_header_data): require error and
	method return messages to have a reply serial field to be valid
	(_dbus_message_loader_queue_messages): break up this function;
	validate that reply serial and plain serial are nonzero;
	clean up the OOM/error handling.
	(get_uint_field): don't return -1 from this
	(dbus_message_create_header): fix signed/unsigned bug

	* bus/connection.c (bus_connections_expect_reply): save serial of
	the incoming message, not reply serial

7 files changed, 151 insertions, 56 deletions

diff --git a/bus/bus.c b/bus/bus.c
index 27506240..a00201af 100644
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -909,6 +909,7 @@ bus_context_check_security_policy (BusContext     *context,
   BusClientPolicy *sender_policy;
   BusClientPolicy *recipient_policy;
   int type;
+  dbus_bool_t requested_reply;
   
   type = dbus_message_get_type (message);
   
@@ -919,49 +920,54 @@ bus_context_check_security_policy (BusContext     *context,
                 addressed_recipient != NULL ||
                 strcmp (dbus_message_get_destination (message), DBUS_SERVICE_ORG_FREEDESKTOP_DBUS) == 0);
   
+  switch (type)
+    {
+    case DBUS_MESSAGE_TYPE_METHOD_CALL:
+    case DBUS_MESSAGE_TYPE_SIGNAL:
+    case DBUS_MESSAGE_TYPE_METHOD_RETURN:
+    case DBUS_MESSAGE_TYPE_ERROR:
+      break;
+      
+    default:
+      _dbus_verbose ("security check disallowing message of unknown type %d\n",
+                     type);
+
+      dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED,
+                      "Message bus will not accept messages of unknown type\n");
+              
+      return FALSE;
+    }
+
+  requested_reply = FALSE;
+  
   if (sender != NULL)
     {
       if (bus_connection_is_active (sender))
         {
           sender_policy = bus_connection_get_policy (sender);
           _dbus_assert (sender_policy != NULL);
-
-          switch (type)
+          
+          /* Fill in requested_reply variable with TRUE if this is a
+           * reply and the reply was pending.
+           */
+          if (dbus_message_get_reply_serial (message) != 0)
             {
-            case DBUS_MESSAGE_TYPE_METHOD_CALL:
-            case DBUS_MESSAGE_TYPE_SIGNAL:
-
-              /* Continue below to check security policy */
-              break;
-              
-            case DBUS_MESSAGE_TYPE_METHOD_RETURN:
-            case DBUS_MESSAGE_TYPE_ERROR:
-              /* These are only allowed if the reply is listed
-               * as pending, or the connection is eavesdropping.
-               * The idea is to prohibit confusing/fake replies.
-               * FIXME In principle a client that's asked to eavesdrop
-               * specifically should probably get bogus replies
-               * even to itself, but here we prohibit that.
-               */
-              
               if (proposed_recipient != NULL /* not to the bus driver */ &&
-                  addressed_recipient == proposed_recipient /* not eavesdropping */ &&
-                  !bus_connections_check_reply (bus_connection_get_connections (sender),
-                                                transaction,
-                                                sender, addressed_recipient, message,
-                                                error))
-                return FALSE;
-
-              /* Continue below to check security policy, since reply was expected */
-              break;
-              
-            default:
-              _dbus_verbose ("security check disallowing message of unknown type\n");
-
-              dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED,
-                              "Message bus will not accept messages of unknown type\n");
-              
-              return FALSE;
+                  addressed_recipient == proposed_recipient /* not eavesdropping */)
+                {
+                  DBusError error2;                  
+                  
+                  dbus_error_init (&error2);
+                  requested_reply = bus_connections_check_reply (bus_connection_get_connections (sender),
+                                                                 transaction,
+                                                                 sender, addressed_recipient, message,
+                                                                 &error2);
+                  if (dbus_error_is_set (&error2))
+                    {
+                      dbus_move_error (&error2, error);
+                      return FALSE;
+                    }
+                }
             }
         }
       else
@@ -992,7 +998,16 @@ bus_context_check_security_policy (BusContext     *context,
         }
     }
   else
-    sender_policy = NULL;
+    {
+      sender_policy = NULL;
+
+      /* If the sender is the bus driver, we assume any reply was a
+       * requested reply as bus driver won't send bogus ones
+       */
+      if (addressed_recipient == proposed_recipient /* not eavesdropping */ &&
+          dbus_message_get_reply_serial (message) != 0)
+        requested_reply = TRUE;
+    }
 
   _dbus_assert ((sender != NULL && sender_policy != NULL) ||
                 (sender == NULL && sender_policy == NULL));
@@ -1050,7 +1065,9 @@ bus_context_check_security_policy (BusContext     *context,
 
   if (recipient_policy &&
       !bus_client_policy_check_can_receive (recipient_policy,
-                                            context->registry, sender,
+                                            context->registry,
+                                            requested_reply,
+                                            sender,
                                             addressed_recipient, proposed_recipient,
                                             message))
     {
@@ -1059,14 +1076,16 @@ bus_context_check_security_policy (BusContext     *context,
                       "A security policy in place prevents this recipient "
                       "from receiving this message from this sender, "
                       "see message bus configuration file (rejected message "
-                      "had interface \"%s\" member \"%s\" error name \"%s\" destination \"%s\")",
+                      "had interface \"%s\" member \"%s\" error name \"%s\" destination \"%s\" reply serial %u requested_reply=%d)",
                       dbus_message_get_interface (message) ?
                       dbus_message_get_interface (message) : "(unset)",
                       dbus_message_get_member (message) ?
                       dbus_message_get_member (message) : "(unset)",
                       dbus_message_get_error_name (message) ?
                       dbus_message_get_error_name (message) : "(unset)",
-                      dest ? dest : DBUS_SERVICE_ORG_FREEDESKTOP_DBUS);
+                      dest ? dest : DBUS_SERVICE_ORG_FREEDESKTOP_DBUS,
+                      dbus_message_get_reply_serial (message),
+                      requested_reply);
       _dbus_verbose ("security policy disallowing message due to recipient policy\n");
       return FALSE;
     }
diff --git a/bus/config-parser.c b/bus/config-parser.c
index b3652591..cc55a492 100644
--- a/bus/config-parser.c
+++ b/bus/config-parser.c
@@ -837,6 +837,7 @@ append_rule_from_element (BusConfigParser   *parser,
   const char *receive_path;
   const char *receive_type;
   const char *eavesdrop;
+  const char *requested_reply;
   const char *own;
   const char *user;
   const char *group;
@@ -859,6 +860,7 @@ append_rule_from_element (BusConfigParser   *parser,
                           "receive_path", &receive_path,
                           "receive_type", &receive_type,
                           "eavesdrop", &eavesdrop,
+                          "requested_reply", &requested_reply,
                           "own", &own,
                           "user", &user,
                           "group", &group,
@@ -868,7 +870,7 @@ append_rule_from_element (BusConfigParser   *parser,
   if (!(send_interface || send_member || send_error || send_destination ||
         send_type || send_path ||
         receive_interface || receive_member || receive_error || receive_sender ||
-        receive_type || receive_path || eavesdrop ||
+        receive_type || receive_path || eavesdrop || requested_reply ||
         own || user || group))
     {
       dbus_set_error (error, DBUS_ERROR_FAILED,
@@ -895,7 +897,7 @@ append_rule_from_element (BusConfigParser   *parser,
    *     error
    * 
    *   base send_ can combine with send_destination, send_path, send_type
-   *   base receive_ with receive_sender, receive_path, receive_type, eavesdrop
+   *   base receive_ with receive_sender, receive_path, receive_type, eavesdrop, requested_reply
    *
    *   user, group, own must occur alone
    *
@@ -908,6 +910,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_interface && receive_error) ||
        (send_interface && receive_sender) ||
        (send_interface && eavesdrop) ||
+       (send_interface && requested_reply) ||
        (send_interface && own) ||
        (send_interface && user) ||
        (send_interface && group)) ||
@@ -918,6 +921,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_member && receive_error) ||
        (send_member && receive_sender) ||
        (send_member && eavesdrop) ||
+       (send_member && requested_reply) ||
        (send_member && own) ||
        (send_member && user) ||
        (send_member && group)) ||
@@ -927,6 +931,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_error && receive_error) ||
        (send_error && receive_sender) ||
        (send_error && eavesdrop) ||
+       (send_error && requested_reply) ||
        (send_error && own) ||
        (send_error && user) ||
        (send_error && group)) ||
@@ -936,6 +941,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_destination && receive_error) ||
        (send_destination && receive_sender) ||
        (send_destination && eavesdrop) ||
+       (send_destination && requested_reply) ||
        (send_destination && own) ||
        (send_destination && user) ||
        (send_destination && group)) ||
@@ -945,6 +951,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_type && receive_error) ||
        (send_type && receive_sender) ||
        (send_type && eavesdrop) ||
+       (send_type && requested_reply) ||
        (send_type && own) ||
        (send_type && user) ||
        (send_type && group)) ||
@@ -954,6 +961,7 @@ append_rule_from_element (BusConfigParser   *parser,
        (send_path && receive_error) ||
        (send_path && receive_sender) ||
        (send_path && eavesdrop) ||
+       (send_path && requested_reply) ||
        (send_path && own) ||
        (send_path && user) ||
        (send_path && group)) ||
@@ -975,6 +983,10 @@ append_rule_from_element (BusConfigParser   *parser,
       ((eavesdrop && own) ||
        (eavesdrop && user) ||
        (eavesdrop && group)) ||
+
+      ((requested_reply && own) ||
+       (requested_reply && user) ||
+       (requested_reply && group)) ||
       
       ((own && user) ||
        (own && group)) ||
@@ -1047,7 +1059,7 @@ append_rule_from_element (BusConfigParser   *parser,
         goto nomem;
     }
   else if (receive_interface || receive_member || receive_error || receive_sender ||
-           receive_path || receive_type || eavesdrop)
+           receive_path || receive_type || eavesdrop || requested_reply)
     {
       int message_type;
       
@@ -1083,8 +1095,18 @@ append_rule_from_element (BusConfigParser   *parser,
             strcmp (eavesdrop, "false") == 0))
         {
           dbus_set_error (error, DBUS_ERROR_FAILED,
-                          "Bad value \"%s\" for eavesdrop attribute, must be true or false",
-                          eavesdrop);
+                          "Bad value \"%s\" for %s attribute, must be true or false",
+                          "eavesdrop", eavesdrop);
+          return FALSE;
+        }
+
+      if (requested_reply &&
+          !(strcmp (requested_reply, "true") == 0 ||
+            strcmp (requested_reply, "false") == 0))
+        {
+          dbus_set_error (error, DBUS_ERROR_FAILED,
+                          "Bad value \"%s\" for %s attribute, must be true or false",
+                          "requested_reply", requested_reply);
           return FALSE;
         }
       
@@ -1094,6 +1116,9 @@ append_rule_from_element (BusConfigParser   *parser,
 
       if (eavesdrop)
         rule->d.receive.eavesdrop = (strcmp (eavesdrop, "true") == 0);
+
+      if (requested_reply)
+        rule->d.receive.requested_reply = (strcmp (requested_reply, "true") == 0);
       
       rule->d.receive.message_type = message_type;
       rule->d.receive.path = _dbus_strdup (receive_path);
@@ -1101,6 +1126,7 @@ append_rule_from_element (BusConfigParser   *parser,
       rule->d.receive.member = _dbus_strdup (receive_member);
       rule->d.receive.error = _dbus_strdup (receive_error);
       rule->d.receive.origin = _dbus_strdup (receive_sender);
+
       if (receive_path && rule->d.receive.path == NULL)
         goto nomem;
       if (receive_interface && rule->d.receive.interface == NULL)
diff --git a/bus/connection.c b/bus/connection.c
index 2ae1d7d2..1e562427 100644
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -1510,7 +1510,7 @@ bus_connections_expect_reply (BusConnections  *connections,
   if (dbus_message_get_no_reply (reply_to_this))
     return TRUE; /* we won't allow a reply, since client doesn't care for one. */
   
-  reply_serial = dbus_message_get_reply_serial (reply_to_this);
+  reply_serial = dbus_message_get_serial (reply_to_this);
 
   link = _dbus_list_get_first_link (&connections->pending_replies->items);
   while (link != NULL)
@@ -1651,13 +1651,8 @@ bus_connections_check_reply (BusConnections *connections,
 
   if (link == NULL)
     {
-      _dbus_verbose ("No pending reply expected, disallowing this reply\n");
-      
-      dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED,
-                      "%s message sent with reply serial %u, but no such reply was requested (or it has timed out already)\n",
-                      dbus_message_get_type (reply) == DBUS_MESSAGE_TYPE_METHOD_RETURN ?
-                      "method return" : "error",
-                      reply_serial);
+      _dbus_verbose ("No pending reply expected\n");
+
       return FALSE;
     }
 
@@ -1807,6 +1802,9 @@ bus_transaction_send_from_driver (BusTransaction *transaction,
   if (!dbus_message_set_sender (message, DBUS_SERVICE_ORG_FREEDESKTOP_DBUS))
     return FALSE;
 
+  /* bus driver never wants a reply */
+  dbus_message_set_no_reply (message, TRUE);
+  
   /* If security policy doesn't allow the message, we silently
    * eat it; the driver doesn't care about getting a reply.
    */
diff --git a/bus/dbus-daemon-1.1.in b/bus/dbus-daemon-1.1.in
index 06bbbd13..7e186e51 100644
--- a/bus/dbus-daemon-1.1.in
+++ b/bus/dbus-daemon-1.1.in
@@ -356,7 +356,8 @@ The possible attributes of these elements are:
    receive_sender="service_name" 
    receive_type="method_call" | "method_return" | "signal" | "error"
    receive_path="/path/name"
-   
+
+   requested_reply="true" | "false"
    eavesdrop="true" | "false"
 
    own="servicename"
@@ -377,7 +378,7 @@ Examples:
 .fi
 
 .PP
-The <deny> attributes determine whether the deny "matches" a
+The <deny> element's attributes determine whether the deny "matches" a
 particular action. If it matches, the action is denied (unless later
 rules in the config file allow it).
 
@@ -408,6 +409,26 @@ also, but here it means that the rule applies always, even when
 not eavesdropping. The eavesdrop attribute can only be combined with
 receive rules (with receive_* attributes).
 
+
+.PP
+The requested_reply attribute works similarly to the eavesdrop
+attribute. It controls whether the <deny> or <allow> matches a reply
+that is expected (corresponds to a previous method call message).
+This attribute only makes sense for reply messages (errors and method
+returns), and is ignored for other message types.
+
+.PP
+For <allow>, requested_reply="true" is the default and indicates that
+only requested replies are allowed by the
+rule. requested_reply="false" means that the rule allows any reply
+even if unexpected.
+
+.PP
+For <deny>, requested_reply="false" is the default but indicates that
+the rule matches only when the reply was not
+requested. requested_reply="true" indicates that the rule applies
+always, regardless of pending reply state.
+
 .PP
 user and group denials mean that the given user or group may 
 not connect to the message bus.
diff --git a/bus/driver.c b/bus/driver.c
index 791fcd69..4d345698 100644
--- a/bus/driver.c
+++ b/bus/driver.c
@@ -814,7 +814,7 @@ bus_driver_handle_message (DBusConnection *connection,
   /* security checks should have kept this from getting here */
   _dbus_assert (sender != NULL || strcmp (name, "Hello") == 0);
 
-  if (dbus_message_get_reply_serial (message) == 0)
+  if (dbus_message_get_reply_serial (message) != 0)
     {
       _dbus_verbose ("Client sent a reply to the bus driver, ignoring it\n");
       return TRUE;
diff --git a/bus/policy.c b/bus/policy.c
index 71137ca9..63131aca 100644
--- a/bus/policy.c
+++ b/bus/policy.c
@@ -56,6 +56,10 @@ bus_policy_rule_new (BusPolicyRuleType type,
       break;
     case BUS_POLICY_RULE_RECEIVE:
       rule->d.receive.message_type = DBUS_MESSAGE_TYPE_INVALID;
+      /* allow rules default to TRUE (only requested replies allowed)
+       * deny rules default to FALSE (only unrequested replies denied)
+       */
+      rule->d.receive.requested_reply = rule->allow;
       break;
     case BUS_POLICY_RULE_OWN:
       break;
@@ -919,6 +923,7 @@ bus_client_policy_check_can_send (BusClientPolicy *policy,
 dbus_bool_t
 bus_client_policy_check_can_receive (BusClientPolicy *policy,
                                      BusRegistry     *registry,
+                                     dbus_bool_t      requested_reply,
                                      DBusConnection  *sender,
                                      DBusConnection  *addressed_recipient,
                                      DBusConnection  *proposed_recipient,
@@ -978,6 +983,30 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy,
           _dbus_verbose ("  (policy) skipping deny rule since it only applies to eavesdropping\n");
           continue;
         }
+
+      /* If it's a reply, the requested_reply flag kicks in */
+      if (dbus_message_get_reply_serial (message) != 0)
+        {
+          /* for allow, requested_reply=true means the rule applies
+           * only when reply was requested. requested_reply=false means
+           * always allow.
+           */
+          if (!requested_reply && rule->allow && rule->d.receive.requested_reply)
+            {
+              _dbus_verbose ("  (policy) skipping allow rule since it only applies to requested replies\n");
+              continue;
+            }
+
+          /* for deny, requested_reply=false means the rule applies only
+           * when the reply was not requested. requested_reply=true means the
+           * rule always applies.
+           */
+          if (requested_reply && !rule->allow && !rule->d.receive.requested_reply)
+            {
+              _dbus_verbose ("  (policy) skipping deny rule since it only applies to unrequested replies\n");
+              continue;
+            }
+        }
       
       if (rule->d.receive.path != NULL)
         {
diff --git a/bus/policy.h b/bus/policy.h
index 63981cc0..17dfbf27 100644
--- a/bus/policy.h
+++ b/bus/policy.h
@@ -61,7 +61,7 @@ struct BusPolicyRule
       char *interface;
       char *member;
       char *error;
-      char *destination;
+      char *destination;      
     } send;
 
     struct
@@ -75,6 +75,7 @@ struct BusPolicyRule
       char *error;
       char *origin;
       unsigned int eavesdrop : 1;
+      unsigned int requested_reply : 1;
     } receive;
 
     struct
@@ -134,6 +135,7 @@ dbus_bool_t      bus_client_policy_check_can_send    (BusClientPolicy  *policy,
                                                       DBusMessage      *message);
 dbus_bool_t      bus_client_policy_check_can_receive (BusClientPolicy  *policy,
                                                       BusRegistry      *registry,
+                                                      dbus_bool_t       requested_reply,
                                                       DBusConnection   *sender,
                                                       DBusConnection   *addressed_recipient,
                                                       DBusConnection   *proposed_recipient,