Re: 9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-29 Thread Pasi Eronen
Hi Xuelei,

I did notice that P11Util has a trimZeroes() function, but I didn't call
it since I didn't want to add a new package dependency. But now that you
mention it, moving it to KeyUtil seems like the best solution.

I have submitted a revised patch to the Bugzilla ticket which does
just this:

https://bugs.openjdk.java.net/attachment.cgi?id=307action=diff

Best regards,
Pasi



On Thu, May 23, 2013 at 1:39 PM, Xuelei Fan xuelei@oracle.com wrote:


 On 5/23/2013 6:03 PM, Andrew Hughes wrote:
  - Original Message -
  On 5/20/2013 5:28 PM, Pasi Eronen wrote:
  Hi Xuelei,
 
  It seems the PKSC11 doesn't actually have this bug.
 
  P11KeyAgreement has a separate code path for the TlsPremasterSecret
  algorithm, which strips leading zeroes if the key can be extracted from
  the token. (And if the key cannot be extracted, then the token is doing
  the premaster secret-master secret computation, and has to do the
  stripping -- it can't be done from the Java PKSC11 provider.)
 
  It makes sense to me.
 
  To make sure this behavior doesn't change, I added a test case
  for the PKSC11 provider to the Bugzilla (which passes with the
  SunPKCS11-NSS provider without any changes).
 
  That's great.  Would you mind to contribute the regression test for
  PKCS11 provider?
 
 
  It's been attached to the bug report:
 https://bugs.openjdk.java.net/show_bug.cgi?id=100316
 
 Thanks Andrew!

  Is there any reason the original patch can't be committed?  I haven't
 seen any mentioned.
 
 It is accepted.  The minor comment I mentioned in the previous mail is
 that we may want to merge the functions to trim leading zeros in one
 method.

 There is a method sun.security.pkcs11.P11Util.trimZeroes(byte[] b) which
 is used to trim leading zeros.  I think it would be nice to move the
 method to sun.security.util.KeyUtil, and make use of this method in the
 patch of DHKeyAgreement.engineGenerateSecret(String algorithm) as well.

 Pasi, what do you think?

 Otherwise, the patch looks fine to me.

 I can be the sponsor if you won't able to merge the changes into openJDk
 workspace.

 Thanks,
 Xuelei

  Thanks,
  Xuelei
 
  Best regards,
  Pasi
 
 
 
  On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan xuelei@oracle.com
  mailto:xuelei@oracle.com wrote:
 
  Hi Pasi,
 
  Thank you for your patience, and contribution to OpenJDK.  The bug
 is
  accepted, and you should be able to review it at:
 
 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618
 
  Let's use the above bug ID to track the issue.
 
  Your patch looks fine in general (I may have some very minor
 comments
  later).  We also have similar problems in PKCS11 provider because
 of
  the
  update of P11KeyAgreement.java in changeset:
 
 
 http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
 
  Would you like to also fix it in your patch?
 
  Thanks again for your nice work.
 
  Regards,
  Xuelei
 
 
  On 5/10/2013 5:00 PM, Pasi Eronen wrote:
   AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher
 suites
  
   I reported this bug over a month of ago, but for some reason,
 it's
   not
   yet visible at bugs.sun.com http://bugs.sun.com
  http://bugs.sun.com. I've included the bug
   report below just in
   case.
  
   It seems this commit from March 2012 inadvertently broke SSL/TLS
 DHE
   cipher suites, causing the SSL/TLS handshake to fail
 approximately
   1 out of 256 times:
  
   http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
  
   The commit was done to fix this bug:
  
   http://bugs.sun.com/view_bug.do?bug_id=7146728
  
   While generating a secret of the same length as modulus may be
 the
  right
   choice generally speaking (and it's what e.g. IPsec uses),
 SSL/TLS
  uses
   a different convention: leading zeroes must be stripped.
  
   This is currently blocking us from updating our production
 systems to
   Java 7, so although I have not contributed to OpenJDK before, I'd
   like
   to submit a patch and a test case for this (I've signed the OCA
   already). But before I do this, I'd like to check that the
 approach
   is
   agreeable.
  
   We have a separate algorithm value TlsPremasterSecret, so
   behavior for other cases could stay the same. Would a patch
   like this:
  
   } else if (algorithm.equals(TlsPremasterSecret)) {
   // remove leading zero bytes per RFC 5246 Section 8.1.2
   int i = 0;
   while ((i  secret.length - 1)  (secret[i] == 0)) {
   i++;
   }
   if (i == 0) {
   return new SecretKeySpec(secret,
 TlsPremasterSecret);
   } else {
   byte[] secret2 = new byte[secret.length - i];
   System.arraycopy(secret, i, secret2, 0,
 secret2.length);
   

Re: 9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-29 Thread Xuelei Fan
Thank you for considering this improvement.  I'm planing to integrate
the patch into OpenJDK workspace this week.

Thanks  Regards,
Xuelei

On 5/29/2013 3:33 PM, Pasi Eronen wrote:
 Hi Xuelei,
 
 I did notice that P11Util has a trimZeroes() function, but I didn't call 
 it since I didn't want to add a new package dependency. But now that you 
 mention it, moving it to KeyUtil seems like the best solution. 
 
 I have submitted a revised patch to the Bugzilla ticket which does
 just this:
 
 https://bugs.openjdk.java.net/attachment.cgi?id=307action=diff
 
 Best regards,
 Pasi
 
 
 
 On Thu, May 23, 2013 at 1:39 PM, Xuelei Fan xuelei@oracle.com
 mailto:xuelei@oracle.com wrote:
 
 
 On 5/23/2013 6:03 PM, Andrew Hughes wrote:
  - Original Message -
  On 5/20/2013 5:28 PM, Pasi Eronen wrote:
  Hi Xuelei,
 
  It seems the PKSC11 doesn't actually have this bug.
 
  P11KeyAgreement has a separate code path for the
 TlsPremasterSecret
  algorithm, which strips leading zeroes if the key can be
 extracted from
  the token. (And if the key cannot be extracted, then the token
 is doing
  the premaster secret-master secret computation, and has to do the
  stripping -- it can't be done from the Java PKSC11 provider.)
 
  It makes sense to me.
 
  To make sure this behavior doesn't change, I added a test case
  for the PKSC11 provider to the Bugzilla (which passes with the
  SunPKCS11-NSS provider without any changes).
 
  That's great.  Would you mind to contribute the regression test for
  PKCS11 provider?
 
 
  It's been attached to the bug report:
 https://bugs.openjdk.java.net/show_bug.cgi?id=100316
 
 Thanks Andrew!
 
  Is there any reason the original patch can't be committed?  I
 haven't seen any mentioned.
 
 It is accepted.  The minor comment I mentioned in the previous mail is
 that we may want to merge the functions to trim leading zeros in one
 method.
 
 There is a method sun.security.pkcs11.P11Util.trimZeroes(byte[] b) which
 is used to trim leading zeros.  I think it would be nice to move the
 method to sun.security.util.KeyUtil, and make use of this method in the
 patch of DHKeyAgreement.engineGenerateSecret(String algorithm) as well.
 
 Pasi, what do you think?
 
 Otherwise, the patch looks fine to me.
 
 I can be the sponsor if you won't able to merge the changes into openJDk
 workspace.
 
 Thanks,
 Xuelei
 
  Thanks,
  Xuelei
 
  Best regards,
  Pasi
 
 
 
  On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan
 xuelei@oracle.com mailto:xuelei@oracle.com
  mailto:xuelei@oracle.com mailto:xuelei@oracle.com
 wrote:
 
  Hi Pasi,
 
  Thank you for your patience, and contribution to OpenJDK.
  The bug is
  accepted, and you should be able to review it at:
 
 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618
 
  Let's use the above bug ID to track the issue.
 
  Your patch looks fine in general (I may have some very minor
 comments
  later).  We also have similar problems in PKCS11 provider
 because of
  the
  update of P11KeyAgreement.java in changeset:
 
 
 http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
 
  Would you like to also fix it in your patch?
 
  Thanks again for your nice work.
 
  Regards,
  Xuelei
 
 
  On 5/10/2013 5:00 PM, Pasi Eronen wrote:
   AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher
 suites
  
   I reported this bug over a month of ago, but for some
 reason, it's
   not
   yet visible at bugs.sun.com http://bugs.sun.com
 http://bugs.sun.com
  http://bugs.sun.com. I've included the bug
   report below just in
   case.
  
   It seems this commit from March 2012 inadvertently broke
 SSL/TLS DHE
   cipher suites, causing the SSL/TLS handshake to fail
 approximately
   1 out of 256 times:
  
  
 http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
  
   The commit was done to fix this bug:
  
   http://bugs.sun.com/view_bug.do?bug_id=7146728
  
   While generating a secret of the same length as modulus
 may be the
  right
   choice generally speaking (and it's what e.g. IPsec uses),
 SSL/TLS
  uses
   a different convention: leading zeroes must be stripped.
  
   This is currently blocking us from updating our production
 systems to
   Java 7, so although I have not contributed to OpenJDK
 before, I'd
   like
   to submit a patch and a 

Re: 9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-23 Thread Andrew Hughes
- Original Message -
 On 5/20/2013 5:28 PM, Pasi Eronen wrote:
  Hi Xuelei,
  
  It seems the PKSC11 doesn't actually have this bug.
  
  P11KeyAgreement has a separate code path for the TlsPremasterSecret
  algorithm, which strips leading zeroes if the key can be extracted from
  the token. (And if the key cannot be extracted, then the token is doing
  the premaster secret-master secret computation, and has to do the
  stripping -- it can't be done from the Java PKSC11 provider.)
  
 It makes sense to me.
 
  To make sure this behavior doesn't change, I added a test case
  for the PKSC11 provider to the Bugzilla (which passes with the
  SunPKCS11-NSS provider without any changes).
  
 That's great.  Would you mind to contribute the regression test for
 PKCS11 provider?
 

It's been attached to the bug report: 
https://bugs.openjdk.java.net/show_bug.cgi?id=100316

Is there any reason the original patch can't be committed?  I haven't seen any 
mentioned.

 Thanks,
 Xuelei
 
  Best regards,
  Pasi
  
  
  
  On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan xuelei@oracle.com
  mailto:xuelei@oracle.com wrote:
  
  Hi Pasi,
  
  Thank you for your patience, and contribution to OpenJDK.  The bug is
  accepted, and you should be able to review it at:
  
 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618
  
  Let's use the above bug ID to track the issue.
  
  Your patch looks fine in general (I may have some very minor comments
  later).  We also have similar problems in PKCS11 provider because of
  the
  update of P11KeyAgreement.java in changeset:
  
  http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
  
  Would you like to also fix it in your patch?
  
  Thanks again for your nice work.
  
  Regards,
  Xuelei
  
  
  On 5/10/2013 5:00 PM, Pasi Eronen wrote:
   AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher suites
  
   I reported this bug over a month of ago, but for some reason, it's
   not
   yet visible at bugs.sun.com http://bugs.sun.com
  http://bugs.sun.com. I've included the bug
   report below just in
   case.
  
   It seems this commit from March 2012 inadvertently broke SSL/TLS DHE
   cipher suites, causing the SSL/TLS handshake to fail approximately
   1 out of 256 times:
  
   http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
  
   The commit was done to fix this bug:
  
   http://bugs.sun.com/view_bug.do?bug_id=7146728
  
   While generating a secret of the same length as modulus may be the
  right
   choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS
  uses
   a different convention: leading zeroes must be stripped.
  
   This is currently blocking us from updating our production systems to
   Java 7, so although I have not contributed to OpenJDK before, I'd
   like
   to submit a patch and a test case for this (I've signed the OCA
   already). But before I do this, I'd like to check that the approach
   is
   agreeable.
  
   We have a separate algorithm value TlsPremasterSecret, so
   behavior for other cases could stay the same. Would a patch
   like this:
  
   } else if (algorithm.equals(TlsPremasterSecret)) {
   // remove leading zero bytes per RFC 5246 Section 8.1.2
   int i = 0;
   while ((i  secret.length - 1)  (secret[i] == 0)) {
   i++;
   }
   if (i == 0) {
   return new SecretKeySpec(secret, TlsPremasterSecret);
   } else {
   byte[] secret2 = new byte[secret.length - i];
   System.arraycopy(secret, i, secret2, 0, secret2.length);
   return new SecretKeySpec(secret2, TlsPremasterSecret);
   }
   }
  
   Plus a test case (with fixed keys) that checks that leading zero is
   stripped
   for TlsPremasterSecret and is not stripped otherwise, be sufficient?
  
   Best regards,
   Pasi
  
   ---snip---
  
   Synopsis:
   DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times
  
   Full OS version:
   Tested on Windows 7 (Microsoft Windows [Version 6.1.7601]), but
  occurs in
   e..g OpenJDK 7 as well.
  
   Development Kit or Runtime version:
   java version 1.7.0_17
   Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
   Java HotSpot(TM) Client VM (build 23.7-b01, mixed mode, sharing)
  
   Description:
   When performing Diffie-Hellman key agreement for SSL/TLS, the TLS
   specification (RFC 5246) says that Leading bytes of Z that
  contain all zero
   bits are stripped before it is used as the pre_master_secret.
  
   However, com.sun.crypto.provider.DHKeyAgreement.java does not
  

Re: 9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-23 Thread Xuelei Fan

On 5/23/2013 6:03 PM, Andrew Hughes wrote:
 - Original Message -
 On 5/20/2013 5:28 PM, Pasi Eronen wrote:
 Hi Xuelei,

 It seems the PKSC11 doesn't actually have this bug.

 P11KeyAgreement has a separate code path for the TlsPremasterSecret
 algorithm, which strips leading zeroes if the key can be extracted from
 the token. (And if the key cannot be extracted, then the token is doing
 the premaster secret-master secret computation, and has to do the
 stripping -- it can't be done from the Java PKSC11 provider.)

 It makes sense to me.

 To make sure this behavior doesn't change, I added a test case
 for the PKSC11 provider to the Bugzilla (which passes with the
 SunPKCS11-NSS provider without any changes).

 That's great.  Would you mind to contribute the regression test for
 PKCS11 provider?

 
 It's been attached to the bug report: 
 https://bugs.openjdk.java.net/show_bug.cgi?id=100316
 
Thanks Andrew!

 Is there any reason the original patch can't be committed?  I haven't seen 
 any mentioned.
 
It is accepted.  The minor comment I mentioned in the previous mail is
that we may want to merge the functions to trim leading zeros in one method.

There is a method sun.security.pkcs11.P11Util.trimZeroes(byte[] b) which
is used to trim leading zeros.  I think it would be nice to move the
method to sun.security.util.KeyUtil, and make use of this method in the
patch of DHKeyAgreement.engineGenerateSecret(String algorithm) as well.

Pasi, what do you think?

Otherwise, the patch looks fine to me.

I can be the sponsor if you won't able to merge the changes into openJDk
workspace.

Thanks,
Xuelei

 Thanks,
 Xuelei

 Best regards,
 Pasi



 On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan xuelei@oracle.com
 mailto:xuelei@oracle.com wrote:

 Hi Pasi,

 Thank you for your patience, and contribution to OpenJDK.  The bug is
 accepted, and you should be able to review it at:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618

 Let's use the above bug ID to track the issue.

 Your patch looks fine in general (I may have some very minor comments
 later).  We also have similar problems in PKCS11 provider because of
 the
 update of P11KeyAgreement.java in changeset:

 http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6

 Would you like to also fix it in your patch?

 Thanks again for your nice work.

 Regards,
 Xuelei


 On 5/10/2013 5:00 PM, Pasi Eronen wrote:
  AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher suites
 
  I reported this bug over a month of ago, but for some reason, it's
  not
  yet visible at bugs.sun.com http://bugs.sun.com
 http://bugs.sun.com. I've included the bug
  report below just in
  case.
 
  It seems this commit from March 2012 inadvertently broke SSL/TLS DHE
  cipher suites, causing the SSL/TLS handshake to fail approximately
  1 out of 256 times:
 
  http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
 
  The commit was done to fix this bug:
 
  http://bugs.sun.com/view_bug.do?bug_id=7146728
 
  While generating a secret of the same length as modulus may be the
 right
  choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS
 uses
  a different convention: leading zeroes must be stripped.
 
  This is currently blocking us from updating our production systems to
  Java 7, so although I have not contributed to OpenJDK before, I'd
  like
  to submit a patch and a test case for this (I've signed the OCA
  already). But before I do this, I'd like to check that the approach
  is
  agreeable.
 
  We have a separate algorithm value TlsPremasterSecret, so
  behavior for other cases could stay the same. Would a patch
  like this:
 
  } else if (algorithm.equals(TlsPremasterSecret)) {
  // remove leading zero bytes per RFC 5246 Section 8.1.2
  int i = 0;
  while ((i  secret.length - 1)  (secret[i] == 0)) {
  i++;
  }
  if (i == 0) {
  return new SecretKeySpec(secret, TlsPremasterSecret);
  } else {
  byte[] secret2 = new byte[secret.length - i];
  System.arraycopy(secret, i, secret2, 0, secret2.length);
  return new SecretKeySpec(secret2, TlsPremasterSecret);
  }
  }
 
  Plus a test case (with fixed keys) that checks that leading zero is
  stripped
  for TlsPremasterSecret and is not stripped otherwise, be sufficient?
 
  Best regards,
  Pasi
 
  ---snip---
 
  Synopsis:
  DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times
 
  Full OS version:
  Tested on Windows 7 (Microsoft Windows [Version 6.1.7601]), but
 occurs in
  e..g OpenJDK 7 as well.
 
  

Re: 9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-16 Thread Xuelei Fan
Hi Pasi,

Thank you for your patience, and contribution to OpenJDK.  The bug is
accepted, and you should be able to review it at:

   http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618

Let's use the above bug ID to track the issue.

Your patch looks fine in general (I may have some very minor comments
later).  We also have similar problems in PKCS11 provider because of the
update of P11KeyAgreement.java in changeset:

http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6

Would you like to also fix it in your patch?

Thanks again for your nice work.

Regards,
Xuelei


On 5/10/2013 5:00 PM, Pasi Eronen wrote:
 AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher suites
 
 I reported this bug over a month of ago, but for some reason, it's not 
 yet visible at bugs.sun.com http://bugs.sun.com. I've included the bug
 report below just in 
 case. 
 
 It seems this commit from March 2012 inadvertently broke SSL/TLS DHE 
 cipher suites, causing the SSL/TLS handshake to fail approximately 
 1 out of 256 times: 
 
 http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6
 
 The commit was done to fix this bug:
 
 http://bugs.sun.com/view_bug.do?bug_id=7146728
 
 While generating a secret of the same length as modulus may be the right 
 choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS uses 
 a different convention: leading zeroes must be stripped. 
 
 This is currently blocking us from updating our production systems to 
 Java 7, so although I have not contributed to OpenJDK before, I'd like 
 to submit a patch and a test case for this (I've signed the OCA 
 already). But before I do this, I'd like to check that the approach is 
 agreeable. 
 
 We have a separate algorithm value TlsPremasterSecret, so 
 behavior for other cases could stay the same. Would a patch
 like this:
 
 } else if (algorithm.equals(TlsPremasterSecret)) {
 // remove leading zero bytes per RFC 5246 Section 8.1.2
 int i = 0;
 while ((i  secret.length - 1)  (secret[i] == 0)) {
 i++;
 }
 if (i == 0) {
 return new SecretKeySpec(secret, TlsPremasterSecret);
 } else {
 byte[] secret2 = new byte[secret.length - i];
 System.arraycopy(secret, i, secret2, 0, secret2.length);
 return new SecretKeySpec(secret2, TlsPremasterSecret);
 }
 }
 
 Plus a test case (with fixed keys) that checks that leading zero is
 stripped 
 for TlsPremasterSecret and is not stripped otherwise, be sufficient?
 
 Best regards,
 Pasi
 
 ---snip---
 
 Synopsis:
 DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times
 
 Full OS version:
 Tested on Windows 7 (Microsoft Windows [Version 6.1.7601]), but occurs in
 e..g OpenJDK 7 as well.
 
 Development Kit or Runtime version:
 java version 1.7.0_17
 Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
 Java HotSpot(TM) Client VM (build 23.7-b01, mixed mode, sharing)
 
 Description:
 When performing Diffie-Hellman key agreement for SSL/TLS, the TLS
 specification (RFC 5246) says that Leading bytes of Z that contain all zero
 bits are stripped before it is used as the pre_master_secret.
 
 However, com.sun.crypto.provider.DHKeyAgreement.java does not strip leading
 zero bytes. This causes approximately 1 out 256 SSL/TLS handshakes with
 DH/DHE cipher suites to fail (when the leading byte happens, by chance, to
 be zero).
 
 Steps to Reproduce:
 1. Start a simple JSSE socket server with -Djavax.net.debug=all.
 
 2. Connect to the server with e.g. OpenSSL command line tool, ensuring that
 DHE cipher suite gets selected (e.g. openssl s_client -cipher
 DHE-RSA-AES128-SHA -connect 192.168.81.1:
 http://192.168.81.1:) repeatedly. Other SSL
 clients can be used -- this is not an OpenSSL bug (see below).
 
 3. Repeat the connection. After a couple of hundred successful connections,
 the connection will fail with handshake_failure alert.
 
 4. Examine the JSSE debug logs produced by the server: the failed connection
 will have a PreMaster secret that begins with zero byte
 (while all other connections have non-zero byte here). For example:
 
 SESSION KEYGEN:
 PreMaster Secret:
 : 00 70 C5 7E 91 38 C8 DE   ED 75 3D 76 8A B5 44 69  .p...8...u=v..Di
 0010: E7 32 1C EE 80 77 50 C7   A9 51 24 2E E3 15 11 30  .2...wP..Q$0
 0020: 9D F6 9F BC 9D EB 5C 18   F7 A4 19 ED 1A AC 2E 0C  ..\.
 0030: E3 18 C5 11 B1 80 07 7D   B1 C6 70 A8 D7 EB CF DD  ..p.
 0040: 2D B5 1D BC 01 3E 28 2A   2B 5B 38 8F EB 20 F2 A2  -(*+[8.. ..
 0050: 00 07 47 F7 87 B8 99 CB   EF B4 13 04 C8 8B 82 FB  ..G.
 
 Expected Result:
 Expected result is that every connection succeed.
 
 Actual Result:
 Roughly one out of 256 connections fail.
 
 Source code for an executable test case:
 
 Java server:
 
 import javax.net.ssl.SSLServerSocket;
 import javax.net.ssl.SSLServerSocketFactory;
 import javax.net.ssl.SSLSocket;
 
 public class TestServer 

9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

2013-05-10 Thread Pasi Eronen
AKA 1 out of 256 SSL/TLS handshakes fails with DHE cipher suites

I reported this bug over a month of ago, but for some reason, it's not
yet visible at bugs.sun.com. I've included the bug report below just in
case.

It seems this commit from March 2012 inadvertently broke SSL/TLS DHE
cipher suites, causing the SSL/TLS handshake to fail approximately
1 out of 256 times:

http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6

The commit was done to fix this bug:

http://bugs.sun.com/view_bug.do?bug_id=7146728

While generating a secret of the same length as modulus may be the right
choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS uses
a different convention: leading zeroes must be stripped.

This is currently blocking us from updating our production systems to
Java 7, so although I have not contributed to OpenJDK before, I'd like
to submit a patch and a test case for this (I've signed the OCA
already). But before I do this, I'd like to check that the approach is
agreeable.

We have a separate algorithm value TlsPremasterSecret, so
behavior for other cases could stay the same. Would a patch
like this:

} else if (algorithm.equals(TlsPremasterSecret)) {
// remove leading zero bytes per RFC 5246 Section 8.1.2
int i = 0;
while ((i  secret.length - 1)  (secret[i] == 0)) {
i++;
}
if (i == 0) {
return new SecretKeySpec(secret, TlsPremasterSecret);
} else {
byte[] secret2 = new byte[secret.length - i];
System.arraycopy(secret, i, secret2, 0, secret2.length);
return new SecretKeySpec(secret2, TlsPremasterSecret);
}
}

Plus a test case (with fixed keys) that checks that leading zero is
stripped
for TlsPremasterSecret and is not stripped otherwise, be sufficient?

Best regards,
Pasi

---snip---

Synopsis:
DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times

Full OS version:
Tested on Windows 7 (Microsoft Windows [Version 6.1.7601]), but occurs in
e..g OpenJDK 7 as well.

Development Kit or Runtime version:
java version 1.7.0_17
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) Client VM (build 23.7-b01, mixed mode, sharing)

Description:
When performing Diffie-Hellman key agreement for SSL/TLS, the TLS
specification (RFC 5246) says that Leading bytes of Z that contain all zero
bits are stripped before it is used as the pre_master_secret.

However, com.sun.crypto.provider.DHKeyAgreement.java does not strip leading
zero bytes. This causes approximately 1 out 256 SSL/TLS handshakes with
DH/DHE cipher suites to fail (when the leading byte happens, by chance, to
be zero).

Steps to Reproduce:
1. Start a simple JSSE socket server with -Djavax.net.debug=all.

2. Connect to the server with e.g. OpenSSL command line tool, ensuring that
DHE cipher suite gets selected (e.g. openssl s_client -cipher
DHE-RSA-AES128-SHA -connect 192.168.81.1:) repeatedly. Other SSL
clients can be used -- this is not an OpenSSL bug (see below).

3. Repeat the connection. After a couple of hundred successful connections,
the connection will fail with handshake_failure alert.

4. Examine the JSSE debug logs produced by the server: the failed connection
will have a PreMaster secret that begins with zero byte
(while all other connections have non-zero byte here). For example:

SESSION KEYGEN:
PreMaster Secret:
: 00 70 C5 7E 91 38 C8 DE   ED 75 3D 76 8A B5 44 69  .p...8...u=v..Di
0010: E7 32 1C EE 80 77 50 C7   A9 51 24 2E E3 15 11 30  .2...wP..Q$0
0020: 9D F6 9F BC 9D EB 5C 18   F7 A4 19 ED 1A AC 2E 0C  ..\.
0030: E3 18 C5 11 B1 80 07 7D   B1 C6 70 A8 D7 EB CF DD  ..p.
0040: 2D B5 1D BC 01 3E 28 2A   2B 5B 38 8F EB 20 F2 A2  -(*+[8.. ..
0050: 00 07 47 F7 87 B8 99 CB   EF B4 13 04 C8 8B 82 FB  ..G.

Expected Result:
Expected result is that every connection succeed.

Actual Result:
Roughly one out of 256 connections fail.

Source code for an executable test case:

Java server:

import javax.net.ssl.SSLServerSocket;
import javax.net.ssl.SSLServerSocketFactory;
import javax.net.ssl.SSLSocket;

public class TestServer {
public static void main(String args[]) throws Exception {
SSLServerSocketFactory ssf = (SSLServerSocketFactory)
SSLServerSocketFactory.getDefault();
SSLServerSocket ss = (SSLServerSocket) ssf.createServerSocket();
System.out.println(Listening on port );
for (String cs : ss.getEnabledCipherSuites()) {
System.out.println(cs);
}
while (true) {
SSLSocket s = (SSLSocket) ss.accept();
System.out.println(Connected with
+s.getSession().getCipherSuite());
s.close();
}
}
}

Run as as follows:

keytool -storepass password -keypass password -genkey -keyalg RSA
-keystore test_keystore.jks -dname CN=test
javac TestServer.java
java -Djavax.net.debug=all -Djavax.net.ssl.keyStore=./test_keystore.jks