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)
{
