From: Harlan Stenn Date: Wed, 13 Jan 2016 05:55:48 +0000 (+0000) Subject: Merge psp-deb1.ntp.org:/home/stenn/ntp-stable-p6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be6f297417c2b175bac5266e4639a69085cedaca;p=thirdparty%2Fntp.git Merge psp-deb1.ntp.org:/home/stenn/ntp-stable-p6 into psp-deb1.ntp.org:/home/perlinger/ntp-stable-2938 bk: 5695e6e4reqdzXibv16Yc9dj_FX8pg --- be6f297417c2b175bac5266e4639a69085cedaca diff --cc ChangeLog index 650f2f17d,cc2e62d99..f4d6dbf3c --- a/ChangeLog +++ b/ChangeLog @@@ -1,65 -1,6 +1,68 @@@ --- -* [Bug 2938] ntpq saveconfig command allows dangerous characters + +* [Sec 2935] Deja Vu: Replay attack on authenticated broadcast mode. HStenn. +* [Sec 2937] ntpq: nextvar() missing length check. perlinger@ntp.org ++* [Sec 2938] ntpq saveconfig command allows dangerous characters + in filenames. perlinger@ntp.org +* Make leapsec_query debug messages less verbose. Harlan Stenn. +--- +(4.2.8p5) 2016/01/07 Released by Harlan Stenn + +* [Sec 2956] small-step/big-step. Close the panic gate earlier. HStenn. +* CID 1339955: Free allocated memory in caljulian test. HStenn. +* CID 1339962: Explicitly initialize variable in caljulian test. HStenn. +* CID 1341527: Quiet a CHECKED_RETURN in sntp/tests/t-log.c. HStenn. +* CID 1341533: Missing assertion in sntp/tests/t-log.c. HStenn. +* CID 1341534: Resource leak in tests/ntpd/t-ntp_signd.c. HStenn. +* CID 1341535: Resource leak in tests/ntpd/t-ntp_signd.c. HStenn. +* CID 1341536: Resource leak in tests/ntpd/t-ntp_signd.c. HStenn. +* CID 1341537: Resource leak in tests/ntpd/t-ntp_signd.c. HStenn. +* CID 1341538: Memory leak in tests/ntpd/ntp_prio_q.c:262. HStenn. +* CID 1341677: Nits in sntp/tests/keyFile.c. HStenn. +* CID 1341678: Nits in sntp/tests/keyFile.c. HStenn. +* CID 1341679: Nits in sntp/tests/keyFile.c. HStenn. +* CID 1341680: Nits in sntp/tests/keyFile.c. HStenn. +* CID 1341681: Nits in sntp/tests/keyFile.c. HStenn. +* CID 1341682: Nit in libntp/authreadkeys.c. HStenn. +* CID 1341684: Nit in tests/ntpd/t-ntp_signd.c. HStenn. +* [Bug 2829] Look at pipe_fds in ntpd.c (did so. perlinger@ntp.org) +* [Bug 2887] stratum -1 config results as showing value 99 + - fudge stratum should only accept values [0..16]. perlinger@ntp.org +* [Bug 2932] Update leapsecond file info in miscopt.html. CWoodbury, HStenn. +* [Bug 2934] tests/ntpd/t-ntp_scanner.c has a magic constant wired in. HMurray +* [Bug 2944] errno is not preserved properly in ntpdate after sendto call. + - applied patch by Christos Zoulas. perlinger@ntp.org +* [Bug 2952] Symmetric active/passive mode is broken. HStenn. +* [Bug 2954] Version 4.2.8p4 crashes on startup with sig fault + - fixed data race conditions in threaded DNS worker. perlinger@ntp.org + - limit threading warm-up to linux; FreeBSD bombs on it. perlinger@ntp.org +* [Bug 2957] 'unsigned int' vs 'size_t' format clash. perlinger@ntp.org + - accept key file only if there are no parsing errors + - fixed size_t/u_int format clash + - fixed wrong use of 'strlcpy' +* [Bug 2958] ntpq: fatal error messages need a final newline. Craig Leres. +* [Bug 2962] truncation of size_t/ptrdiff_t on 64bit targets. perlinger@ntp.org + - fixed several other warnings (cast-alignment, missing const, missing prototypes) + - promote use of 'size_t' for values that express a size + - use ptr-to-const for read-only arguments + - make sure SOCKET values are not truncated (win32-specific) + - format string fixes +* [Bug 2965] Local clock didn't work since 4.2.8p4. Martin Burnicki. +* [Bug 2967] ntpdate command suffers an assertion failure + - fixed ntp_rfc2553.c to return proper address length. perlinger@ntp.org +* [Bug 2969] Seg fault from ntpq/mrulist when looking at server with + lots of clients. perlinger@ntp.org +* [Bug 2971] ntpq bails on ^C: select fails: Interrupted system call + - changed stacked/nested handling of CTRL-C. perlinger@ntp.org +* Unity cleanup for FreeBSD-6.4. Harlan Stenn. +* Unity test cleanup. Harlan Stenn. +* Libevent autoconf pthread fixes for FreeBSD-10. Harlan Stenn. +* Header cleanup in tests/sandbox/uglydate.c. Harlan Stenn. +* Header cleanup in tests/libntp/sfptostr.c. Harlan Stenn. +* Quiet a warning from clang. Harlan Stenn. +* Update the NEWS file. Harlan Stenn. +* Update scripts/calc_tickadj/Makefile.am. Harlan Stenn. ++>>>>>>> --- (4.2.8p4-RC1) 2015/10/06 Released by Harlan Stenn diff --cc ntpd/ntp_control.c index 2e174d021,13cdb0046..e5a567e78 --- a/ntpd/ntp_control.c +++ b/ntpd/ntp_control.c @@@ -873,6 -876,57 +876,58 @@@ ctl_error CTL_HEADER_LEN); } + int/*BOOL*/ + is_safe_filename(const char * name) + { + /* We need a strict validation of filenames we should write: The + * daemon might run with special permissions and is remote + * controllable, so we better take care what we allow as file + * name! + * + * The first character must be digit or a letter from the ASCII + * base plane or a '_' ([_A-Za-z0-9]), the following characters + * must be from [-._+A-Za-z0-9]. + * + * We do not trust the character classification much here: Since + * the NTP protocol makes no provisions for UTF-8 or local code + * pages, we strictly require the 7bit ASCII code page. + * + * The following table is a packed bit field of 128 two-bit + * groups. The LSB in each group tells us if a character is + * acceptable at the first position, the MSB if the character is + * accepted at any other position. + * + * This does not ensure that the file name is syntactically + * correct (multiple dots will not work with VMS...) but it will + * exclude potential globbing bombs and directory traversal. It + * also rules out drive selection. (For systems that have this + * notion, like Windows or VMS.) + */ + static const uint32_t chclass[8] = { + 0x00000000, 0x00000000, + 0x28800000, 0x000FFFFF, + 0xFFFFFFFC, 0xC03FFFFF, + 0xFFFFFFFC, 0x003FFFFF + }; + + u_int widx, bidx, mask; + if (!*name) + return FALSE; + + mask = 1u; + while (0 != (widx = (u_char)*name++)) { + bidx = (widx & 15) << 1; + widx = widx >> 4; + if (widx >= sizeof(chclass)) + return FALSE; + if (0 == ((chclass[widx] >> bidx) & mask)) + return FALSE; + mask |= 2u; + } + return TRUE; + } + ++ /* * save_config - Implements ntpq -c "saveconfig " * Writes current configuration including any runtime @@@ -884,30 -942,24 +943,45 @@@ save_config int restrict_mask ) { + /* block directory traversal by searching for characters that + * indicate directory components in a file path. + * + * Conceptually we should be searching for DIRSEP in filename, + * however Windows actually recognizes both forward and + * backslashes as equivalent directory separators at the API + * level. On POSIX systems we could allow '\\' but such + * filenames are tricky to manipulate from a shell, so just + * reject both types of slashes on all platforms. + */ + /* TALOS-CAN-0062: block directory traversal for VMS, too */ + static const char * illegal_in_filename = +#if defined(VMS) + ":[]" /* do not allow drive and path components here */ +#elif defined(SYS_WINNT) + ":\\/" /* path and drive separators */ +#else + "\\/" /* separator and critical char for POSIX */ +#endif + ; - - + char reply[128]; #ifdef SAVECONFIG + static const char savedconfig_eq[] = "savedconfig="; + + /* Build a safe open mode from the available mode flags. We want + * to create a new file and write it in text mode (when + * applicable -- only Windows does this...) + */ + static const int openmode = O_CREAT | O_TRUNC | O_WRONLY + # if defined(O_EXCL) /* posix, vms */ + | O_EXCL + # elif defined(_O_EXCL) /* windows is alway very special... */ + | _O_EXCL + # endif + # if defined(_O_TEXT) /* windows, again */ + | _O_TEXT + #endif + ; + char filespec[128]; char filename[128]; char fullpath[512]; @@@ -955,29 -1027,58 +1049,75 @@@ * strftime() format such as: * ntpq -c "saveconfig ntp-%Y%m%d-%H%M%S.conf" * XXX: Nice feature, but not too safe. + * YYY: The check for permitted characters in file names should + * weed out the worst. Let's hope 'strftime()' does not + * develop pathological problems. */ + time(&now); if (0 == strftime(filename, sizeof(filename), filespec, - localtime(&now))) + localtime(&now))) + { + /* + * If we arrive here, 'strftime()' balked; most likely + * the buffer was too short. (Or it encounterd an empty + * format, or just a format that expands to an empty + * string.) We try to use the original name, though this + * is very likely to fail later if there are format + * specs in the string. Note that truncation cannot + * happen here as long as both buffers have the same + * size! + */ strlcpy(filename, filespec, sizeof(filename)); + } + + /* - * Check the file name for sanity. This migth/will rule out file ++ * Check the file name for sanity. This might/will rule out file + * names that would be legal but problematic, and it blocks + * directory traversal. + */ + if (!is_safe_filename(filename)) { + ctl_printf("saveconfig rejects unsafe file name '%s'", + filename); + ctl_flushpkt(0); + msyslog(LOG_NOTICE, + "saveconfig rejects unsafe file name from %s", + stoa(&rbufp->recv_srcadr)); + return; + } + ++ /* ++ * XXX: This next test may not be needed with is_safe_filename() ++ */ + + /* block directory/drive traversal */ + /* TALOS-CAN-0062: block directory traversal for VMS, too */ + if (NULL != strpbrk(filename, illegal_in_filename)) { + snprintf(reply, sizeof(reply), + "saveconfig does not allow directory in filename"); + ctl_putdata(reply, strlen(reply), 0); + ctl_flushpkt(0); + msyslog(LOG_NOTICE, - "saveconfig with path from %s rejected", ++ "saveconfig rejects unsafe file name from %s", + stoa(&rbufp->recv_srcadr)); + return; + } + - snprintf(fullpath, sizeof(fullpath), "%s%s", - saveconfigdir, filename); + /* concatenation of directory and path can cause another + * truncation... + */ + prc = snprintf(fullpath, sizeof(fullpath), "%s%s", + saveconfigdir, filename); + if (prc < 0 || prc >= sizeof(fullpath)) { + ctl_printf("saveconfig exceeded maximum path length (%u)", + (u_int)sizeof(fullpath)); + ctl_flushpkt(0); + msyslog(LOG_NOTICE, + "saveconfig exceeded maximum path length from %s", + stoa(&rbufp->recv_srcadr)); + return; + } - fd = open(fullpath, O_CREAT | O_TRUNC | O_WRONLY, - S_IRUSR | S_IWUSR); + fd = open(fullpath, openmode, S_IRUSR | S_IWUSR); if (-1 == fd) fptr = NULL; else