add cmd line option to restrict timetravel, minor code cleanup of keystate logic

5 files changed, 213 insertions, 106 deletions

diff --git a/src/exchange/taler-exchange-httpd.c b/src/exchange/taler-exchange-httpd.c
index 4095d00f..0754163b 100644
--- a/src/exchange/taler-exchange-httpd.c
+++ b/src/exchange/taler-exchange-httpd.c
@@ -83,6 +83,14 @@ char *TEH_exchange_directory;
 char *TEH_revocation_directory;
 
 /**
+ * Are clients allowed to request /keys for times other than the
+ * current time? Allowing this could be abused in a DoS-attack
+ * as building new /keys responses is expensive. Should only be
+ * enabled for testcases, development and test systems.
+ */
+int TEH_allow_keys_timetravel;
+
+/**
  * The exchange's configuration (global)
  */
 struct GNUNET_CONFIGURATION_Handle *TEH_cfg;
@@ -1183,6 +1191,10 @@ main (int argc,
   char *logfile = NULL;
   int connection_close = GNUNET_NO;
   const struct GNUNET_GETOPT_CommandLineOption options[] = {
+    GNUNET_GETOPT_option_flag ('a',
+                               "allow-timetravel",
+                               "allow clients to request /keys for arbitrary timestamps (for testing and development only)",
+                               &TEH_allow_keys_timetravel),
     GNUNET_GETOPT_option_flag ('C',
                                "connection-close",
                                "force HTTP connections to be closed after each request",
diff --git a/src/exchange/taler-exchange-httpd.h b/src/exchange/taler-exchange-httpd.h
index 137cee72..512fae8f 100644
--- a/src/exchange/taler-exchange-httpd.h
+++ b/src/exchange/taler-exchange-httpd.h
@@ -44,6 +44,14 @@ extern struct GNUNET_CONFIGURATION_Handle *TEH_cfg;
 extern char *TEH_exchange_directory;
 
 /**
+ * Are clients allowed to request /keys for times other than the
+ * current time? Allowing this could be abused in a DoS-attack
+ * as building new /keys responses is expensive. Should only be
+ * enabled for testcases, development and test systems.
+ */
+extern int TEH_allow_keys_timetravel;
+
+/**
  * Main directory with revocation data.
  */
 extern char *TEH_revocation_directory;
diff --git a/src/exchange/taler-exchange-httpd_keystate.c b/src/exchange/taler-exchange-httpd_keystate.c
index 5bc89324..4117c13a 100644
--- a/src/exchange/taler-exchange-httpd_keystate.c
+++ b/src/exchange/taler-exchange-httpd_keystate.c
@@ -121,7 +121,7 @@ struct KeysResponseData
 {
 
   /**
-   * Response to return if the client supports (gzip) compression.
+   * Response to return if the client supports (deflate) compression.
    */
   struct MHD_Response *response_compressed;
 
@@ -212,14 +212,14 @@ struct ResponseFactoryContext
   struct TEH_KS_StateHandle *key_state;
 
   /**
-   * Length of the @e denomkey_array.
+   * Time stamp used as "now".
    */
-  unsigned int denomkey_array_length;
+  struct GNUNET_TIME_Absolute now;
 
   /**
-   * Time stamp used as "now".
+   * Length of the @e denomkey_array.
    */
-  struct GNUNET_TIME_Absolute now;
+  unsigned int denomkey_array_length;
 };
 
 
@@ -249,7 +249,7 @@ struct TEH_KS_StateHandle
   struct GNUNET_CONTAINER_MultiHashMap *revoked_map;
 
   /**
-   * Sorted array of responses to /keys (sorted by cherry-picking date) of
+   * Sorted array of responses to /keys (MUST be sorted by cherry-picking date) of
    * length @e krd_array_length;
    */
   struct KeysResponseData *krd_array;
@@ -309,6 +309,12 @@ static struct TEH_KS_StateHandle *internal_key_state;
  */
 static pthread_mutex_t internal_key_state_mutex = PTHREAD_MUTEX_INITIALIZER;
 
+/**
+ * Configuration value LOOKAHEAD_PROVIDE that says for how far in the
+ * future we want to provide exchange key information to clients.
+ */
+static struct GNUNET_TIME_Relative conf_duration_provide;
+
 
 /* ************************** Clean up logic *********************** */
 
@@ -368,6 +374,11 @@ destroy_response_builder (struct ResponseBuilderContext *rbc)
     json_decref (rbc->auditors_array);
     rbc->auditors_array = NULL;
   }
+  if (NULL != rbc->hash_context)
+  {
+    GNUNET_CRYPTO_hash_context_abort (rbc->hash_context);
+    rbc->hash_context = NULL;
+  }
 }
 
 
@@ -447,7 +458,7 @@ ks_free (struct TEH_KS_StateHandle *key_state)
 /**
  * Pipe used for signaling reloading of our key state.
  */
-static int reload_pipe[2];
+static int reload_pipe[2] = { -1, -1 };
 
 
 /**
@@ -458,23 +469,15 @@ static int reload_pipe[2];
 static void
 handle_signal (int signal_number)
 {
-  ssize_t res;
-  char c = signal_number;
-
-  res = write (reload_pipe[1],
-               &c,
-               1);
-  if ( (res < 0) &&
-       (EINTR != errno) )
-  {
-    GNUNET_break (0);
-    return;
-  }
-  if (0 == res)
-  {
-    GNUNET_break (0);
-    return;
-  }
+  char c = (char) signal_number; /* never seen a signal_number > 127 on any platform */
+
+  (void) ! write (reload_pipe[1],
+                  &c,
+                  1);
+  /* While one might like to "handle errors" here, even logging via fprintf()
+     isn't safe inside of a signal handler. So there is nothing we safely CAN
+     do. OTOH, also very little that can go wrong in pratice. Calling _exit()
+     on errors might be a possibility, but that might do more harm than good. *///
 }
 
 
