In ntp-stable:

571 is in crypto_recv(), in the ASSOC|RESP clause, where we copy the peer->subject.

1162 is in crypto_xmit(), in the CERT|RESP clause, where we copy the certname.

1461 is in crypto_verify(), where we check the signature length.

1559 is in crypto_encrypt(), where we get the length of the public key.

2117 is in crypto_bob(), where we get the length of 'r' for the challenge.

In ntp-dev, these areas are:

575 (copying peer->subject)

1170 (copy the certname)

1461 (check the signature length)

1560 (length of the public key)

2122 (length of 'r')

Note to Harlan: see line 1503 in the current ntp_crypto.c

Stephen,

I'll email you separately with the location of a tarball that has the patches described below.

The reported problem at line 571 is now at line 601, and I believe the check at line 529 handles this case.  Do you agree?

The CERT|RESP case that was at 1162 is now near line 1209, and I believe I have fixed that around line 1198.

The reported problem in crypto_verify() that was at 1461 in the old code is at (I believe) line 1561 in the current code.  I believe the check at line 1502 handles this case already.  Do you agree?

The reported problem in crypto_encrypt() that was at line 1559 in the original code is now at line 1599 in the new code.  I think I have this fixed with the new test at line 1350.  Do you agree?

The reported problem that was at line 2117 of crypto_bob() in the old code is now around line 2187 of the new code.  I think I have fixed this with the new test at line 2189.  Do you agree?

> The reported problem at line 571 is now at line 601, and I believe the check at
> line 529 handles this case.  Do you agree?

yes, looks good to me.

> The CERT|RESP case that was at 1162 is now near line 1209, and I believe I have
> fixed that around line 1198.

looks good to me.

> The reported problem in crypto_verify() that was at 1461 in the old code is at
> (I believe) line 1561 in the current code.  I believe the check at line 1502
> handles this case already.  Do you agree?

The code looks like this:
	i = (vallen + 3) / 4;
	siglen = ntohl(ep->pkt[i++]);
	if (len < VALUE_LEN + ((vallen + 3) / 4) * 4 + ((siglen + 3) /
	    4) * 4)
		return (XEVNT_LEN);
"i" is calculated from vallen and used as a read index to ep->pkt. Are there any checks that I'm missing? Also, the calculations in the check afterwards can overflow, for example if vallen is UINT32_MAX-3, the result will be 0 and pass the check.

> The reported problem in crypto_encrypt() that was at line 1559 in the original
> code is now at line 1599 in the new code.  I think I have this fixed with the
new test at line 1350.  Do you agree?

looks good to me

> The reported problem that was at line 2117 of crypto_bob() in the old code is
> now around line 2187 of the new code.  I think I have fixed this with the new
> test at line 2189.  Do you agree?

looks good to me

What about crypto_bob2? That one looks similar to crypto_bob. I think it would be the better approach to verify the packet format once in the beginning instead of doing it every time a value is used. This way it's easy to miss something and it makes it very tough to verify. Is there a reason you decided against that approach?

Stephen,

I put in an "early" check anyway, and I've asked some folks to test this to make sure it hasn't broken anything.  I've made a number of additional changes to the codebase, which I suspect you've now seen.

I'm one of the folks who ran a quick test on this. ;-)

I've set up a server and a client, both running 4.2.8p1-beta5, configured autokey, and verified that the client synchronizes to the server.

Then I replaced first the client only, then the server only, and finally both binaries with the v4.2.8p1-bug2671-RC1 from Harlan's private directory, and I can confirm that each combination still works, at least with the IFF keys. Haven't looked at the patches, yet, so I can't tell if the key type matters.

Fixed in 4.2.8p1.

Hi Harlan,

Myself JGhosh, an opensource developer, working on NTP cherry pick integration from specific Bug 2671 into a FreeBSD private repo.

Would you please kindly confirm the final 2671 Changelist as inline, since myself manually cherry-picking the commits related to Bug 2671 from github into FreeBSD private repository, need your kind help on the same.


Thanks in advance.

Reference:

https://github.com/ntp-project/ntp/blob/stable/ChangeLog


$ git branch
* stable

