[TALOS-CAN-0055] Infinite loop if extended logging enabled and the logfile and keyfil...

bk: 560f7ee4rt41Q67A9ABC4xKhiqSYGQ

diff --git a/ChangeLog b/ChangeLog
index 6159ec02decbc5f3f867e4165f7fc25849a9eee2..86ff0753ef76df27c6ff3ccae203108e80f8d39b 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,6 @@
 ---
+* [TALOS-CAN-0055] Infinite loop if extended logging enabled and
+  the logfile and keyfile are the same. perlinger@ntp.org
 * [TALOS-CAN-0062] prevent directory traversal for VMS, too, when
   using 'saveconfig' command.  perlinger@ntp.org
 * [TALOS-CAN-0064] signed/unsiged clash could lead to buffer overun

diff --git a/include/ntp_stdlib.h b/include/ntp_stdlib.h
index bad2697d06fd7875126ff469b68db60c7bbbf986..a2e62dabefd9e284e44d70a1ac3df7aba36d3bd6 100644 (file)
--- a/include/ntp_stdlib.h
+++ b/include/ntp_stdlib.h
@@ -31,6 +31,7 @@ extern        int     mvsnprintf(char *, size_t, const char *, va_list)
 extern int     msnprintf(char *, size_t, const char *, ...)
                        NTP_PRINTF(3, 4);
 extern void    msyslog(int, const char *, ...) NTP_PRINTF(2, 3);
+extern void    mvsyslog(int, const char *, va_list) NTP_PRINTF(2, 0);
 extern void    init_logging    (const char *, u_int32, int);
 extern int     change_logfile  (const char *, int);
 extern void    setup_logfile   (const char *);

diff --git a/include/ntp_syslog.h b/include/ntp_syslog.h
index a0152b53d9ca3fc2cd8dc28dd5e373ada370e566..ecc6346796e8da255ea50d18df42eccb1666f956 100644 (file)
--- a/include/ntp_syslog.h
+++ b/include/ntp_syslog.h
@@ -9,6 +9,7 @@
 
 #ifdef VMS
 extern void msyslog();
+extern void mvsyslog();
 #else
 # ifndef SYS_VXWORKS
 #  include <syslog.h>

diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c
index e8ddc942a6650a1a918df5e580b6d05983e2f599..e656c13ae5e090f763025e6e063bd0dffc7ca770 100644 (file)
--- a/libntp/authreadkeys.c
+++ b/libntp/authreadkeys.c
@@ -62,6 +62,40 @@ nexttok(
 }
 
 
+/* TALOS-CAN-0055: possibly DoS attack by setting the key file to the
+ * log file. This is hard to prevent (it would need to check two files
+ * to be the same on the inode level, which will not work so easily with
+ * Windows or VMS) but we can avoid the self-amplification loop: We only
+ * log the first 5 errors, silently ignore the next 10 errors, and give
+ * up when when we have found more than 15 errors.
+ *
+ * This avoids the endless file iteration we will end up with otherwise,
+ * and also avoids overflowing the log file.
+ *
+ * Nevertheless, once this happens, the keys are gone since this would
+ * require a save/swap strategy that is not easy to apply due to the
+ * data on global/static level.
+ */
+
+static const size_t nerr_loglimit = 5u;
+static const size_t nerr_maxlimit = 15;
+
+static void log_maybe(size_t*, const char*, ...) NTP_PRINTF(2, 3);
+
+static void
+log_maybe(
+       size_t     *pnerr,
+       const char *fmt  ,
+       ...)
+{
+       va_list ap;
+       if (++(*pnerr) <= nerr_loglimit) {
+               va_start(ap, fmt);
+               mvsyslog(LOG_ERR, fmt, ap);
+               va_end(ap);
+       }
+}
+
 /*
  * authreadkeys - (re)read keys from a file.
  */