@@ -482,11 +485,10 @@ handle_signal (int signal_number)
 
 
 /**
- * Convert the public part of a denomination key issue to a JSON
- * object.
+ * Convert the public part of denomination key data to a JSON object.
  *
- * @param pk public key of the denomination key
- * @param dki the denomination key issue
+ * @param pk public key of the denomination
+ * @param dki the denomination key issue information
  * @return a JSON object describing the denomination key isue (public part)
  */
 static json_t *
@@ -511,7 +513,9 @@ denom_key_issue_to_json (
   TALER_amount_ntoh (&fee_refund,
                      &dki->properties.fee_refund);
   return
-    json_pack ("{s:o, s:o, s:o, s:o, s:o, s:o, s:o, s:o, s:o, s:o, s:o}",
+    json_pack ("{s:o, s:o, s:o, s:o, s:o,"
+               " s:o, s:o, s:o, s:o, s:o,"
+               " s:o}",
                "master_sig",
                GNUNET_JSON_from_data_auto (&dki->signature),
                "stamp_start",
@@ -526,6 +530,7 @@ denom_key_issue_to_json (
                "stamp_expire_legal",
                GNUNET_JSON_from_time_abs (GNUNET_TIME_absolute_ntoh (
                                             dki->properties.expire_legal)),
+               /* 5 entries until here */
                "denom_pub",
                GNUNET_JSON_from_rsa_public_key (pk->rsa_public_key),
                "value",
@@ -536,6 +541,7 @@ denom_key_issue_to_json (
                TALER_JSON_from_amount (&fee_deposit),
                "fee_refresh",
                TALER_JSON_from_amount (&fee_refresh),
+               /* 10 entries until here */
                "fee_refund",
                TALER_JSON_from_amount (&fee_refund));
 }
@@ -553,20 +559,37 @@ static int
 store_in_map (struct GNUNET_CONTAINER_MultiHashMap *map,
               const struct TALER_EXCHANGEDB_DenominationKey *dki)
 {
-  struct TALER_EXCHANGEDB_DenominationKey *d2;
-  int res;
-
+  /* First, we verify that the @a dki is actually well-formed.  While it comes
+     from our own hard disk, there is the possibility of missconfiguration
+     (i.e. bogus file in the directory), or that the administrator used the
+     wrong master public key, and we should not accept entries that are not
+     well-formed. *///
   {
     const struct TALER_EXCHANGEDB_DenominationKeyInformationP *dkip;
     struct TALER_DenominationKeyValidityPS denom_key_issue;
 
     dkip = &dki->issue;
     denom_key_issue = dkip->properties;
+    /* The above is straight from our disk. We should not trust
+       that it is well-formed, so we setup crucial fields ourselves. */
     denom_key_issue.purpose.purpose
       = htonl (TALER_SIGNATURE_MASTER_DENOMINATION_KEY_VALIDITY);
     denom_key_issue.purpose.size
       = htonl (sizeof (struct TALER_DenominationKeyValidityPS));
     denom_key_issue.master = TEH_master_public_key;
+
+    /* Check that the data we read matches our expectations */
+    if (0 !=
+        GNUNET_memcmp (&denom_key_issue,
+                       &dkip->properties))
+    {
+      GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+                  "Invalid fields in denomination key `%s'\n",
+                  GNUNET_h2s (&dkip->properties.denom_hash));
+      return GNUNET_SYSERR;
+    }
+
+    /* Also check the signature by the master public key */
     if (GNUNET_SYSERR ==
         GNUNET_CRYPTO_eddsa_verify (
           TALER_SIGNATURE_MASTER_DENOMINATION_KEY_VALIDITY,
@@ -581,27 +604,36 @@ store_in_map (struct GNUNET_CONTAINER_MultiHashMap *map,
     }
   }
 
-  d2 = GNUNET_new (struct TALER_EXCHANGEDB_DenominationKey);
-  d2->issue = dki->issue;
-  if (NULL != dki->denom_priv.rsa_private_key)
-    d2->denom_priv.rsa_private_key
-      = GNUNET_CRYPTO_rsa_private_key_dup (dki->denom_priv.rsa_private_key);
-  d2->denom_pub.rsa_public_key
-    = GNUNET_CRYPTO_rsa_public_key_dup (dki->denom_pub.rsa_public_key);
-  res = GNUNET_CONTAINER_multihashmap_put (map,
+  /* We need to make a deep copy of the @a dki, as the original was allocated
+     elsewhere and will be freed by the caller. */
+  {
+    struct TALER_EXCHANGEDB_DenominationKey *d2;
+
+    d2 = GNUNET_new (struct TALER_EXCHANGEDB_DenominationKey);
+    d2->issue = dki->issue;
+    if (GNUNET_OK !=
+        GNUNET_CONTAINER_multihashmap_put (map,
                                            &d2->issue.properties.denom_hash,
                                            d2,
-                                           GNUNET_CONTAINER_MULTIHASHMAPOPTION_UNIQUE_ONLY);
-  if (GNUNET_OK != res)
-  {
-    GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
-                "Duplicate denomination key `%s'\n",
-                GNUNET_h2s (&d2->issue.properties.denom_hash));
-    if (NULL != d2->denom_priv.rsa_private_key)
-      GNUNET_CRYPTO_rsa_private_key_free (d2->denom_priv.rsa_private_key);
-    GNUNET_CRYPTO_rsa_public_key_free (d2->denom_pub.rsa_public_key);
-    GNUNET_free (d2);
-    return GNUNET_NO;
+                                           GNUNET_CONTAINER_MULTIHASHMAPOPTION_UNIQUE_ONLY))
+    {
+      GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+                  "Duplicate denomination key `%s'\n",
+                  GNUNET_h2s (&d2->issue.properties.denom_hash));
+      GNUNET_free (d2);
+      return GNUNET_NO;
+    }
+
+    /* finish *deep* part of deep copy */
+    if (NULL != dki->denom_priv.rsa_private_key)
+    {
+      /* we might be past the withdraw period, so private key could have been deleted,
+         so only try to (deep) copy if non-NULL. */
+      d2->denom_priv.rsa_private_key
+        = GNUNET_CRYPTO_rsa_private_key_dup (dki->denom_priv.rsa_private_key);
+    }
+    d2->denom_pub.rsa_public_key
+      = GNUNET_CRYPTO_rsa_public_key_dup (dki->denom_pub.rsa_public_key);
   }
   return GNUNET_OK;
 }
@@ -625,33 +657,6 @@ struct AddRevocationContext
 
 
 /**
- * Get the relative time value that describes how
- * far in the future do we want to provide coin keys.
- *
- * @return the provide duration
- */
-static struct GNUNET_TIME_Relative
-TALER_EXCHANGE_conf_duration_provide (void)
-{
-  struct GNUNET_TIME_Relative rel;
-
-  if (GNUNET_OK !=
-      GNUNET_CONFIGURATION_get_value_time (TEH_cfg,
-                                           "exchange",
-                                           "LOOKAHEAD_PROVIDE",
-                                           &rel))
-  {
-    GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
-                               "exchange",
-                               "LOOKAHEAD_PROVIDE",
-                               "time value required");
-    GNUNET_assert (0);
-  }
-  return rel;
-}
-
-
-/**
  * Execute transaction to add revocations.
  *
  * @param cls closure with the `struct AddRevocationContext *`
@@ -771,7 +776,7 @@ reload_keys_denom_iter (void *cls,
   }
 
   horizon = GNUNET_TIME_absolute_add (rfc->now,
-                                      TALER_EXCHANGE_conf_duration_provide ());
+                                      conf_duration_provide);
   start = GNUNET_TIME_absolute_ntoh (dki->issue.properties.start);
   if (start.abs_value_us > horizon.abs_value_us)
   {
@@ -944,8 +949,7 @@ reload_keys_sign_iter (
   struct GNUNET_TIME_Absolute now;
   struct GNUNET_TIME_Absolute horizon;
 
-  horizon = GNUNET_TIME_relative_to_absolute (
-    TALER_EXCHANGE_conf_duration_provide ());
+  horizon = GNUNET_TIME_relative_to_absolute (conf_duration_provide);
   if (GNUNET_TIME_absolute_ntoh (ski->issue.start).abs_value_us >
       horizon.abs_value_us)
   {
@@ -1127,7 +1131,9 @@ initialize_denomkey_array (void *cls,
 
 /**
  * Comparator used to sort the `struct DenominationKeyEntry` array
- * by the validity period's starting time of the keys.
+ * by the validity period's starting time of the keys.  Must match
+ * the expected sorting by cherry_pick_date (which is based on the
+ * issue.properties.start) used in #krd_search_comparator.
  *
  * @param k1 a `struct DenominationKeyEntry *`
  * @param k2 a `struct DenominationKeyEntry *`
@@ -1460,7 +1466,6 @@ build_keys_response (const struct ResponseFactoryContext *rfc,
                                                  &free_auditor_entry,
                                                  NULL);
           GNUNET_CONTAINER_multihashmap_destroy (auditors);
-          GNUNET_CRYPTO_hash_context_abort (rbc.hash_context);
           return GNUNET_SYSERR;
         }
       }
@@ -1822,6 +1827,8 @@ make_fresh_key_state (struct GNUNET_TIME_Absolute now)
 
     if (last.abs_value_us == d.abs_value_us)
       continue;
+    /* must be monotonically increasing as per qsort() call above: */
+    GNUNET_assert (last.abs_value_us < d.abs_value_us);
     last = d;
     off++;
   }
@@ -2277,11 +2284,31 @@ static struct GNUNET_SIGNAL_Context *sigchld;
 
 
 /**
- * Setup initial #internal_key_state.
+ * Setup initial #internal_key_state and our signal handlers.
  */
 int
 TEH_KS_init (void)
 {
+  if (GNUNET_OK !=
+      GNUNET_CONFIGURATION_get_value_time (TEH_cfg,
+                                           "exchange",
+                                           "LOOKAHEAD_PROVIDE",
+                                           &conf_duration_provide))
+  {
+    GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+                               "exchange",
+                               "LOOKAHEAD_PROVIDE",
+                               "time value required");
+    return GNUNET_SYSERR;
+  }
+  if (0 == conf_duration_provide.rel_value_us)
+  {
+    GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+                               "exchange",
+                               "LOOKAHEAD_PROVIDE",
+                               "value cannot be zero");
+    return GNUNET_SYSERR;
+  }
   if (0 != pipe (reload_pipe))
   {
     GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR,
@@ -2312,7 +2339,7 @@ TEH_KS_init (void)
 
 
 /**
- * Finally release #internal_key_state.
+ * Finally release #internal_key_state and our signal handlers.
  */
 void
 TEH_KS_free (void)
@@ -2322,18 +2349,43 @@ TEH_KS_free (void)
   /* Note: locking is no longer be required, as we are again
      single-threaded. */
   ks = internal_key_state;
-  if (NULL == ks)
-    return;
-  GNUNET_assert (1 == ks->refcnt);
-  ks->refcnt--;
-  ks_free (ks);
-  GNUNET_SIGNAL_handler_uninstall (sigusr1);
-  GNUNET_SIGNAL_handler_uninstall (sigterm);
-  GNUNET_SIGNAL_handler_uninstall (sigint);
-  GNUNET_SIGNAL_handler_uninstall (sighup);
-  GNUNET_SIGNAL_handler_uninstall (sigchld);
-  GNUNET_break (0 == close (reload_pipe[0]));
-  GNUNET_break (0 == close (reload_pipe[1]));
+  if (NULL != ks)
+  {
+    GNUNET_assert (1 == ks->refcnt);
+    ks->refcnt--;
+    ks_free (ks);
+  }
+  if (NULL != sigusr1)
+  {
+    GNUNET_SIGNAL_handler_uninstall (sigusr1);
+    sigusr1 = NULL;
+  }
+  if (NULL != sigterm)
+  {
+    GNUNET_SIGNAL_handler_uninstall (sigterm);
+    sigterm = NULL;
+  }
+  if (NULL != sigint)
+  {
+    GNUNET_SIGNAL_handler_uninstall (sigint);
+    sigint = NULL;
+  }
+  if (NULL != sighup)
+  {
+    GNUNET_SIGNAL_handler_uninstall (sighup);
+    sighup = NULL;
+  }
+  if (NULL != sigchld)
+  {
+    GNUNET_SIGNAL_handler_uninstall (sigchld);
+    sigchld = NULL;
+  }
+  if (-1 != reload_pipe[0])
+  {
+    GNUNET_break (0 == close (reload_pipe[0]));
+    GNUNET_break (0 == close (reload_pipe[1]));
+    reload_pipe[0] = reload_pipe[1] = -1;
+  }
 }
 
 