$ git log --grep="Sec 2671"
commit 5e08c9af76a5e4214bc8369ddf01ee0e86747b3a
Author:  <stenn@psp-deb1.ntp.org>
Date:   Tue Jan 6 10:01:10 2015 +0000

    [Sec 2671] vallen in extension fields are not validated

commit 158d5aa33f5ce3c10f99cdef364ce8e2cb05c4c5
Author:  <stenn@psp-deb1.ntp.org>
Date:   Sat Jan 3 10:33:57 2015 +0000

    [Sec 2671] vallen in extension fields are not validated

commit 348fc9fa390c7894f589104fbca4d635868b7a45
Author:  <stenn@psp-deb1.ntp.org>
Date:   Thu Dec 18 13:14:59 2014 +0000

    [Sec 2671] vallen in extension fields are not validated
joyabrata@jg 172.30.36.127 ~/jg/proj/sw/ntp/git/ntp [Wed Oct 28 15:24:22]



====


$ git log --stat -p 5e08c9af76a5e4214bc8369ddf01ee0e86747b3a
commit 5e08c9af76a5e4214bc8369ddf01ee0e86747b3a
Author:  <stenn@psp-deb1.ntp.org>
Date:   Tue Jan 6 10:01:10 2015 +0000

    [Sec 2671] vallen in extension fields are not validated
---
 ntpd/ntp_crypto.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 65 insertions(+), 35 deletions(-)

diff --git a/ntpd/ntp_crypto.c b/ntpd/ntp_crypto.c
index f8093ad..089dc6f 100644
--- a/ntpd/ntp_crypto.c
+++ b/ntpd/ntp_crypto.c
@@ -139,6 +139,7 @@ static int calcomp(struct calendar *pjd1, struct calendar *pjd2)
 #define TAI_1972       10      /* initial TAI offset (s) */
 #define MAX_LEAP       100     /* max UTC leapseconds (s) */
 #define VALUE_LEN      (6 * 4) /* min response field length */
+#define MAX_VALLEN     (65535 - VALUE_LEN)
 #define YEAR           (60 * 60 * 24 * 365) /* seconds in year */
 
 /*
@@ -179,8 +180,8 @@ static char *rand_file = NULL;      /* random seed file */
  */
 static int     crypto_verify   (struct exten *, struct value *,
                                    struct peer *);
-static int     crypto_encrypt  (struct exten *, struct value *,
-                                   keyid_t *);
+static int     crypto_encrypt  (const u_char *, u_int, keyid_t *,
+                                   struct value *);
 static int     crypto_alice    (struct peer *, struct value *);
 static int     crypto_alice2   (struct peer *, struct value *);
 static int     crypto_alice3   (struct peer *, struct value *);
