<div dir="ltr">Hi Xuelei,<div><br></div><div><div>It seems the PKSC11 doesn't actually have this bug.</div><div><br></div><div>P11KeyAgreement has a separate code path for the "TlsPremasterSecret" </div><div>
algorithm, which strips leading zeroes if the key can be extracted from </div><div>the token. (And if the key cannot be extracted, then the token is doing </div><div>the premaster secret->master secret computation, and has to do the </div>
<div>stripping -- it can't be done from the Java PKSC11 provider.) </div><div><br></div><div>To make sure this behavior doesn't change, I added a test case </div><div>for the PKSC11 provider to the Bugzilla (which passes with the </div>
<div>"SunPKCS11-NSS" provider without any changes).</div></div><div><br></div><div style>Best regards,</div><div style>Pasi</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan <span dir="ltr"><<a href="mailto:xuelei.fan@oracle.com" target="_blank">xuelei.fan@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pasi,<br>
<br>
Thank you for your patience, and contribution to OpenJDK. The bug is<br>
accepted, and you should be able to review it at:<br>
<br>
<a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618" target="_blank">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014618</a><br>
<br>
Let's use the above bug ID to track the issue.<br>
<br>
Your patch looks fine in general (I may have some very minor comments<br>
later). We also have similar problems in PKCS11 provider because of the<br>
update of P11KeyAgreement.java in changeset:<br>
<br>
<a href="http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6" target="_blank">http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6</a><br>
<br>
Would you like to also fix it in your patch?<br>
<br>
Thanks again for your nice work.<br>
<div class="im"><br>
Regards,<br>
Xuelei<br>
<br>
<br>
On 5/10/2013 5:00 PM, Pasi Eronen wrote:<br>
</div><div class="im">> AKA "1 out of 256 SSL/TLS handshakes fails with DHE cipher suites"<br>
><br>
> I reported this bug over a month of ago, but for some reason, it's not<br>
</div>> yet visible at <a href="http://bugs.sun.com" target="_blank">bugs.sun.com</a> <<a href="http://bugs.sun.com" target="_blank">http://bugs.sun.com</a>>. I've included the bug<br>
<div><div class="h5">> report below just in<br>
> case.<br>
><br>
> It seems this commit from March 2012 inadvertently broke SSL/TLS DHE<br>
> cipher suites, causing the SSL/TLS handshake to fail approximately<br>
> 1 out of 256 times:<br>
><br>
> <a href="http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6" target="_blank">http://hg.openjdk.java.net/jdk7u/jdk7u-gate/jdk/rev/e574e475c8a6</a><br>
><br>
> The commit was done to fix this bug:<br>
><br>
> <a href="http://bugs.sun.com/view_bug.do?bug_id=7146728" target="_blank">http://bugs.sun.com/view_bug.do?bug_id=7146728</a><br>
><br>
> While generating a secret of the same length as modulus may be the right<br>
> choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS uses<br>
> a different convention: leading zeroes must be stripped.<br>
><br>
> This is currently blocking us from updating our production systems to<br>
> Java 7, so although I have not contributed to OpenJDK before, I'd like<br>
> to submit a patch and a test case for this (I've signed the OCA<br>
> already). But before I do this, I'd like to check that the approach is<br>
> agreeable.<br>
><br>
> We have a separate "algorithm" value "TlsPremasterSecret", so<br>
> behavior for other cases could stay the same. Would a patch<br>
> like this:<br>
><br>
> } else if (algorithm.equals("TlsPremasterSecret")) {<br>
> // remove leading zero bytes per RFC 5246 Section 8.1.2<br>
> int i = 0;<br>
> while ((i < secret.length - 1) && (secret[i] == 0)) {<br>
> i++;<br>
> }<br>
> if (i == 0) {<br>
> return new SecretKeySpec(secret, "TlsPremasterSecret");<br>
> } else {<br>
> byte[] secret2 = new byte[secret.length - i];<br>
> System.arraycopy(secret, i, secret2, 0, secret2.length);<br>
> return new SecretKeySpec(secret2, "TlsPremasterSecret");<br>
> }<br>
> }<br>
><br>
> Plus a test case (with fixed keys) that checks that leading zero is<br>
> stripped<br>
> for TlsPremasterSecret and is not stripped otherwise, be sufficient?<br>
><br>
> Best regards,<br>
> Pasi<br>
><br>
> ---snip---<br>
><br>
> Synopsis:<br>
> DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times<br>
><br>
> Full OS version:<br>
> Tested on Windows 7 (Microsoft Windows [Version 6.1.7601]), but occurs in<br>
> e..g OpenJDK 7 as well.<br>
><br>
> Development Kit or Runtime version:<br>
> java version "1.7.0_17"<br>
> Java(TM) SE Runtime Environment (build 1.7.0_17-b02)<br>
> Java HotSpot(TM) Client VM (build 23.7-b01, mixed mode, sharing)<br>
><br>
> Description:<br>
> When performing Diffie-Hellman key agreement for SSL/TLS, the TLS<br>
> specification (RFC 5246) says that "Leading bytes of Z that contain all zero<br>
> bits are stripped before it is used as the pre_master_secret."<br>
><br>
> However, com.sun.crypto.provider.DHKeyAgreement.java does not strip leading<br>
> zero bytes. This causes approximately 1 out 256 SSL/TLS handshakes with<br>
> DH/DHE cipher suites to fail (when the leading byte happens, by chance, to<br>
> be zero).<br>
><br>
> Steps to Reproduce:<br>
> 1. Start a simple JSSE socket server with -Djavax.net.debug=all.<br>
><br>
> 2. Connect to the server with e.g. OpenSSL command line tool, ensuring that<br>
> DHE cipher suite gets selected (e.g. "openssl s_client -cipher<br>
> DHE-RSA-AES128-SHA -connect <a href="http://192.168.81.1:9999" target="_blank">192.168.81.1:9999</a><br>
</div></div>> <<a href="http://192.168.81.1:9999" target="_blank">http://192.168.81.1:9999</a>>") repeatedly. Other SSL<br>
<div><div class="h5">> clients can be used -- this is not an OpenSSL bug (see below).<br>
><br>
> 3. Repeat the connection. After a couple of hundred successful connections,<br>
> the connection will fail with handshake_failure alert.<br>
><br>
> 4. Examine the JSSE debug logs produced by the server: the failed connection<br>
> will have a PreMaster secret that begins with zero byte<br>
> (while all other connections have non-zero byte here). For example:<br>
><br>
> SESSION KEYGEN:<br>
> PreMaster Secret:<br>
> 0000: 00 70 C5 7E 91 38 C8 DE ED 75 3D 76 8A B5 44 69 .p...8...u=v..Di<br>
> 0010: E7 32 1C EE 80 77 50 C7 A9 51 24 2E E3 15 11 30 .2...wP..Q$....0<br>
> 0020: 9D F6 9F BC 9D EB 5C 18 F7 A4 19 ED 1A AC 2E 0C ......\.........<br>
> 0030: E3 18 C5 11 B1 80 07 7D B1 C6 70 A8 D7 EB CF DD ..........p.....<br>
> 0040: 2D B5 1D BC 01 3E 28 2A 2B 5B 38 8F EB 20 F2 A2 -....>(*+[8.. ..<br>
> 0050: 00 07 47 F7 87 B8 99 CB EF B4 13 04 C8 8B 82 FB ..G.............<br>
><br>
> Expected Result:<br>
> Expected result is that every connection succeed.<br>
><br>
> Actual Result:<br>
> Roughly one out of 256 connections fail.<br>
><br>
> Source code for an executable test case:<br>
><br>
> Java server:<br>
><br>
> import javax.net.ssl.SSLServerSocket;<br>
> import javax.net.ssl.SSLServerSocketFactory;<br>
> import javax.net.ssl.SSLSocket;<br>
><br>
> public class TestServer {<br>
> public static void main(String args[]) throws Exception {<br>
> SSLServerSocketFactory ssf = (SSLServerSocketFactory)<br>
> SSLServerSocketFactory.getDefault();<br>
> SSLServerSocket ss = (SSLServerSocket) ssf.createServerSocket(9999);<br>
> System.out.println("Listening on port 9999");<br>
> for (String cs : ss.getEnabledCipherSuites()) {<br>
> System.out.println(cs);<br>
> }<br>
> while (true) {<br>
> SSLSocket s = (SSLSocket) ss.accept();<br>
> System.out.println("Connected with<br>
> "+s.getSession().getCipherSuite());<br>
> s.close();<br>
> }<br>
> }<br>
> }<br>
><br>
> Run as as follows:<br>
><br>
> keytool -storepass "password" -keypass "password" -genkey -keyalg RSA<br>
> -keystore test_keystore.jks -dname CN=test<br>
> javac TestServer.java<br>
> java -Djavax.net.debug=all -Djavax.net.ssl.keyStore=./test_keystore.jks<br>
> -Djavax.net.ssl.keyStorePassword=password TestServer<br>
><br>
> OpenSSL client:<br>
><br>
> set -e<br>
> while true; do<br>
> openssl s_client -cipher DHE-RSA-AES128-SHA -connect <a href="http://127.0.0.1:9999" target="_blank">127.0.0.1:9999</a><br>
</div></div>> <<a href="http://127.0.0.1:9999" target="_blank">http://127.0.0.1:9999</a>> -quiet -no_ign_eof < /dev/null<br>
<div class="HOEnZb"><div class="h5">> done<br>
><br>
> Workaround:<br>
> Disable Diffie-Hellman cipher suites.<br>
><br>
> ---snip---<br>
><br>
<br>
</div></div></blockquote></div><br></div>