[tor-commits] [tor/master] Do not use strcmp() to compare an http authenticator to its expected value

[nickm at torproject.org]

commit 9a69c24150965e54322ed9616638d4f1939b1289
Author: Nick Mathewson <nickm at torproject.org>
Date:   Sat Mar 31 22:51:28 2012 -0400

    Do not use strcmp() to compare an http authenticator to its expected value
    
    This fixes a side-channel attack on the (fortunately unused!)
    BridgePassword option for bridge authorities.  Fix for bug 5543;
    bugfix on 0.2.0.14-alpha.
---
 changes/bridgepassword |   11 +++++++++++
 src/or/config.c        |   19 +++++++++++++++++++
 src/or/directory.c     |   11 ++++++-----
 src/or/or.h            |    7 ++++---
 4 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/changes/bridgepassword b/changes/bridgepassword
new file mode 100644
index 0000000..5f0e250
--- /dev/null
+++ b/changes/bridgepassword
@@ -0,0 +1,11 @@
+  o Security fixes:
+    - When using the debuging BridgePassword field, a bridge authority
+      now compares alleged passwords by hashing them, then comparing
+      the result to a digest of the expected authenticator. This avoids
+      a potential side-channel attack in the previous code, which
+      had foolishly used strcmp().  Fortunately, the BridgePassword field
+      *is not in use*, but if it had been, the timing
+      behavior of strcmp() might have allowed an adversary to guess the
+      BridgePassword value, and enumerate the bridges. Bugfix on
+      0.2.0.14-alpha. Fixes bug 5543.
+
diff --git a/src/or/config.c b/src/or/config.c
index c3e285d..1e7bd58 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -713,6 +713,7 @@ or_options_free(or_options_t *options)
     return;
 
   routerset_free(options->_ExcludeExitNodesUnion);
+  tor_free(options->BridgePassword_AuthDigest);
   config_free(&options_format, options);
 }
 
@@ -1298,6 +1299,24 @@ options_act(or_options_t *old_options)
   /* Change the cell EWMA settings */
   cell_ewma_set_scale_factor(options, networkstatus_get_latest_consensus());
 
+  /* Update the BridgePassword's hashed version as needed.  We store this as a
+   * digest so that we can do side-channel-proof comparisons on it.
+   */
+  if (options->BridgePassword) {
+    char *http_authenticator;
+    http_authenticator = alloc_http_authenticator(options->BridgePassword);
+    if (!http_authenticator) {
+      log_warn(LD_BUG, "Unable to allocate HTTP authenticator. Not setting "
+               "BridgePassword.");
+      return -1;
+    }
+    options->BridgePassword_AuthDigest = tor_malloc(DIGEST256_LEN);
+    crypto_digest256(options->BridgePassword_AuthDigest,
+                     http_authenticator, strlen(http_authenticator),
+                     DIGEST_SHA256);
+    tor_free(http_authenticator);
+  }
+
   /* Check for transitions that need action. */
   if (old_options) {
     int revise_trackexithosts = 0;
diff --git a/src/or/directory.c b/src/or/directory.c
index e3cc70f..9bc58e5 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -3069,22 +3069,23 @@ directory_handle_command_get(dir_connection_t *conn, const char *headers,
   }
 
   if (options->BridgeAuthoritativeDir &&
-      options->BridgePassword &&
+      options->BridgePassword_AuthDigest &&
       connection_dir_is_encrypted(conn) &&
       !strcmp(url,"/tor/networkstatus-bridges")) {
     char *status;
-    char *secret = alloc_http_authenticator(options->BridgePassword);
+    char digest[DIGEST256_LEN];
 
     header = http_get_header(headers, "Authorization: Basic ");
+    if (header)
+      crypto_digest256(digest, header, strlen(header), DIGEST_SHA256);
 
     /* now make sure the password is there and right */
-    if (!header || strcmp(header, secret)) {
+    if (!header ||
+        tor_memneq(digest, options->BridgePassword_AuthDigest, DIGEST256_LEN)) {
       write_http_status_line(conn, 404, "Not found");
-      tor_free(secret);
       tor_free(header);
       goto done;
     }
-    tor_free(secret);
     tor_free(header);
 
     /* all happy now. send an answer. */
diff --git a/src/or/or.h b/src/or/or.h
index eecd375..92592e5 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -2489,10 +2489,11 @@ typedef struct {
                                * that aggregates bridge descriptors? */
 
   /** If set on a bridge authority, it will answer requests on its dirport
-   * for bridge statuses -- but only if the requests use this password.
-   * If set on a bridge user, request bridge statuses, and use this password
-   * when doing so. */
+   * for bridge statuses -- but only if the requests use this password. */
   char *BridgePassword;
+  /** If BridgePassword is set, this is a SHA256 digest of the basic http
+   * authenticator for it. */
+  char *BridgePassword_AuthDigest;
 
   int UseBridges; /**< Boolean: should we start all circuits with a bridge? */
   config_line_t *Bridges; /**< List of bootstrap bridge addresses. */