IPcheck: unit + integration tests; fix exempt-address crash, rehash, and 16-bit/unsigned arithmetic bugs

include/IPcheck.h

@@ -13,6 +13,11 @@
 struct Client;
 struct irc_in_addr;
 
+/** Results of IPcheck_local_connect(). */
+#define IPCHECK_REFUSED 0 /**< Too many recent connections: refuse. */
+#define IPCHECK_COUNTED 1 /**< Accepted and recorded; mark client IPChecked. */
+#define IPCHECK_EXEMPT  2 /**< Accepted, address exempt; not recorded. */
+
 /*
  * Prototypes
  */

ircd/IPcheck.c

@@ -69,8 +69,12 @@ struct IPRegistry48 {
 #define IP_REGISTRY_TABLE_SIZE 0x10000
 /** Report current time for tracking in IPRegistryEntry::last_connect. */
 #define NOW ((unsigned short)(CurrentTime & 0xffff))
-/** Time from \a x until now, in seconds. */
-#define CONNECTED_SINCE(x) (NOW - (x))
+/** Time from \a x until now, in seconds.  Both operands are 16-bit
+ * timestamps, so reduce the difference modulo 2^16 as well: a plain
+ * subtraction goes negative once CurrentTime crosses a multiple of 65536
+ * (every 18.2 hours) and the period, expiry and free-target arithmetic
+ * misbehave until the entry is next stamped. */
+#define CONNECTED_SINCE(x) ((unsigned short)(NOW - (x)))
 
 /** Macro for easy access to configured IPcheck clone limit. */
 #define IPCHECK_CLONE_LIMIT feature_int(FEAT_IPCHECK_CLONE_LIMIT)
@@ -433,15 +437,19 @@ static int ip_registry_is_exempt(const struct irc_in_addr *addr)
  * separated by no more than IPCHECK_CLONE_PERIOD seconds.
  * @param[in] addr Address of client.
  * @param[out] next_target_out Receives time to grant another free target.
- * @return Non-zero if the connection is permitted, zero if denied.
+ * @return IPCHECK_REFUSED if denied, IPCHECK_COUNTED if permitted and
+ *   recorded in the registry, IPCHECK_EXEMPT if permitted because the
+ *   address is exempt (nothing recorded; the caller must not mark the
+ *   client IPChecked, or its disconnect would decrement a count it never
+ *   incremented).
  */
 static int ip_registry_check_local(const struct irc_in_addr *addr, time_t* next_target_out)
 {
   struct IPRegistryEntry* entry;
   unsigned int free_targets = STARTTARGETS;
 
   if (ip_registry_is_exempt(addr)) {
-    return 1;
+    return IPCHECK_EXEMPT;
   }
 
   entry = ip_registry_find(addr);
@@ -502,7 +510,9 @@ static int ip_registry_check_local(const struct irc_in_addr *addr, time_t* next_
 
   if (entry->attempts < IPCHECK_CLONE_LIMIT) {
     if (next_target_out)
-      *next_target_out = CurrentTime - (TARGET_DELAY * free_targets - 1);
+      /* free_targets is unsigned: with none left, TARGET_DELAY * 0 - 1
+       * must be -1 (next target in one second), not UINT_MAX. */
+      *next_target_out = CurrentTime - ((time_t)TARGET_DELAY * free_targets - 1);
   }
 #ifndef NOTHROTTLE
   else if ((CurrentTime - cli_since(&me)) > IPCHECK_CLONE_DELAY) {
@@ -535,10 +545,6 @@ static int ip_registry_check_remote(struct Client* cptr, int is_burst)
 {
   struct IPRegistryEntry* entry;
 
-  /*
-   * Mark that we did add/update an IPregistry entry
-   */
-  SetIPChecked(cptr);
   if (!irc_in_addr_valid(&cli_ip(cptr))) {
     Debug((DEBUG_DNS, "IPcheck accepting remote connection from invalid %s.", ircd_ntoa(&cli_ip(cptr))));
     return 1;
@@ -548,6 +554,13 @@ static int ip_registry_check_remote(struct Client* cptr, int is_burst)
     return 1;
   }
 
+  /*
+   * Mark that we did add/update an IPregistry entry.  Only now: an exempt
+   * or unroutable address is not counted, and IPcheck_disconnect() must not
+   * decrement a count that was never incremented.
+   */
+  SetIPChecked(cptr);
+
   if (!irc_in_addr_is_ipv4(&cli_ip(cptr))) {
     struct IPRegistry48* entry_48 = ip_48_find(&cli_ip(cptr));
     if (CONNECTED_SINCE(entry_48->last_connect) > IPCHECK_48_CLONE_PERIOD)
@@ -569,6 +582,7 @@ static int ip_registry_check_remote(struct Client* cptr, int is_burst)
   }
   /* Avoid overflowing the connection counter. */
   if (0 == ++entry->connected) {
+    entry->connected--;
     Debug((DEBUG_DNS, "IPcheck refusing remote connection from %s: counter overflow.", ircd_ntoa(&entry->addr)));
     return 0;
   }
@@ -732,7 +746,9 @@ static int ip_registry_count(const struct irc_in_addr *addr)
 /** Check whether a client is allowed to connect locally.
  * @param[in] a Address of client.
  * @param[out] next_target_out Receives time to grant another free target.
- * @return Non-zero if the connection is permitted, zero if denied.
+ * @return IPCHECK_REFUSED (zero) if denied; IPCHECK_COUNTED if permitted
+ *   and recorded (the caller marks the client IPChecked); IPCHECK_EXEMPT
+ *   if permitted without being recorded (do not mark it).
  */
 int IPcheck_local_connect(const struct irc_in_addr *a, time_t* next_target_out)
 {

ircd/m_nick.c

@@ -248,7 +248,8 @@ int m_nick(struct Client* cptr, struct Client* sptr, int parc, char* parv[])
    */
   if (IsUnknown(acptr) && MyConnect(acptr)) {
     ++ServerStats->is_reg_collided;
-    IPcheck_connect_fail(acptr, 0);
+    if (IsIPChecked(acptr))
+      IPcheck_connect_fail(acptr, 0);
     exit_client(cptr, acptr, &me, "Overridden by other sign on");
     return set_nick_name(cptr, sptr, nick, parc, parv);
   }
@@ -381,7 +382,8 @@ int ms_nick(struct Client* cptr, struct Client* sptr, int parc, char* parv[])
   if (IsUnknown(acptr) && MyConnect(acptr))
   {
     ++ServerStats->is_reg_collided;
-    IPcheck_connect_fail(acptr, 0);
+    if (IsIPChecked(acptr))
+      IPcheck_connect_fail(acptr, 0);
     exit_client(cptr, acptr, &me, "Overridden by other sign on");
     return set_nick_name(cptr, sptr, nick, parc, parv);
   }

ircd/s_auth.c

@@ -747,7 +747,8 @@ static int preregister_user(struct Client *cptr)
     /* Can this ever happen? */
   case ACR_BAD_SOCKET:
     ++ServerStats->is_bad_socket;
-    IPcheck_connect_fail(cptr, 0);
+    if (IsIPChecked(cptr))
+      IPcheck_connect_fail(cptr, 0);
     return exit_client(cptr, cptr, &me, "Unknown error -- Try again");
   }
   return 0;
@@ -1544,11 +1545,17 @@ int auth_spoof_user(struct AuthRequest *auth, const char *username, const char *
     return 1;
   if (!ipmask_parse(ip, &cli_ip(sptr), NULL))
     return 2;
-  if (!IPcheck_local_connect(&cli_ip(sptr), &next_target)) {
+  switch (IPcheck_local_connect(&cli_ip(sptr), &next_target)) {
+  case IPCHECK_REFUSED:
     ++ServerStats->is_throttled;
     return exit_client(sptr, sptr, &me, "Your host is trying to (re)connect too fast -- throttled");
+  case IPCHECK_COUNTED:
+    SetIPChecked(sptr);
+    break;
+  default: /* IPCHECK_EXEMPT: accepted, not recorded */
+    ClearIPChecked(sptr);
+    break;
   }
-  SetIPChecked(sptr);
 
   if (next_target)
     cli_nexttarget(sptr) = next_target;
@@ -2218,9 +2225,11 @@ static int iauth_cmd_ip_address(struct IAuth *iauth, struct Client *cli,
   if (!irc_in_addr_valid(&auth->original))
     memcpy(&auth->original, &cli_ip(cli), sizeof(auth->original));
 
-  /* Undo original IP connection in IPcheck. */
-  IPcheck_connect_fail(cli, 1);
-  ClearIPChecked(cli);
+  /* Undo original IP connection in IPcheck (unless it was exempt). */
+  if (IsIPChecked(cli)) {
+    IPcheck_connect_fail(cli, 1);
+    ClearIPChecked(cli);
+  }
 
   /* Update the IP and charge them as a remote connect. */
   memcpy(&cli_ip(cli), &addr, sizeof(cli_ip(cli)));

ircd/s_bsd.c

@@ -549,6 +549,7 @@ void add_connection(struct Listener* listener, int fd) {
   struct irc_sockaddr addr;
   struct Client      *new_client;
   time_t             next_target = 0;
+  int                ipcheck;
   void               *tls;
 
   const char* const throttle_message =
@@ -612,14 +613,16 @@ void add_connection(struct Listener* listener, int fd) {
      * known at handshake; the socket peer is a Cloudflare edge node.
      */
     if (!(listener_websocket(listener) && listener_cloudflare(listener))) {
-      if (!IPcheck_local_connect(&addr.addr, &next_target))
+      ipcheck = IPcheck_local_connect(&addr.addr, &next_target);
+      if (ipcheck == IPCHECK_REFUSED)
       {
         ++ServerStats->is_throttled;
         write(fd, throttle_message, strlen(throttle_message));
         close(fd);
         return;
       }
-      SetIPChecked(new_client);
+      if (ipcheck == IPCHECK_COUNTED)
+        SetIPChecked(new_client);
     }
   }
 

ircd/s_conf.c

@@ -1011,6 +1011,7 @@ int read_configuration_file(void)
   conf_error = 0;
   feature_unmark(); /* unmark all features for resetting later */
   clear_nameservers(); /* clear previous list of DNS servers */
+  IPcheck_clear_config(); /* IPCheck exemptions, in case the block is gone */
   if (!init_lexer())
     return 0;
   yyparse();

ircd/s_user.c

@@ -394,7 +394,8 @@ int register_user(struct Client *cptr, struct Client *sptr)
                            cli_sock_ip(sptr), get_client_class(sptr),
                            cli_info(sptr), NumNick(cptr) /* two %s's */);
 
-    IPcheck_connect_succeeded(sptr);
+    if (IsIPChecked(sptr))
+      IPcheck_connect_succeeded(sptr);
   }
   else {
     struct Client *acptr = user->server;

ircd/test/Makefile.am

@@ -1,14 +1,18 @@
 AM_CPPFLAGS = -I$(top_srcdir)/include -I../..
 AM_CFLAGS = -g -Wall
 
-check_PROGRAMS = cidr_lookups_t ircd_chattr_t ircd_in_addr_t ircd_match_t ircd_string_t msgq_excise_t
+check_PROGRAMS = cidr_lookups_t ipcheck_t ircd_chattr_t ircd_in_addr_t ircd_match_t ircd_string_t msgq_excise_t
 
 TESTS = $(check_PROGRAMS)
 
 cidr_lookups_t_CPPFLAGS = $(AM_CPPFLAGS) -DIRCU2_BUILD
 cidr_lookups_t_SOURCES = cidr_lookups_t.c test_stub.c
 cidr_lookups_t_LDADD = ../cidr_lookups.o ../ircd_alloc.o ../ircd_string.o
 
+ipcheck_t_CPPFLAGS = $(AM_CPPFLAGS) -DIRCU2_BUILD
+ipcheck_t_SOURCES = ipcheck_t.c test_stub.c
+ipcheck_t_LDADD = ../IPcheck.o ../ircd_alloc.o ../ircd_string.o ../match.o
+
 msgq_excise_t_CPPFLAGS = $(AM_CPPFLAGS) -DIRCU2_BUILD
 msgq_excise_t_SOURCES = msgq_excise_t.c test_stub.c
 msgq_excise_t_LDADD = ../msgq.o ../ircd_snprintf.o ../ircd_alloc.o ../ircd_string.o