From: Juergen Perlinger Date: Sat, 3 Oct 2015 07:08:20 +0000 (+0200) Subject: [TALOS-CAN-0055] Infinite loop if extended logging enabled and the logfile and keyfil... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=596de71230a122945cd796581f2d03f591b6137d;p=thirdparty%2Fntp.git [TALOS-CAN-0055] Infinite loop if extended logging enabled and the logfile and keyfile are the same bk: 560f7ee4rt41Q67A9ABC4xKhiqSYGQ --- diff --git a/ChangeLog b/ChangeLog index 6159ec02d..86ff0753e 100644 --- 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 bad2697d0..a2e62dabe 100644 --- 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 a0152b53d..ecc634679 100644 --- 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 diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c index e8ddc942a..e656c13ae 100644 --- 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 46bda059a..cc8868f8e 100644 --- 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