From: Mark Wooding <mdw@distorted.org.uk>
Date: Thu, 31 Dec 2015 21:02:28 +0000 (+0000)
Subject: cgi/actions.c, lib/client*.[ch]: Don't use priv connection to check passwd.
X-Git-Tag: 5.2~72
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~mdw/git/disorder/commitdiff_plain/ff92debd29cab62e0f824ca3e40914f64e0bcb05

cgi/actions.c, lib/client*.[ch]: Don't use priv connection to check passwd.

If the CGI runs as the main `jukebox' user, then it can connect using
the special `.../private/socket', and the `find_server' function
arranges to do this if it can and no network address has been assigned
in the configuration.

The server doesn't bother to check passwords from clients on privileged
connections.  The result is that if the CGI program runs as the
`jukebox' user, its attempt to check the end-user's password through the
usual login machinery is stymied, since the library automatically sets
up a privileged connection and then the server ignores the password
entirely.  The end result is that, if you set things up in this way,
anyone can log into the CGI program with any known user and any password
at all, at which point they're given a cookie which can be used in
direct communication with the server.

Fix this as follows.

  * Introduce a new version (in the correct namespace for a change) of
    `find_server' which accepts some flags to guide the choice of server
    addresses.

  * Add a flag for the new `disorder_find_server' function to prevent
    trying the privileged socket.

  * Add a function to the client interface (and a bit of state to the
    client structure) to instruct the connection functions not to use a
    privileged connection even if they can.

  * Get the CGI program to use this new function when logging in with a
    password, so that it will always be checked.

Cookies are checked properly, even from privileged clients, so there's
nothing to fix there.
---

diff --git a/cgi/actions.c b/cgi/actions.c
index 6e18098..6f094cb 100644
--- a/cgi/actions.c
+++ b/cgi/actions.c
@@ -307,8 +307,12 @@ static int login_as(const char *username, const char *password) {
 
   if(dcgi_cookie && dcgi_client)
     disorder_revoke(dcgi_client);
-  /* We'll need a new connection as we are going to stop being guest */
+  /* We'll need a new connection as we are going to stop being guest.
+   * Make sure it's unprivileged, so that the server actually bothers checking
+   * the password we supply.
+   */
   c = disorder_new(0);
+  disorder_force_unpriv(c);
   if(disorder_connect_user(c, username, password)) {
     login_error("loginfailed");
     return -1;
diff --git a/lib/client-common.c b/lib/client-common.c
index a4a9851..ead7d2e 100644
--- a/lib/client-common.c
+++ b/lib/client-common.c
@@ -46,12 +46,13 @@
 
 /** @brief Figure out what address to connect to
  * @param c Configuration to honor
+ * @param flags Flags to guide the choice
  * @param sap Where to store pointer to sockaddr
  * @param namep Where to store socket name
  * @return Socket length, or (socklen_t)-1
  */
-socklen_t find_server(struct config *c,
-                      struct sockaddr **sap, char **namep) {
+socklen_t disorder_find_server(struct config *c, unsigned flags,
+                               struct sockaddr **sap, char **namep) {
   struct sockaddr *sa;
 #if !_WIN32
   struct sockaddr_un su;
@@ -71,10 +72,12 @@ socklen_t find_server(struct config *c,
     disorder_fatal(0, "local connections are not supported on Windows");
 #else
     /* use the private socket if possible (which it should be) */
-    name = config_get_file2(c, "private/socket");
-    if(access(name, R_OK) != 0) {
-      xfree(name);
-      name = NULL;
+    if (!(flags & DISORDER_FS_NOTPRIV)) {
+      name = config_get_file2(c, "private/socket");
+      if(access(name, R_OK) != 0) {
+        xfree(name);
+        name = NULL;
+      }
     }
     if(!name)
       name = config_get_file2(c, "socket");
@@ -99,6 +102,21 @@ socklen_t find_server(struct config *c,
   return len;
 }
 
+/** @brief Figure out what address to connect to
+ * @param c Configuration to honor
+ * @param sap Where to store pointer to sockaddr
+ * @param namep Where to store socket name
+ * @return Socket length, or (socklen_t)-1
+ *
+ * The function disorder_find_server() isn't a namespace violation, and has
+ * more functionality.  This function is equivalent, to disorder_find_server()
+ * with a zero @c flags argument.
+ */
+socklen_t find_server(struct config *c,
+                      struct sockaddr **sap, char **namep) {
+  return disorder_find_server(c, 0, sap, namep);
+}
+
 const char disorder__body[1];
 const char disorder__list[1];
 const char disorder__integer[1];
diff --git a/lib/client-common.h b/lib/client-common.h
index dd53a1d..4eca7e0 100644
--- a/lib/client-common.h
+++ b/lib/client-common.h
@@ -27,6 +27,10 @@
 # include <sys/socket.h>
 #endif
 
+#define DISORDER_FS_NOTPRIV 1u
+
+socklen_t disorder_find_server(struct config *c, unsigned flags,
+                               struct sockaddr **sap, char **namep);
 socklen_t find_server(struct config *c, struct sockaddr **sap, char **namep);
 
 /** @brief Marker for a command body */
diff --git a/lib/client.c b/lib/client.c
index 52c5b68..1967e0e 100644
--- a/lib/client.c
+++ b/lib/client.c
@@ -85,6 +85,8 @@ struct disorder_client {
   int open;
   /** @brief Socket I/O context */
   struct socketio sio;
+  /** @brief Whether to try to open a privileged connection */
+  int trypriv;
 };
 
 /** @brief Create a new client
@@ -100,9 +102,21 @@ disorder_client *disorder_new(int verbose) {
 
   c->verbose = verbose;
   c->family = -1;
+  c->trypriv = 1;
   return c;
 }
 
+/** @brief Don't try to make a privileged connection
+ * @param c Client
+ *
+ * You must call this before any of the connection functions (e.g.,
+ * disorder_connect(), disorder_connect_user()), if at all.
+ */
+void disorder_force_unpriv(disorder_client *c) {
+  assert(!c->open);
+  c->trypriv = 0;
+}
+
 /** @brief Return the address family used by this client */
 int disorder_client_af(disorder_client *c) {
   return c->family;
@@ -432,7 +446,9 @@ int disorder_connect_generic(struct config *conf,
   socklen_t salen;
   char errbuf[1024];
 
-  if((salen = find_server(conf, &sa, &c->ident)) == (socklen_t)-1)
+  if((salen = disorder_find_server(conf,
+				   (c->trypriv ? 0 : DISORDER_FS_NOTPRIV),
+				   &sa, &c->ident)) == (socklen_t)-1)
     return -1;
   c->input = 0;
   c->output = 0;
diff --git a/lib/client.h b/lib/client.h
index c7964d6..c443f7e 100644
--- a/lib/client.h
+++ b/lib/client.h
@@ -39,6 +39,7 @@ struct kvp;
 struct sink;
 
 disorder_client *disorder_new(int verbose);
+void disorder_force_unpriv(disorder_client *c);
 int disorder_client_af(disorder_client *c);
 int disorder_connect(disorder_client *c);
 int disorder_connect_user(disorder_client *c,