@@ -2356,10 +2408,11 @@ TEH_KS_sign (const struct GNUNET_CRYPTO_EccSignaturePurpose *purpose,
   key_state = TEH_KS_acquire (GNUNET_TIME_absolute_get ());
   if (NULL == key_state)
   {
-    /* This *can* happen if the exchange's keys are
-       not properly maintained. */
+    /* This *can* happen if the exchange's keys are not properly maintained
+       (i.e. administrator forgets to provision us with non-expired signing
+       keys or to send signal to reload keys after provisioning). */
     GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
-                "Cannot sign request, no valid keys available\n");
+                "Cannot sign request, no valid signing keys available. Were they properly provisioned and did you signal the exchange to reload the keys?\n");
     return GNUNET_SYSERR;
   }
   *pub = key_state->current_sign_key_issue.issue.signkey_pub;
@@ -2374,8 +2427,8 @@ TEH_KS_sign (const struct GNUNET_CRYPTO_EccSignaturePurpose *purpose,
 
 
 /**
- * Comparator used for a binary search for @a key in the
- * `struct KeysResponseData` array.
+ * Comparator used for a binary search by cherry_pick_date for @a key in the
+ * `struct KeysResponseData` array. See libc's qsort() and bsearch() functions.
  *
  * @param key pointer to a `struct GNUNET_TIME_Absolute`
  * @param value pointer to a `struct KeysResponseData` array entry
@@ -2437,6 +2490,10 @@ TEH_handler_keys (const struct TEH_RequestHandler *rh,
                                          TALER_EC_KEYS_HAVE_NOT_NUMERIC,
                                          "last_issue_date");
     }
+    /* The following multiplication may overflow; but this should not really
+       be a problem, as giving back 'older' data than what the client asks for
+       (given that the client asks for data in the distant future) is not
+       problematic */
     last_issue_date.abs_value_us = (uint64_t) cherrypickn * 1000000LLU;
   }
   else
