On Thu, Jun 09, 2011 at 09:30:02AM -0600, Mike Belopuhov wrote:
>  The problem is that BN_bn2bin skips leading zeroes when it
>  converts a bignum to the char array and we have to prepend
>  zeroes ourselves.  Otherwise peers will compute different
>  DH keys.
>  
>  The following diff seems to fix an issue.  I've been running
>  a tunnel for three days and got 0% ping loss, Pawel has similar
>  results.
>  
>  Index: dh.c
>  ===================================================================
>  RCS file: /home/cvs/src/sbin/isakmpd/dh.c,v
>  retrieving revision 1.13
>  diff -u -p -u -p -r1.13 dh.c
>  --- dh.c     29 Nov 2010 22:49:26 -0000      1.13
>  +++ dh.c     7 Jun 2011 15:13:13 -0000
>  @@ -402,12 +402,22 @@ int
>   modp_create_exchange(struct group *group, u_int8_t *buf)

[cut]

>  @@ -415,15 +425,23 @@ int
>   modp_create_shared(struct group *group, u_int8_t *secret, u_int8_t 
> *exchange)

[cut]

That's right. The above diff fixes the problem of different
Diffie-Hellman shared secret key (g^xy) being calculated on two peers,
which resulted in different authentication and encryption keys being
set on two peers.

As described in the PR and in e-mails sent to bugs (at) openbsd.org
the problem of different DH shared key repeated at least every few
hours when default phase-1 and phase-2 lifetimes were set to
relatively small values:
# grep lifetime /etc/isakmpd/isakmpd.conf
Default-phase-1-lifetime=240,60:86400
Default-phase-2-lifetime=120,60:86400

I've applied the above patch which adds zero padding to:
 - the CVS current version
 - OPENBSD_4_9 version

Both applied cleanly and the problem did not appear for the past three
days. The test were proceeded on Openbsd 4.9 operating system with
isakmpd changed only.

To fix isakmpd provided with OPENBSD_4_8 the zero padding should also
be added. I attach the patch (isakmpd-4.8+patchpr6601.diff), which
applies to OPENBSD_4_8 isakmpd version cleanly.

The tests proceeded successfully in the following testing
environments:
  - Two OpenBSD 4.9 peers with patch applied to isakmpd from CVS current
    version -- 3 days without a problem.
  - Two OpenBSD 4.9 peers with patch applied to isakmpd
    OPENBSD_4_9 branch -- almost 2 days without a problem.
  - One OpenBSD 4.9 peer with isakmpd patched and one OpenBSD 4.6
    peer. -- almost 2 days without a problem

  - Two OpenBSD 4.8 peers with the attached patch applied to
    OPENBSD_4_8 isakmpd version -- almost 3 days without a problem
  - One OpenBSD 4.8 peer with isakmpd patched and one OpenBSD 4.6 
    peer -- almoast 2 days without a problem.
  
I can confirm that adding zero padding fix to the modp_create_exchange() and
modp_create_shared() functions of DH code included in dh.c file fixes
the reported problem on both OpenBSD 4.8 and OpenBSD 4.9, as well as
current CVS version.

Please consider applying the above patch to CVS branches:
 - HEAD
 - OPENBSD_4_9
as it applies cleanly and simply works.

Also please apply the attached patch (isakmpd-4.8+patchpr6601.diff) to
the CVS branch:
 - OPENBSD_4_8
as it does the same and applies cleanly to OPENBSD_4_8.


The commands to apply, build and install the attached patch for
OpenBSD 4.8 users:

obsd48:/usr/src/sbin# patch --directory=isakmpd --strip=1 < 
isakmpd-4.8+patchpr6601.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff -ru isakmpd-ORG/dh.c isakmpd/dh.c
|--- isakmpd-ORG/dh.c   Sat Jul 10 01:40:50 2010
|+++ isakmpd/dh.c       Thu Jun  8 12:23:05 2011
--------------------------
Patching file dh.c using Plan A...
Hunk #1 succeeded at 403.
Hunk #2 succeeded at 428.
done
obsd48:/usr/src/sbin# cd isakmpd
obsd48:/usr/src/sbin/isakmpd# make obj
obsd48:/usr/src/sbin/isakmpd# make
obsd48:/usr/src/sbin/isakmpd# make install

Regards
Pawel Wieleba


P.S.

*The clean patch for OPENBSD_4_8:*

# cat isakmpd-4.8+patchpr6601.diff
diff -ru isakmpd-ORG/dh.c isakmpd/dh.c
--- isakmpd-ORG/dh.c    Sat Jul 10 01:40:50 2010
+++ isakmpd/dh.c        Thu Jun  8 12:23:05 2011
@@ -403,14 +403,24 @@
 {
        int      codes;
        DH      *dh = group->dh;
+       int      len, ret;
 
        if (!DH_generate_key(dh))
                return (-1);
        if (!DH_check(dh, &codes))
                return (-1);
-       if (!BN_bn2bin(dh->pub_key, buf))
+       ret = BN_bn2bin(dh->pub_key, buf);
+       if (!ret)
                return (-1);
 
+       len = dh_getlen(group);
+
+       /* add zero padding */
+       if (ret < len) {
+               bcopy(buf, buf + (len - ret), ret);
+               bzero(buf, len - ret);
+       }
+
        return (0);
 }
 
@@ -418,15 +428,23 @@
 modp_create_shared(struct group *group, u_int8_t *secret, u_int8_t *exchange)
 {
        BIGNUM  *ex;
-       int      ret;
+       int      len, ret;
 
-       if ((ex = BN_bin2bn(exchange, dh_getlen(group), NULL)) == NULL)
+       len = dh_getlen(group);
+
+       if ((ex = BN_bin2bn(exchange, len, NULL)) == NULL)
                return (-1);
 
        ret = DH_compute_key(secret, ex, group->dh);
        BN_clear_free(ex);
        if (!ret)
                return (-1);
+
+       /* add zero padding */
+       if (ret < len) {
+               bcopy(secret, secret + (len - ret), ret);
+               bzero(secret, len - ret);
+       }
 
        return (0);
 }

Reply via email to