[Git][NTPsec/ntpsec][master] Merge Classic fix for CVE-2018-7182.

Eric S. Raymond gitlab at mg.gitlab.com
Wed Feb 28 21:52:56 UTC 2018


Eric S. Raymond pushed to branch master at NTPsec / ntpsec


Commits:
6d6aa6da by Eric S. Raymond at 2018-02-28T16:50:42-05:00
Merge Classic fix for CVE-2018-7182.

- - - - -


2 changed files:

- NEWS
- ntpd/ntp_control.c


Changes:

=====================================
NEWS
=====================================
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,8 @@ on user-visible changes.
 
 == Repository head ==
 
+We have merged Classic's fix for CVE-2018-7182.
+
 It is now possible to unpeer refclocks using a type/unit specification
 rather than a magic IP address.  This was the last obligatory use of
 magic IP addresses in the configuration grammar.


=====================================
ntpd/ntp_control.c
=====================================
--- a/ntpd/ntp_control.c
+++ b/ntpd/ntp_control.c
@@ -2469,7 +2469,6 @@ ctl_putclock(
 #endif
 
 
-
 /*
  * ctl_getitem - get the next data item from the incoming packet
  */
@@ -2479,84 +2478,125 @@ ctl_getitem(
 	char **data
 	)
 {
+	/* [Bug 3008] First check the packet data sanity, then search
+	 * the key. This improves the consistency of result values: If
+	 * the result is NULL once, it will never be EOV again for this
+	 * packet; If it's EOV, it will never be NULL again until the
+	 * variable is found and processed in a given 'var_list'. (That
+	 * is, a result is returned that is neither NULL nor EOV).
+	 */ 
 	static const struct ctl_var eol = { 0, EOV, NULL };
 	static char buf[128];
-	static unsigned long quiet_until;
+	static u_long quiet_until;
 	const struct ctl_var *v;
-	const char *pch;
 	char *cp;
 	char *tp;
 
 	/*
-	 * Delete leading commas and white space
+	 * Part One: Validate the packet state
 	 */
+
+	/* Delete leading commas and white space */
 	while (reqpt < reqend && (*reqpt == ',' ||
 				  isspace((unsigned char)*reqpt)))
 		reqpt++;
 	if (reqpt >= reqend)
 		return NULL;
 
+	/* Scan the string in the packet until we hit comma or
+	 * EoB. Register position of first '=' on the fly. */
+	for (tp = NULL, cp = reqpt; cp != reqend; ++cp) {
+		if (*cp == '=' && tp == NULL)
+			tp = cp;
+		if (*cp == ',')
+			break;
+	}
+
+	/* Process payload, if any. */
+	*data = NULL;
+	if (NULL != tp) {
+		/* eventually strip white space from argument. */
+		const char *plhead = tp + 1; /* skip the '=' */
+		const char *pltail = cp;
+		size_t      plsize;
+
+		while (plhead != pltail && isspace((u_char)plhead[0]))
+			++plhead;
+		while (plhead != pltail && isspace((u_char)pltail[-1]))
+			--pltail;
+		
+		/* check payload size, terminate packet on overflow */
+		plsize = (size_t)(pltail - plhead);
+		if (plsize >= sizeof(buf))
+			goto badpacket;
+
+		/* copy data, NUL terminate, and set result data ptr */
+		memcpy(buf, plhead, plsize);
+		buf[plsize] = '\0';
+		*data = buf;
+	} else {
+		/* no payload, current end --> current name termination */
+		tp = cp;
+	}
+
+	/* Part Two
+	 *
+	 * Now we're sure that the packet data itself is sane. Scan the
+	 * list now. Make sure a NULL list is properly treated by
+	 * returning a synthetic End-Of-Values record. We must not
+	 * return NULL pointers after this point, or the behaviour would
+	 * become inconsistent if called several times with different
+	 * variable lists after an EoV was returned.  (Such a behavior
+	 * actually caused Bug 3008.)
+	 */
+	
 	if (NULL == var_list)
 		return &eol;
 
-	/*
-	 * Look for a first character match on the tag.  If we find
-	 * one, see if it is a full match.
-	 */
-	v = var_list;
-	cp = reqpt;
-	for (v = var_list; !(EOV & v->flags); v++) {
-		if (!(PADDING & v->flags) && *cp == *(v->text)) {
-			pch = v->text;
-			while ('\0' != *pch && '=' != *pch && cp < reqend
-			       && *cp == *pch) {
-				cp++;
-				pch++;
-			}
-			if ('\0' == *pch || '=' == *pch) {
-				while (cp < reqend && isspace((uint8_t)*cp))
-					cp++;
-				if (cp == reqend || ',' == *cp) {
-					buf[0] = '\0';
-					*data = buf;
-					if (cp < reqend)
-						cp++;
-					reqpt = cp;
-					return v;
-				}
-				if ('=' == *cp) {
-					cp++;
-					tp = buf;
-					while (cp < reqend && isspace((uint8_t)*cp))
-						cp++;
-					while (cp < reqend && *cp != ',') {
-						*tp++ = *cp++;
-						if ((size_t)(tp - buf) >= sizeof(buf)) {
-							ctl_error(CERR_BADFMT);
-							numctlbadpkts++;
-							NLOG(NLOG_SYSEVENT)
-								if (quiet_until <= current_time) {
-									quiet_until = current_time + 300;
-									msyslog(LOG_WARNING,
-"MODE6: Possible 'ntpdx' exploit from %s#%u (possibly spoofed)", socktoa(rmt_addr), SRCPORT(rmt_addr));
-								}
-							return NULL;
-						}
-					}
-					if (cp < reqend)
-						cp++;
-					*tp-- = '\0';
-					while (tp >= buf && isspace((uint8_t)*tp))
-						*tp-- = '\0';
-					reqpt = cp;
-					*data = buf;
-					return v;
-				}
+	for (v = var_list; !(EOV & v->flags); ++v)
+		if (!(PADDING & v->flags)) {
+			/* Check if the var name matches the buffer. The
+			 * name is bracketed by [reqpt..tp] and not NUL
+			 * terminated, and it contains no '=' char. The
+			 * lookup value IS NUL-terminated but might
+			 * include a '='... We have to look out for
+			 * that!
+			 */
+			const char *sp1 = reqpt;
+			const char *sp2 = v->text;
+
+			/* [Bug 3412] do not compare past NUL byte in name */
+			while (   (sp1 != tp)
+			       && ('\0' != *sp2) && (*sp1 == *sp2)) {
+				++sp1;
+				++sp2;
 			}
-			cp = reqpt;
+			if (sp1 == tp && (*sp2 == '\0' || *sp2 == '='))
+				break;
 		}
-	}
+
+	/* See if we have found a valid entry or not. If found, advance
+	 * the request pointer for the next round; if not, clear the
+	 * data pointer so we have no dangling garbage here.
+	 */
+	if (EOV & v->flags)
+		*data = NULL;
+	else
+		reqpt = cp + (cp != reqend);
 	return v;
+
+  badpacket:
+	/*TODO? somehow indicate this packet was bad, apart from syslog? */
+	numctlbadpkts++;
+	NLOG(NLOG_SYSEVENT)
+	    if (quiet_until <= current_time) {
+		    quiet_until = current_time + 300;
+		    msyslog(LOG_WARNING,
+			    "Possible 'ntpdx' exploit from %s#%u (possibly spoofed)",
+			    socktoa(rmt_addr), SRCPORT(rmt_addr));
+	    }
+	reqpt = reqend; /* never again for this packet! */
+	return NULL;
 }
 
 



View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/6d6aa6da0fe685011f5a9633c3618409af8349d7

---
View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/6d6aa6da0fe685011f5a9633c3618409af8349d7
You're receiving this email because of your account on gitlab.com.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ntpsec.org/pipermail/vc/attachments/20180228/0e4c313b/attachment.html>


More information about the vc mailing list