@@ -2458,11 +2515,26 @@ TEH_handler_keys (const struct TEH_RequestHandler *rh,
     {
       GNUNET_break_op (0);
       return TALER_MHD_reply_with_error (connection,
-                                         MHD_HTTP_BAD_REQUEST,
+                                         MHD_HTTP_FORBIDDEN,
                                          TALER_EC_KEYS_HAVE_NOT_NUMERIC,
                                          "now");
     }
-    now.abs_value_us = (uint64_t) fakenown * 1000000LLU;
+    if (TEH_allow_keys_timetravel)
+    {
+      /* The following multiplication may overflow; but this should not really
+         be a problem, as giving back 'older' data than what the client asks for
+         (given that the client asks for data in the distant future) is not
+         problematic */
+      now.abs_value_us = (uint64_t) fakenown * 1000000LLU;
+    }
+    else
+    {
+      /* Option not allowed by configuration */
+      return TALER_MHD_reply_with_error (connection,
+                                         MHD_HTTP_BAD_REQUEST,
+                                         TALER_EC_KEYS_TIMETRAVEL_FORBIDDEN,
+                                         "timetravel not allowed by this exchange");
+    }
   }
   GNUNET_log (GNUNET_ERROR_TYPE_INFO,
               "Handling request for /keys (%s/%s)\n",
@@ -2474,11 +2546,14 @@ TEH_handler_keys (const struct TEH_RequestHandler *rh,
     key_state = TEH_KS_acquire (now);
     if (NULL == key_state)
     {
-      TALER_LOG_ERROR ("Lacking keys to operate\n");
+      /* Maybe client picked time stamp too far in the future?  In that case,
+         #MHD_HTTP_INTERNAL_SERVER_ERROR might be missleading, could be more like a
+         NOT_FOUND situation. But, OTOH, for 'sane' clients it is more likely
+         to be our fault, so let's speculatively assume we are to blame ;-) */
       return TALER_MHD_reply_with_error (connection,
                                          MHD_HTTP_INTERNAL_SERVER_ERROR,
                                          TALER_EC_EXCHANGE_BAD_CONFIGURATION,
-                                         "no keys");
+                                         "no keys for requested time");
     }
     krd = bsearch (&last_issue_date,
                    key_state->krd_array,
@@ -2501,6 +2576,10 @@ TEH_handler_keys (const struct TEH_RequestHandler *rh,
     }
     if (NULL == krd)
     {
+      /* Maybe client picked time stamp too far in the future?  In that case,
+         "INTERNAL_SERVER_ERROR" might be missleading, could be more like a
+         NOT_FOUND situation. But, OTOH, for 'sane' clients it is more likely
+         to be our fault, so let's speculatively assume we are to blame ;-) *///
       GNUNET_break (0);
       TEH_KS_release (key_state);
       return TALER_MHD_reply_with_error (connection,
diff --git a/src/include/taler_error_codes.h b/src/include/taler_error_codes.h
index 9a7c544b..0969261a 100644
--- a/src/include/taler_error_codes.h
+++ b/src/include/taler_error_codes.h
@@ -1052,6 +1052,13 @@ enum TALER_ErrorCode
   TALER_EC_KEYS_MISSING = 1901,
 
   /**
+   * This exchange does not allow clients to request /keys for times
+   * other than the current (exchange) time. This reponse is provied
+   * with an HTTP status code of MHD_HTTP_FORBIDDEN.
+   */
+  TALER_EC_KEYS_TIMETRAVEL_FORBIDDEN = 1902,
+
+  /**
    * The backend could not find the merchant instance specified in the
    * request.   This response is provided with HTTP status code
    * MHD_HTTP_NOT_FOUND.
diff --git a/src/testing/testing_api_helpers_exchange.c b/src/testing/testing_api_helpers_exchange.c
index 967a4efb..9f571684 100644
--- a/src/testing/testing_api_helpers_exchange.c
+++ b/src/testing/testing_api_helpers_exchange.c
@@ -762,6 +762,7 @@ TALER_TESTING_setup_with_exchange_cfg (void *cls,
                                        NULL, NULL, NULL,
                                        "taler-exchange-httpd",
                                        "taler-exchange-httpd",
+                                       "-a", /* some tests may need timetravel */
                                        "-c", setup_ctx->config_filename,
                                        NULL);