FreeBSD-SA-09:11: NTPd Remote Stack Based Buffer Overflows

This was reported by Chris Ries and affects all of the supported FreeBSD releases. NTPd is the FreeBSD implementation of the Network Time Protocol and among other features it includes Autokey. Autokey is a security model that uses public key cryptography to authenticate NTPd to its clients. Here is the vulnerable code from FreeBSD 7.2-RELEASE:

/*
 * ntp_crypto.c - NTP version 4 public key routines
 */
       ...
/*
 * crypto_recv - parse extension fields
 *
 * This routine is called when the packet has been matched to an
 * association and passed sanity, format and MAC checks. We believe the
 * extension field values only if the field has proper format and
 * length, the timestamp and filestamp are valid and the signature has
 * valid length and is verified. There are a few cases where some values
 * are believed even if the signature fails, but only if the proventic
 * bit is not set.
 */
int
crypto_recv(
	struct peer *peer,	/* peer structure pointer */
	struct recvbuf *rbufp	/* packet buffer pointer */
	)
{
       ...
	char	statstr[NTP_MAXSTRLEN]; /* statistics for filegen */
       ...
		switch (code) {
       ...
		/* fall through */

		case CRYPTO_ASSOC | CRYPTO_RESP:
       ...
		/*
			 * Save status word, host name and message
			 * digest/signature type. If PC identity, be
			 * sure not to sign the certificate.
			 */
			if (crypto_flags & CRYPTO_FLAG_PRIV)
				fstamp |= CRYPTO_FLAG_SIGN;
			peer->crypto = fstamp;
			peer->digest = dp;
			peer->subject = emalloc(vallen + 1);
			memcpy(peer->subject, ep->pkt, vallen);
			peer->subject[vallen] = '';
			peer->issuer = emalloc(vallen + 1);
			strcpy(peer->issuer, peer->subject);
			temp32 = (fstamp >> 16) & 0xffff;
			sprintf(statstr,
			    "flags 0x%x host %s signature %s", fstamp,
			    peer->subject, OBJ_nid2ln(temp32));
			record_crypto_stats(&peer->srcadr, statstr);
       ...

This is not a joke, this is another sprintf() stack overflow in FreeBSD! This was fixed by applying this patch:

 			temp32 = (fstamp >> 16) & 0xffff;
-			sprintf(statstr,
+			snprintf(statstr, NTP_MAXSTRLEN,
 			    "flags 0x%x host %s signature %s", fstamp,
 			    peer->subject, OBJ_nid2ln(temp32));

However, the code includes more sprintf() overflows. Here is the next one from the same routine:

	/*
		 * Decode X509 certificate in ASN.1 format and extract
		 * the data containing, among other things, subject
		 * name and public key. In the default identification
		 * scheme, the certificate trail is followed to a self
		 * signed trusted certificate.
		 */
		case CRYPTO_CERT | CRYPTO_RESP:
       ...
			temp32 = cinfo->nid;
			sprintf(statstr,"cert %s 0x%x %s (%u) fs %u",
			    cinfo->subject, cinfo->flags,
			    OBJ_nid2ln(temp32), temp32,
			    ntohl(ep->fstamp));
			record_crypto_stats(&peer->srcadr, statstr);

Where everything is once again user controlled and it was patched like this:

 			temp32 = cinfo->nid;
-			sprintf(statstr, "cert %s 0x%x %s (%u) fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN,
+			    "cert %s 0x%x %s (%u) fs %u",
 			    cinfo->subject, cinfo->flags,
 			    OBJ_nid2ln(temp32), temp32,

Another one…

		/*
		 * Schnorr (IFF)identity scheme. This scheme is designed
		 * for use with shared secret group keys and where the
		 * certificate may be generated by a third party. The
		 * client sends a challenge to the server, which
		 * performs a calculation and returns the result. A
		 * positive result is possible only if both client and
		 * server contain the same secret group key.
		 */
		case CRYPTO_IFF | CRYPTO_RESP:
            ...
			peer->crypto |= CRYPTO_FLAG_VRFY |
			    CRYPTO_FLAG_PROV;
			peer->flash &= ~TEST10;
			sprintf(statstr, "iff fs %u",
			    ntohl(ep->fstamp));
			record_crypto_stats(&peer->srcadr, statstr);

Which was patched in a similar manner:

 			peer->flash &= ~TEST10;
-			sprintf(statstr, "iff fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "iff fs %u",
 			    ntohl(ep->fstamp));

Yes… there are more…

		/*
		 * Guillou-Quisquater (GQ) identity scheme. This scheme
		 * is designed for use with public certificates carrying
		 * the GQ public key in an extension field. The client
		 * sends a challenge to the server, which performs a
		 * calculation and returns the result. A positive result
		 * is possible only if both client and server contain
		 * the same group key and the server has the matching GQ
		 * private key.
		 */
		case CRYPTO_GQ | CRYPTO_RESP:
              ...
			peer->crypto |= CRYPTO_FLAG_VRFY |
			    CRYPTO_FLAG_PROV;
			peer->flash &= ~TEST10;
			sprintf(statstr, "gq fs %u",
			    ntohl(ep->fstamp));
			record_crypto_stats(&peer->srcadr, statstr);

Which was also patched by replacing sprintf() with snprintf() like this:

 			peer->flash &= ~TEST10;
-			sprintf(statstr, "gq fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "gq fs %u",
 			    ntohl(ep->fstamp));

Next one…

		/*
		 * MV
		 */
		case CRYPTO_MV | CRYPTO_RESP:
            ...
			peer->crypto |= CRYPTO_FLAG_VRFY |
			    CRYPTO_FLAG_PROV;
			peer->flash &= ~TEST10;
			sprintf(statstr, "mv fs %u",
			    ntohl(ep->fstamp));
			record_crypto_stats(&peer->srcadr, statstr);

Patch:

 			peer->flash &= ~TEST10;
-			sprintf(statstr, "mv fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "mv fs %u",
 			    ntohl(ep->fstamp));

Another one…

		/*
		 * X509 certificate sign response. Validate the
		 * certificate signed by the server and install. Later
		 * this can be provided to clients of this server in
		 * lieu of the self signed certificate in order to
		 * validate the public key.
		 */
		case CRYPTO_SIGN | CRYPTO_RESP:
         ...
			temp32 = cinfo->nid;
			sprintf(statstr, "sign %s 0x%x %s (%u) fs %u",
			    cinfo->issuer, cinfo->flags,
			    OBJ_nid2ln(temp32), temp32,
			    ntohl(ep->fstamp));

Which was surprisingly patched like this :P

 			temp32 = cinfo->nid;
-			sprintf(statstr, "sign %s 0x%x %s (%u) fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "sign %s 0x%x %s (%u) fs %u",
 			    cinfo->issuer, cinfo->flags,
 			    OBJ_nid2ln(temp32), temp32,
 			    ntohl(ep->fstamp));

Not over yet…

		/*
		 * Cookie request in symmetric modes. Roll a random
		 * cookie and install in symmetric mode. Encrypt for the
		 * response, which is transmitted later.
		 */
		case CRYPTO_COOK:
          ...
			peer->flash &= ~TEST10;
			sprintf(statstr, "cook %x ts %u fs %u",
			    peer->pcookie, ntohl(ep->tstamp),
			    ntohl(ep->fstamp));

And the equivalent patch:

 			peer->flash &= ~TEST10;
-			sprintf(statstr, "cook %x ts %u fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "cook %x ts %u fs %u",
 			    peer->pcookie, ntohl(ep->tstamp),
 			    ntohl(ep->fstamp));

They are everywhere…

		/*
		 * Cookie response in client and symmetric modes. If the
		 * cookie bit is set, the working cookie is the EXOR of
		 * the current and new values.
		 */
		case CRYPTO_COOK | CRYPTO_RESP:
         ...
			peer->flash &= ~TEST10;
			sprintf(statstr, "cook %x ts %u fs %u",
			    peer->pcookie, ntohl(ep->tstamp),
			    ntohl(ep->fstamp));

It looks like a bad movie..

 			peer->flash &= ~TEST10;
-			sprintf(statstr, "cook %x ts %u fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "cook %x ts %u fs %u",
 			    peer->pcookie, ntohl(ep->tstamp),
 			    ntohl(ep->fstamp));

That you wish it will end sometime

		/*
		 * Install autokey values in broadcast client and
		 * symmetric modes. We have to do this every time the
		 * sever/peer cookie changes or a new keylist is
		 * rolled. Ordinarily, this is automatic as this message
		 * is piggybacked on the first NTP packet sent upon
		 * either of these events. Note that a broadcast client
		 * or symmetric peer can receive this response without a
		 * matching request.
		 */
		case CRYPTO_AUTO | CRYPTO_RESP:
            ...
			peer->flash &= ~TEST10;
			sprintf(statstr,
			    "auto seq %d key %x ts %u fs %u", bp->seq,
			    bp->key, ntohl(ep->tstamp),
			    ntohl(ep->fstamp));

It is boring…

 			peer->flash &= ~TEST10;
-			sprintf(statstr,
+			snprintf(statstr, NTP_MAXSTRLEN,
 			    "auto seq %d key %x ts %u fs %u", bp->seq,
 			    bp->key, ntohl(ep->tstamp),
 			    ntohl(ep->fstamp));

A few more I guess

		/* fall through */

		case CRYPTO_TAI | CRYPTO_RESP:
            ...
 #endif /* KERNEL_PLL */
			sprintf(statstr, "leap %u ts %u fs %u",
			    vallen, ntohl(ep->tstamp),
			    ntohl(ep->fstamp));
			record_crypto_stats(&peer->srcadr, statstr);

So exciting

 #endif /* KERNEL_PLL */
-			sprintf(statstr, "leap %u ts %u fs %u",
+			snprintf(statstr, NTP_MAXSTRLEN, "leap %u ts %u fs %u",
 			    vallen, ntohl(ep->tstamp),
 			    ntohl(ep->fstamp));

I cannot believe this

		/*
		 * We log everything except length/format errors and
		 * duplicates, which are log clogging vulnerabilities.
		 * The first error found terminates the extension field
		 * scan and we return the laundry to the caller.
		 */
		if (rval != XEVNT_OK) {
			sprintf(statstr,
			    "error %x opcode %x ts %u fs %u", rval,
			    code, tstamp, fstamp);

I wonder how they patched this one. Let’s see

 		if (rval != XEVNT_OK) {
-			sprintf(statstr,
+			snprintf(statstr, NTP_MAXSTRLEN,
 			    "error %x opcode %x ts %u fs %u", rval,
 			    code, tstamp, fstamp);

At least there are no more case statements

	/*
	 * We ignore length/format errors and duplicates. Other errors
	 * are reported to the log and deny further service. To really
	 * persistent rascals we toss back a kiss-of-death grenade.
	 */
	if (rval > XEVNT_TSP) {
		opcode |= CRYPTO_ERROR;
		sprintf(statstr, "error %x opcode %x", rval, opcode);

And it was patched…

	if (rval > XEVNT_TSP) {
 		opcode |= CRYPTO_ERROR;
-		sprintf(statstr, "error %x opcode %x", rval, opcode);
+		snprintf(statstr, NTP_MAXSTRLEN,
+		    "error %x opcode %x", rval, opcode);
 		record_crypto_stats(srcadr_sin, statstr);

The next one

	/*
	 * Sign leapseconds table and timestamps. The filestamp is
	 * derived from the leapsecond file extension from wherever the
	 * file was generated.
	 */
        ...
	sprintf(statstr, "update ts %u", ntohl(hostval.tstamp));
	record_crypto_stats(NULL, statstr);

Patch:

 	}
-	sprintf(statstr, "update ts %u", ntohl(hostval.tstamp));
+	snprintf(statstr, NTP_MAXSTRLEN,
+	    "update ts %u", ntohl(hostval.tstamp));
 	record_crypto_stats(NULL, statstr);

This is a good one

	/*
	 * Leave tracks in the cryptostats.
	 */
	if ((ptr = strrchr(linkname, '\n')) != NULL)
		*ptr = '';
	sprintf(statstr, "%s mod %d", &linkname[2],
	    EVP_PKEY_size(pkey) * 8);
	record_crypto_stats(NULL, statstr);

And the patch:

 		*ptr = '';
-	sprintf(statstr, "%s mod %d", &linkname[2],
+	snprintf(statstr, NTP_MAXSTRLEN, "%s mod %d", &linkname[2],
 	    EVP_PKEY_size(pkey) * 8);

And at last…

	return (NULL);
	if ((ptr = strrchr(linkname, '\n')) != NULL)
		*ptr = '';
	sprintf(statstr, "%s 0x%x len %lu", &linkname[2], ret->flags,
            len);
	record_crypto_stats(NULL, statstr);

And its patch:

 		*ptr = '';
-	sprintf(statstr, "%s 0x%x len %lu", &linkname[2], ret->flags,
-	    len);
+	snprintf(statstr, NTP_MAXSTRLEN,
+	    "%s 0x%x len %lu", &linkname[2], ret->flags, len);
 	record_crypto_stats(NULL, statstr);

I was just kidding, MORE!

#endif /* KERNEL_PLL */
	sprintf(statstr, "%s link %d fs %u offset %u", cp, rval, fstamp,
	    ntohl(tai_leap.vallen) / 4 + TAI_1972 - 1);
	record_crypto_stats(NULL, statstr);

Ok, that is either the last one or they died of boredom while patching them..

 #endif /* KERNEL_PLL */
-	sprintf(statstr, "%s link %d fs %u offset %u", cp, rval, fstamp,
+	snprintf(statstr, NTP_MAXSTRLEN, "%s link %d fs %u offset %u", cp, rval, fstamp,
 	    ntohl(tai_leap.vallen) / 4 + TAI_1972 - 1);

If you cannot find a way to trigger any of these… I don’t know… hm.. quit.