Package: ntp Version: 1:4.2.6.p2+dfsg-1 Severity: important Tags: patch User: ubuntu-de...@lists.ubuntu.com Usertags: origin-ubuntu ubuntu-patch oneiric
Ubuntu's ntp package is built with hardening-wrapper (#542721). Version 1.32 of that package enables -Werror=format-security by default. This exposes some mistakes which might be potential security problems in the future (although I don't *think* they're problems right now), and which I think we ought to fix: * There are a few cases where ntp assumes that the translation of a %-less string will also be a %-less string; ntp still uses catgets (!) so I'm having trouble figuring out how its translations are handled, but this is a fragile assumption that's often broken by mistake let alone malice. (gettext at least does c-format checking on strings that look like format strings, but not usually on %-less strings.) * There are some functions that use snprintf or similar to construct a string, and then pass that to a printf-like function as a format string; this will cause the printf-like function to dereference junk pointers from the stack if one of the arguments passed to snprintf itself contains %, which again is easy to break by mistake let alone malice. * There's a case in ntpd/ntp_config.c where GCC isn't smart enough to prove that a const char * that's only ever assigned a %-less literal string can be treated as equivalent to such; but given the other problems I think it's reasonable to change the code to pacify GCC. A quilt patch fixing these warnings is attached. Thanks, -- Colin Watson [cjwat...@ubuntu.com]
Description: Fix build failures with -Werror=format-security The change to ntp_config.c is merely pacifying GCC, since signd_warning is a constant string containing no '%' characters. In the other cases, it is much more difficult to prove that the format string cannot contain any '%' characters. Author: Colin Watson <cjwat...@ubuntu.com> Last-Update: 2011-05-20 Index: b/lib/isc/unix/ifiter_ioctl.c =================================================================== --- a/lib/isc/unix/ifiter_ioctl.c +++ b/lib/isc/unix/ifiter_ioctl.c @@ -159,7 +159,7 @@ break; } if (iter->bufsize >= IFCONF_BUFSIZE_MAX) { - UNEXPECTED_ERROR(__FILE__, __LINE__, + UNEXPECTED_ERROR(__FILE__, __LINE__, "%s", isc_msgcat_get(isc_msgcat, ISC_MSGSET_IFITERIOCTL, ISC_MSG_BUFFERMAX, @@ -260,7 +260,7 @@ break; } if (iter->bufsize6 >= IFCONF_BUFSIZE_MAX) { - UNEXPECTED_ERROR(__FILE__, __LINE__, + UNEXPECTED_ERROR(__FILE__, __LINE__, "%s", isc_msgcat_get(isc_msgcat, ISC_MSGSET_IFITERIOCTL, ISC_MSG_BUFFERMAX, Index: b/ntpd/ntp_config.c =================================================================== --- a/ntpd/ntp_config.c +++ b/ntpd/ntp_config.c @@ -2324,7 +2324,7 @@ if ((RES_MSSNTP & flags) && !warned_signd) { warned_signd = 1; fprintf(stderr, "%s\n", signd_warning); - msyslog(LOG_WARNING, signd_warning); + msyslog(LOG_WARNING, "%s", signd_warning); } } } Index: b/ntpd/ntp_control.c =================================================================== --- a/ntpd/ntp_control.c +++ b/ntpd/ntp_control.c @@ -2948,7 +2948,7 @@ " %s", str); } NLOG(NLOG_SYSEVENT) - msyslog(LOG_INFO, statstr); + msyslog(LOG_INFO, "%s", statstr); } else { /* @@ -2980,7 +2980,7 @@ " %s", str); } NLOG(NLOG_PEEREVENT) - msyslog(LOG_INFO, statstr); + msyslog(LOG_INFO, "%s", statstr); } record_proto_stats(statstr); #if DEBUG Index: b/ntpd/ntpd.c =================================================================== --- a/ntpd/ntpd.c +++ b/ntpd/ntpd.c @@ -1283,7 +1283,7 @@ msyslog(LOG_ERR, "%s:%d: fatal error:", file, line); vsnprintf(errbuf, sizeof(errbuf), format, args); - msyslog(LOG_ERR, errbuf); + msyslog(LOG_ERR, "%s", errbuf); msyslog(LOG_ERR, "exiting (due to fatal error in library)"); abort(); @@ -1305,7 +1305,7 @@ msyslog(LOG_ERR, "%s:%d: unexpected error:", file, line); vsnprintf(errbuf, sizeof(errbuf), format, args); - msyslog(LOG_ERR, errbuf); + msyslog(LOG_ERR, "%s", errbuf); if (++unexpected_error_cnt == MAX_UNEXPECTED_ERRORS) {