r8835@totoro: nickm | 2006-10-02 12:54:41 -0400

 Improve error messages from AUTHENTICATE attempts to controller.


svn:r8574

3 files changed, 26 insertions, 14 deletions

diff --git a/ChangeLog b/ChangeLog
index 42b2ff061f..8d939557e7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -50,6 +50,7 @@ Changes in version 0.1.2.2-alpha - 2006-10-??
     - Avoid choosing Exit nodes for entry or middle hops when the bandwidth
       available in non-Exit nodes is much higher then the bandwidth available
       in Exit nodes. (Fixes bug 200.)
+    - Give more meaningful errors on control authentication failure.
 
   o Security Fixes, minor:
     - If a client asked for a server by name, and we didn't have a
diff --git a/doc/control-spec.txt b/doc/control-spec.txt
index 2bc8d7db0d..163dc3bf1a 100644
--- a/doc/control-spec.txt
+++ b/doc/control-spec.txt
@@ -788,12 +788,7 @@ $Id$
 
   If the 'CookieAuthentication' option is true, Tor writes a "magic cookie"
   file named "control_auth_cookie" into its data directory.  To authenticate,
-  the controller must send the contents of this file.
-
-  [With the v1 controller protocol, what we really mean is that you should
-   send the base16 of the contents of this file. Is this it, or is there
-   more to it? Should we write a control_auth_cookie.asc file too that
-   makes this step easier for people doing it manually? -RD]
+  the controller must send the contents of this file, encoded in hexadecimal.
 
   If the 'HashedControlPassword' option is set, it must contain the salted
   hash of a secret password.  The salted hash is computed according to the
diff --git a/src/or/control.c b/src/or/control.c
index d75a4e47a6..353c23592e 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -1010,6 +1010,7 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len,
 {
   int used_quoted_string = 0;
   or_options_t *options = get_options();
+  const char *errstr = NULL;
   char *password;
   size_t password_len;
   if (STATE_IS_V0(conn->_base.state)) {
@@ -1043,8 +1044,16 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len,
     }
   }
   if (options->CookieAuthentication) {
-    if (password_len == AUTHENTICATION_COOKIE_LEN &&
-        !memcmp(authentication_cookie, password, password_len)) {
+    if (password_len != AUTHENTICATION_COOKIE_LEN) {
+      log_warn(LD_CONTROL, "Got authentication cookie with wrong length (%d)",
+               password_len);
+      errstr = "Wrong length on authentication cookie.";
+      goto err;
+    } else if (memcmp(authentication_cookie, password, password_len)) {
+      log_warn(LD_CONTROL, "Got mismatched authentication cookie");
+      errstr = "Authentication cookie did not match expected value.";
+      goto err;
+    } else {
       goto ok;
     }
   } else if (options->HashedControlPassword) {
@@ -1053,11 +1062,20 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len,
     if (decode_hashed_password(expected, options->HashedControlPassword)<0) {
       log_warn(LD_CONTROL,
                "Couldn't decode HashedControlPassword: invalid base16");
+      errstr = "Couldn't decode HashedControlPassword value in configuration.";
       goto err;
     }
     secret_to_key(received,DIGEST_LEN,password,password_len,expected);
     if (!memcmp(expected+S2K_SPECIFIER_LEN, received, DIGEST_LEN))
       goto ok;
+
+    if (used_quoted_string)
+      errstr = "Password did not match HashedControlPassword value from "
+        "configuration";
+    else
+      errstr = "Password did not match HashedControlPassword value from "
+        "configuration. Maybe you tried a plain text password? "
+        "If so, the standard requires you put it in double quotes.";
     goto err;
   } else {
     /* if Tor doesn't demand any stronger authentication, then
@@ -1071,12 +1089,10 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len,
                         "Authentication failed");
   else {
     tor_free(password);
-    if (used_quoted_string)
-      connection_write_str_to_buf("515 Authentication failed\r\n", conn);
-    else
-      connection_write_str_to_buf(
-         "515 Authentication failed.  Maybe you tried a plain text password?  "
-         "If so, the standard requires you put it in double quotes.\r\n",conn);
+    if (!errstr)
+      errstr = "Unknown reason.";
+    connection_printf_to_buf(conn, "515 Authentication failed: %s\r\n",
+                             errstr);
   }
   return 0;
  ok: