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=307&action=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 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:9999 > <http://192.168.81.1:9999> > >>> <http://192.168.81.1:9999> > >>> > <http://192.168.81.1:9999>") 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: > >>> > 0000: 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(9999); > >>> > System.out.println("Listening on port 9999"); > >>> > 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 > >>> > -Djavax.net.ssl.keyStorePassword=password TestServer > >>> > > >>> > OpenSSL client: > >>> > > >>> > set -e > >>> > while true; do > >>> > openssl s_client -cipher DHE-RSA-AES128-SHA -connect > >>> 127.0.0.1:9999 <http://127.0.0.1:9999> <http://127.0.0.1:9999> > >>> > <http://127.0.0.1:9999> -quiet -no_ign_eof < /dev/null > >>> > done > >>> > > >>> > Workaround: > >>> > Disable Diffie-Hellman cipher suites. > >>> > > >>> > ---snip--- > >>> > > >>> > >>> > >> > >> > > > >