@@ -475,6 +476,12 @@ crypto_recv(
                if (len >= VALUE_LEN) {
                        fstamp = ntohl(ep->fstamp);
                        vallen = ntohl(ep->vallen);
+                       /*
+                        * Bug 2761: I hope this isn't too early...
+                        */
+                       if (   vallen == 0
+                           || len - VALUE_LEN < vallen)
+                               return XEVNT_LEN;
                }
                switch (code) {
 
@@ -525,8 +532,9 @@ crypto_recv(
                                        rval = XEVNT_ERR;
                                break;
                        }
+                       INSIST(len >= VALUE_LEN);
                        if (vallen == 0 || vallen > MAXHOSTNAME ||
-                           len < VALUE_LEN + vallen) {
+                           len - VALUE_LEN < vallen) {
                                rval = XEVNT_LEN;
                                break;
                        }
@@ -1193,9 +1201,9 @@ crypto_xmit(
         * choice. 
         */
        case CRYPTO_CERT | CRYPTO_RESP:
-               vallen = ntohl(ep->vallen);
+               vallen = ntohl(ep->vallen);     /* Must be <64k */
                if (vallen == 0 || vallen > MAXHOSTNAME ||
-                   len < VALUE_LEN + vallen) {
+                   len - VALUE_LEN < vallen) {
                        rval = XEVNT_LEN;
                        break;
                }
@@ -1345,8 +1353,9 @@ crypto_xmit(
         * anything goes wrong.
         */
        case CRYPTO_COOK | CRYPTO_RESP:
-               vallen = ntohl(ep->vallen);
-               if (vallen == 0
+               vallen = ntohl(ep->vallen);     /* Must be <64k */
+               if (   vallen == 0
+                   || (vallen >= MAX_VALLEN)
                    || (opcode & 0x0000ffff)  < VALUE_LEN + vallen) {
                        rval = XEVNT_LEN;
                        break;
@@ -1355,8 +1364,8 @@ crypto_xmit(
                        tcookie = cookie;
                else
                        tcookie = peer->hcookie;
-               if ((rval = crypto_encrypt(ep, &vtemp, &tcookie)) ==
-                   XEVNT_OK) {
+               if ((rval = crypto_encrypt((const u_char *)ep->pkt, vallen, &tcookie, &vtemp))
+                   == XEVNT_OK) {
                        len = crypto_send(fp, &vtemp, start);
                        value_free(&vtemp);
                }
@@ -1496,13 +1505,16 @@ crypto_verify(
         * up to the next word (4 octets).
         */
        vallen = ntohl(ep->vallen);
-       if (vallen == 0)
+       if (   vallen == 0
+           || vallen > MAX_VALLEN)
                return (XEVNT_LEN);
 
        i = (vallen + 3) / 4;
        siglen = ntohl(ep->pkt[i++]);
-       if (len < VALUE_LEN + ((vallen + 3) / 4) * 4 + ((siglen + 3) /
-           4) * 4)
+       if (   siglen > MAX_VALLEN
+           || len - VALUE_LEN < ((vallen + 3) / 4) * 4
+           || len - VALUE_LEN - ((vallen + 3) / 4) * 4
+             < ((siglen + 3) / 4) * 4)
                return (XEVNT_LEN);
 
        /*
@@ -1560,6 +1572,7 @@ crypto_verify(
         * proventic bit. What a relief.
         */
        EVP_VerifyInit(&ctx, peer->digest);
+       /* XXX: the "+ 12" needs to be at least documented... */
        EVP_VerifyUpdate(&ctx, (u_char *)&ep->tstamp, vallen + 12);
        if (EVP_VerifyFinal(&ctx, (u_char *)&ep->pkt[i], siglen,
            pkey) <= 0)
@@ -1572,35 +1585,32 @@ crypto_verify(
 
 
 /*
- * crypto_encrypt - construct encrypted cookie and signature from
- * extension field and cookie
+ * crypto_encrypt - construct vp (encrypted cookie and signature) from
+ * the public key and cookie.
  *
- * Returns
+ * Returns:
  * XEVNT_OK    success
  * XEVNT_CKY   bad or missing cookie
  * XEVNT_PUB   bad or missing public key
  */
 static int
 crypto_encrypt(
-       struct exten *ep,       /* extension pointer */
-       struct value *vp,       /* value pointer */
-       keyid_t *cookie         /* server cookie */
+       const u_char *ptr,      /* Public Key */
+       u_int   vallen,         /* Length of Public Key */
+       keyid_t *cookie,        /* server cookie */
+       struct value *vp        /* value pointer */
        )
 {
        EVP_PKEY *pkey;         /* public key */
        EVP_MD_CTX ctx;         /* signature context */
        tstamp_t tstamp;        /* NTP timestamp */
        u_int32 temp32;
-       u_int   len;
-       const u_char *ptr;
        u_char *puch;
 
        /*
         * Extract the public key from the request.
         */
-       len = ntohl(ep->vallen);
-       ptr = (void *)ep->pkt;
-       pkey = d2i_PublicKey(EVP_PKEY_RSA, NULL, &ptr, len);
+       pkey = d2i_PublicKey(EVP_PKEY_RSA, NULL, &ptr, vallen);
        if (pkey == NULL) {
                msyslog(LOG_ERR, "crypto_encrypt: %s",
                    ERR_error_string(ERR_get_error(), NULL));
@@ -1614,9 +1624,9 @@ crypto_encrypt(
        tstamp = crypto_time();
        vp->tstamp = htonl(tstamp);
        vp->fstamp = hostval.tstamp;
-       len = EVP_PKEY_size(pkey);
-       vp->vallen = htonl(len);
-       vp->ptr = emalloc(len);
+       vallen = EVP_PKEY_size(pkey);
+       vp->vallen = htonl(vallen);
+       vp->ptr = emalloc(vallen);
        puch = vp->ptr;
        temp32 = htonl(*cookie);
        if (RSA_public_encrypt(4, (u_char *)&temp32, puch,
@@ -1634,8 +1644,8 @@ crypto_encrypt(
        vp->sig = emalloc(sign_siglen);
        EVP_SignInit(&ctx, sign_digest);
        EVP_SignUpdate(&ctx, (u_char *)&vp->tstamp, 12);
-       EVP_SignUpdate(&ctx, vp->ptr, len);
-       if (EVP_SignFinal(&ctx, vp->sig, &len, sign_pkey))
+       EVP_SignUpdate(&ctx, vp->ptr, vallen);
+       if (EVP_SignFinal(&ctx, vp->sig, &vallen, sign_pkey))
                vp->siglen = htonl(sign_siglen);
        return (XEVNT_OK);
 }
@@ -1706,6 +1716,9 @@ crypto_ident(
  * call in the protocol module.
  *
  * Returns extension field pointer (no errors)
+ *
+ * XXX: opcode and len should really be 32-bit quantities and
+ * we should make sure that str is not too big.
  */
 struct exten *
 crypto_args(
@@ -1718,23 +1731,30 @@ crypto_args(
        tstamp_t tstamp;        /* NTP timestamp */
        struct exten *ep;       /* extension field pointer */
        u_int   len;            /* extension field length */
+       size_t  slen;
 
        tstamp = crypto_time();
        len = sizeof(struct exten);
-       if (str != NULL)
-               len += strlen(str);
+       if (str != NULL) {
+               slen = strlen(str);
+               INSIST(slen < MAX_VALLEN);
+               len += slen;
+       }
        ep = emalloc_zero(len);
        if (opcode == 0)
                return (ep);
 
+       REQUIRE(0 == (len    & ~0x0000ffff));
+       REQUIRE(0 == (opcode & ~0xffff0000));
+
        ep->opcode = htonl(opcode + len);
        ep->associd = htonl(associd);
        ep->tstamp = htonl(tstamp);
        ep->fstamp = hostval.tstamp;
        ep->vallen = 0;
        if (str != NULL) {
-               ep->vallen = htonl(strlen(str));
-               memcpy((char *)ep->pkt, str, strlen(str));
+               ep->vallen = htonl(slen);
+               memcpy((char *)ep->pkt, str, slen);
        }
        return (ep);
 }
@@ -1747,6 +1767,8 @@ crypto_args(
  * Note: it is not polite to send a nonempty signature with zero
  * timestamp or a nonzero timestamp with an empty signature, but those
  * rules are not enforced here.
+ *
+ * XXX This code won't work on a box with 16-bit ints.
  */
 int
 crypto_send(
@@ -1762,8 +1784,9 @@ crypto_send(
         * Calculate extension field length and check for buffer
         * overflow. Leave room for the MAC.
         */
-       len = 16;
+       len = 16;                               /* XXX Document! */
        vallen = ntohl(vp->vallen);
+       INSIST(vallen <= MAX_VALLEN);
        len += ((vallen + 3) / 4 + 1) * 4; 
        siglen = ntohl(vp->siglen);
        len += ((siglen + 3) / 4 + 1) * 4; 
@@ -1804,6 +1827,7 @@ crypto_send(
        }
        opcode = ntohl(ep->opcode);
        ep->opcode = htonl((opcode & 0xffff0000) | len); 
+       ENSURE(len <= MAX_VALLEN);
        return (len);
 }
 
@@ -1840,7 +1864,6 @@ crypto_update(void)
        if (hostval.tstamp == 0)
                return;
 
-
        /*
         * Sign public key and timestamps. The filestamp is derived from
         * the host key file extension from wherever the file was
@@ -2186,7 +2209,7 @@ crypto_bob(
         */
        vallen = ntohl(ep->vallen);
        len = ntohl(ep->opcode) & 0x0000ffff;
-       if (vallen == 0 || len < VALUE_LEN + vallen)
+       if (vallen == 0 || len < VALUE_LEN || len - VALUE_LEN < vallen)
                return XEVNT_LEN;
        if ((r = BN_bin2bn((u_char *)ep->pkt, vallen, NULL)) == NULL) {
                msyslog(LOG_ERR, "crypto_bob: %s",
@@ -2226,6 +2249,12 @@ crypto_bob(
                DSA_SIG_free(sdsa);
                return (XEVNT_ERR);
        }
+       if (vallen > MAX_VALLEN) {
+               msyslog(LOG_ERR, "crypto_bob: signature is too big: %d",
+                   vallen);
+               DSA_SIG_free(sdsa);
+               return (XEVNT_LEN);
+       }
        memset(vp, 0, sizeof(struct value));
        tstamp = crypto_time();
        vp->tstamp = htonl(tstamp);
@@ -2238,6 +2267,7 @@ crypto_bob(
        if (tstamp == 0)
                return (XEVNT_OK);
 
+       /* XXX: more validation to make sure the sign fits... */
        vp->sig = emalloc(sign_siglen);
        EVP_SignInit(&ctx, sign_digest);
        EVP_SignUpdate(&ctx, (u_char *)&vp->tstamp, 12);



====



commit 158d5aa33f5ce3c10f99cdef364ce8e2cb05c4c5
Author:  <stenn@psp-deb1.ntp.org>
Date:   Sat Jan 3 10:33:57 2015 +0000

    [Sec 2671] vallen in extension fields are not validated
---
 ChangeLog         |  3 +++
 ntpd/ntp_crypto.c | 31 +++++++++++++++++++------------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index afd18da..441dc7b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,7 @@
 ---
+
+* [Sec 2671] vallen in extension fields are not validated.
+---
 (4.2.8p1-beta3) 2015/01/02 Released by Harlan Stenn <stenn@ntp.org>
 
 * [Bug 2627] shm refclock allows only two units with owner-only access
diff --git a/ntpd/ntp_crypto.c b/ntpd/ntp_crypto.c
index 4f27ab8..f8093ad 100644
--- a/ntpd/ntp_crypto.c
+++ b/ntpd/ntp_crypto.c
@@ -1194,7 +1194,8 @@ crypto_xmit(
         */
        case CRYPTO_CERT | CRYPTO_RESP:
                vallen = ntohl(ep->vallen);
-               if (vallen == 0 || vallen > MAXHOSTNAME) {
+               if (vallen == 0 || vallen > MAXHOSTNAME ||
+                   len < VALUE_LEN + vallen) {
                        rval = XEVNT_LEN;
                        break;
                }
@@ -1344,7 +1345,9 @@ crypto_xmit(
         * anything goes wrong.
         */
        case CRYPTO_COOK | CRYPTO_RESP:
-               if ((opcode & 0xffff) < VALUE_LEN) {
+               vallen = ntohl(ep->vallen);
+               if (vallen == 0
+                   || (opcode & 0x0000ffff)  < VALUE_LEN + vallen) {
                        rval = XEVNT_LEN;
                        break;
                }
@@ -2165,7 +2168,8 @@ crypto_bob(
        tstamp_t tstamp;        /* NTP timestamp */
        BIGNUM  *bn, *bk, *r;
        u_char  *ptr;
-       u_int   len;
+       u_int   len;            /* extension field length */
+       u_int   vallen = 0;     /* value length */
 
        /*
         * If the IFF parameters are not valid, something awful
@@ -2180,8 +2184,11 @@ crypto_bob(
        /*
         * Extract r from the challenge.
         */
-       len = ntohl(ep->vallen);
-       if ((r = BN_bin2bn((u_char *)ep->pkt, len, NULL)) == NULL) {
+       vallen = ntohl(ep->vallen);
+       len = ntohl(ep->opcode) & 0x0000ffff;
+       if (vallen == 0 || len < VALUE_LEN + vallen)
+               return XEVNT_LEN;
+       if ((r = BN_bin2bn((u_char *)ep->pkt, vallen, NULL)) == NULL) {
                msyslog(LOG_ERR, "crypto_bob: %s",
                    ERR_error_string(ERR_get_error(), NULL));
                return (XEVNT_ERR);
@@ -2193,7 +2200,7 @@ crypto_bob(
         */
        bctx = BN_CTX_new(); bk = BN_new(); bn = BN_new();
        sdsa = DSA_SIG_new();
-       BN_rand(bk, len * 8, -1, 1);            /* k */
+       BN_rand(bk, vallen * 8, -1, 1);         /* k */
        BN_mod_mul(bn, dsa->priv_key, r, dsa->q, bctx); /* b r mod q */
        BN_add(bn, bn, bk);
        BN_mod(bn, bn, dsa->q, bctx);           /* k + b r mod q */
@@ -2212,8 +2219,8 @@ crypto_bob(
         * Encode the values in ASN.1 and sign. The filestamp is from
         * the local file.
         */
-       len = i2d_DSA_SIG(sdsa, NULL);
-       if (len == 0) {
+       vallen = i2d_DSA_SIG(sdsa, NULL);
+       if (vallen == 0) {
                msyslog(LOG_ERR, "crypto_bob: %s",
                    ERR_error_string(ERR_get_error(), NULL));
                DSA_SIG_free(sdsa);
@@ -2223,8 +2230,8 @@ crypto_bob(
        tstamp = crypto_time();
        vp->tstamp = htonl(tstamp);
        vp->fstamp = htonl(iffkey_info->fstamp);
-       vp->vallen = htonl(len);
-       ptr = emalloc(len);
+       vp->vallen = htonl(vallen);
+       ptr = emalloc(vallen);
        vp->ptr = ptr;
        i2d_DSA_SIG(sdsa, &ptr);
        DSA_SIG_free(sdsa);
@@ -2234,8 +2241,8 @@ crypto_bob(
        vp->sig = emalloc(sign_siglen);
        EVP_SignInit(&ctx, sign_digest);
        EVP_SignUpdate(&ctx, (u_char *)&vp->tstamp, 12);
-       EVP_SignUpdate(&ctx, vp->ptr, len);
-       if (EVP_SignFinal(&ctx, vp->sig, &len, sign_pkey))
+       EVP_SignUpdate(&ctx, vp->ptr, vallen);
+       if (EVP_SignFinal(&ctx, vp->sig, &vallen, sign_pkey))
                vp->siglen = htonl(sign_siglen);
        return (XEVNT_OK);
 }



====


$ git log --stat -p 348fc9fa390c7894f589104fbca4d635868b7a45
commit 348fc9fa390c7894f589104fbca4d635868b7a45
Author:  <stenn@psp-deb1.ntp.org>
Date:   Thu Dec 18 13:14:59 2014 +0000

    [Sec 2671] vallen in extension fields are not validated
---
 ChangeLog        |  1 +
 ntpd/ntp_proto.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 3e4b518..825c748 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,7 @@
 * [Sec 2668] buffer overflow in ctl_putdata().
 * [Sec 2669] buffer overflow in configure().
 * [Sec 2670] Missing return; from error clause.
+* [Sec 2671] vallen in extension fields are not validated.
 * [Sec 2672] On some OSes ::1 can be spoofed, bypassing source IP ACLs.
 (4.2.7p486-RC) 2014/12/18 Released by Harlan Stenn <stenn@ntp.org>
 * [Bug 2687] RefClock 26/hpgps doesn't work at default line speed
diff --git a/ntpd/ntp_proto.c b/ntpd/ntp_proto.c
index e658b64..091fcf5 100644
--- a/ntpd/ntp_proto.c
+++ b/ntpd/ntp_proto.c
@@ -486,7 +486,7 @@ receive(
         */
        authlen = LEN_PKT_NOMAC;
        has_mac = rbufp->recv_length - authlen;
-       while (has_mac != 0) {
+       while (has_mac > 0) {
                u_int32 len;
 #ifdef AUTOKEY
                u_int32 hostlen;
@@ -541,6 +541,14 @@ receive(
        }
 
        /*
+        * If has_mac is < 0 we had a malformed packet.
+        */
+       if (has_mac < 0) {
+               sys_badlength++;
+               return;         /* bad length */
+       }
+
+       /*
         * If authentication required, a MAC must be present.
         */
        if (restrict_mask & RES_DONTTRUST && has_mac == 0) {