@@ -79,7 +113,7 @@ authreadkeys(
        u_char  keystr[32];             /* Bug 2537 */
        size_t  len;
        size_t  j;
-
+       size_t  nerr;
        /*
         * Open file.  Complain and return if it can't be opened.
         */
@@ -99,7 +133,10 @@ authreadkeys(
        /*
         * Now read lines from the file, looking for key entries
         */
+       nerr = 0;
        while ((line = fgets(buf, sizeof buf, fp)) != NULL) {
+               if (nerr > nerr_maxlimit)
+                       break;
                token = nexttok(&line);
                if (token == NULL)
                        continue;
@@ -109,15 +146,16 @@ authreadkeys(
                 */
                keyno = atoi(token);
                if (keyno == 0) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: cannot change key %s", token);
+                       log_maybe(&nerr,
+                                 "authreadkeys: cannot change key %s",
+                                 token);
                        continue;
                }
 
                if (keyno > NTP_MAXKEY) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: key %s > %d reserved for Autokey",
-                           token, NTP_MAXKEY);
+                       log_maybe(&nerr,
+                                 "authreadkeys: key %s > %d reserved for Autokey",
+                                 token, NTP_MAXKEY);
                        continue;
                }
 
@@ -126,8 +164,9 @@ authreadkeys(
                 */
                token = nexttok(&line);
                if (token == NULL) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: no key type for key %d", keyno);
+                       log_maybe(&nerr,
+                                 "authreadkeys: no key type for key %d",
+                                 keyno);
                        continue;
                }
 #ifdef OPENSSL
@@ -139,13 +178,15 @@ authreadkeys(
                 */
                keytype = keytype_from_text(token, NULL);
                if (keytype == 0) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: invalid type for key %d", keyno);
+                       log_maybe(&nerr,
+                                 "authreadkeys: invalid type for key %d",
+                                 keyno);
                        continue;
                }
                if (EVP_get_digestbynid(keytype) == NULL) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: no algorithm for key %d", keyno);
+                       log_maybe(&nerr,
+                                 "authreadkeys: no algorithm for key %d",
+                                 keyno);
                        continue;
                }
 #else  /* !OPENSSL follows */
@@ -155,8 +196,9 @@ authreadkeys(
                 * 'm' for compatibility.
                 */
                if (!(*token == 'M' || *token == 'm')) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: invalid type for key %d", keyno);
+                       log_maybe(&nerr,
+                                 "authreadkeys: invalid type for key %d",
+                                 keyno);
                        continue;
                }
                keytype = KEY_TYPE_MD5;
@@ -170,8 +212,8 @@ authreadkeys(
                 */
                token = nexttok(&line);
                if (token == NULL) {
-                       msyslog(LOG_ERR,
-                           "authreadkeys: no key for key %d", keyno);
+                       log_maybe(&nerr,
+                                 "authreadkeys: no key for key %d", keyno);
                        continue;
                }
                len = strlen(token);
@@ -195,13 +237,24 @@ authreadkeys(
                                        keystr[j / 2] = temp << 4;
                        }
                        if (j < jlim) {
-                               msyslog(LOG_ERR,
-                                       "authreadkeys: invalid hex digit for key %d", keyno);
+                               log_maybe(&nerr,
+                                         "authreadkeys: invalid hex digit for key %d",
+                                         keyno);
                                continue;
                        }
                        MD5auth_setkey(keyno, keytype, keystr, jlim / 2);
                }
        }
        fclose(fp);
+       if (nerr > nerr_maxlimit) {
+               msyslog(LOG_ERR,
+                       "authreadkeys: emergeny break after %u errors",
+                       nerr);
+               return (0);
+       } else if (nerr > nerr_loglimit) {
+               msyslog(LOG_ERR,
+                       "authreadkeys: found %u more error(s)",
+                       nerr - nerr_loglimit);
+       }
        return (1);
 }

diff --git a/libntp/msyslog.c b/libntp/msyslog.c
index 46bda059a030fbff2bbbdb3949606dc2621c7c82..cc8868f8e7b84c66c603c8c4957b36c8c7e8a51d 100644 (file)
--- a/libntp/msyslog.c
+++ b/libntp/msyslog.c
@@ -357,6 +357,18 @@ msyslog(
        addto_syslog(level, buf);
 }
 
+void
+mvsyslog(
+       int             level,
+       const char *    fmt,
+       va_list         ap
+       )
+{
+       char    buf[1024];
+       mvsnprintf(buf, sizeof(buf), fmt, ap);
+       addto_syslog(level, buf);
+}
+
 
 /*
  * Initialize the logging