<div dir="ltr"><div>Hi Xuelei,</div><div><br></div><div>I did notice that P11Util has a trimZeroes() function, but I didn't call </div><div>it since I didn't want to add a new package dependency. But now that you </div>
<div>mention it, moving it to KeyUtil seems like the best solution. </div><div><br></div><div>I have submitted a revised patch to the Bugzilla ticket which does</div><div>just this:</div><div><br></div><div><a href="https://bugs.openjdk.java.net/attachment.cgi?id=307&action=diff">https://bugs.openjdk.java.net/attachment.cgi?id=307&action=diff</a><br>
</div><div><br></div><div>Best regards,<br></div><div>Pasi</div><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, May 23, 2013 at 1:39 PM, 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"><div class="im"><br>
On 5/23/2013 6:03 PM, Andrew Hughes wrote:<br>
> ----- Original Message -----<br>
>> On 5/20/2013 5:28 PM, Pasi Eronen wrote:<br>
>>> Hi Xuelei,<br>
>>><br>
>>> It seems the PKSC11 doesn't actually have this bug.<br>
>>><br>
>>> P11KeyAgreement has a separate code path for the "TlsPremasterSecret"<br>
>>> algorithm, which strips leading zeroes if the key can be extracted from<br>
>>> the token. (And if the key cannot be extracted, then the token is doing<br>
>>> the premaster secret->master secret computation, and has to do the<br>
>>> stripping -- it can't be done from the Java PKSC11 provider.)<br>
>>><br>
>> It makes sense to me.<br>
>><br>
>>> To make sure this behavior doesn't change, I added a test case<br>
>>> for the PKSC11 provider to the Bugzilla (which passes with the<br>
>>> "SunPKCS11-NSS" provider without any changes).<br>
>>><br>
>> That's great. Would you mind to contribute the regression test for<br>
>> PKCS11 provider?<br>
>><br>
><br>
> It's been attached to the bug report: <a href="https://bugs.openjdk.java.net/show_bug.cgi?id=100316" target="_blank">https://bugs.openjdk.java.net/show_bug.cgi?id=100316</a><br>
><br>
</div>Thanks Andrew!<br>
<div class="im"><br>
> Is there any reason the original patch can't be committed? I haven't seen any mentioned.<br>
><br>
</div>It is accepted. The minor comment I mentioned in the previous mail is<br>
that we may want to merge the functions to trim leading zeros in one method.<br>
<br>
There is a method sun.security.pkcs11.P11Util.trimZeroes(byte[] b) which<br>
is used to trim leading zeros. I think it would be nice to move the<br>
method to sun.security.util.KeyUtil, and make use of this method in the<br>
patch of DHKeyAgreement.engineGenerateSecret(String algorithm) as well.<br>
<br>
Pasi, what do you think?<br>
<br>
Otherwise, the patch looks fine to me.<br>
<br>
I can be the sponsor if you won't able to merge the changes into openJDk<br>
workspace.<br>
<br>
Thanks,<br>
Xuelei<br>
<div class="HOEnZb"><div class="h5"><br>
>> Thanks,<br>
>> Xuelei<br>
>><br>
>>> Best regards,<br>
>>> Pasi<br>
>>><br>
>>><br>
>>><br>
>>> On Thu, May 16, 2013 at 10:56 AM, Xuelei Fan <<a href="mailto:xuelei.fan@oracle.com">xuelei.fan@oracle.com</a><br>
>>> <mailto:<a href="mailto:xuelei.fan@oracle.com">xuelei.fan@oracle.com</a>>> wrote:<br>
>>><br>
>>> 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<br>
>>> 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>
>>><br>
>>> Regards,<br>
>>> Xuelei<br>
>>><br>
>>><br>
>>> On 5/10/2013 5:00 PM, Pasi Eronen wrote:<br>
>>> > 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<br>
>>> > not<br>
>>> > 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>><br>
>>> <<a href="http://bugs.sun.com" target="_blank">http://bugs.sun.com</a>>. I've included the bug<br>
>>> > 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<br>
>>> right<br>
>>> > choice generally speaking (and it's what e.g. IPsec uses), SSL/TLS<br>
>>> 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<br>
>>> > 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<br>
>>> > 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<br>
>>> 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<br>
>>> 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<br>
>>> strip leading<br>
>>> > zero bytes. This causes approximately 1 out 256 SSL/TLS handshakes<br>
>>> with<br>
>>> > DH/DHE cipher suites to fail (when the leading byte happens, by<br>
>>> 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,<br>
>>> 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>
>>> <<a href="http://192.168.81.1:9999" target="_blank">http://192.168.81.1:9999</a>><br>
>>> > <<a href="http://192.168.81.1:9999" target="_blank">http://192.168.81.1:9999</a>>") repeatedly. Other SSL<br>
>>> > clients can be used -- this is not an OpenSSL bug (see below).<br>
>>> ><br>
>>> > 3. Repeat the connection. After a couple of hundred successful<br>
>>> connections,<br>
>>> > the connection will fail with handshake_failure alert.<br>
>>> ><br>
>>> > 4. Examine the JSSE debug logs produced by the server: the failed<br>
>>> 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<br>
>>> .p...8...u=v..Di<br>
>>> > 0010: E7 32 1C EE 80 77 50 C7 A9 51 24 2E E3 15 11 30<br>
>>> .2...wP..Q$....0<br>
>>> > 0020: 9D F6 9F BC 9D EB 5C 18 F7 A4 19 ED 1A AC 2E 0C<br>
>>> ......\.........<br>
>>> > 0030: E3 18 C5 11 B1 80 07 7D B1 C6 70 A8 D7 EB CF DD<br>
>>> ..........p.....<br>
>>> > 0040: 2D B5 1D BC 01 3E 28 2A 2B 5B 38 8F EB 20 F2 A2<br>
>>> -....>(*+[8.. ..<br>
>>> > 0050: 00 07 47 F7 87 B8 99 CB EF B4 13 04 C8 8B 82 FB<br>
>>> ..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)<br>
>>> 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<br>
>>> -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<br>
>>> <a href="http://127.0.0.1:9999" target="_blank">127.0.0.1:9999</a> <<a href="http://127.0.0.1:9999" target="_blank">http://127.0.0.1:9999</a>><br>
>>> > <<a href="http://127.0.0.1:9999" target="_blank">http://127.0.0.1:9999</a>> -quiet -no_ign_eof < /dev/null<br>
>>> > done<br>
>>> ><br>
>>> > Workaround:<br>
>>> > Disable Diffie-Hellman cipher suites.<br>
>>> ><br>
>>> > ---snip---<br>
>>> ><br>
>>><br>
>>><br>
>><br>
>><br>
><br>
<br>
</div></div></blockquote></div><br></div>