----- 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 > > 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>") 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> -quiet -no_ign_eof < /dev/null > > > done > > > > > > Workaround: > > > Disable Diffie-Hellman cipher suites. > > > > > > ---snip--- > > > > > > > > > -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
