9001039?: DHKeyAgreement calculates wrong TlsPremasterSecret 1 out of 256 times
Pasi Eronen
pe at iki.fi
Wed May 29 07:33:53 UTC 2013
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.fan at 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.fan at oracle.com
> >>> <mailto:xuelei.fan at 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---
> >>> >
> >>>
> >>>
> >>
> >>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20130529/682035e3/attachment.htm>
More information about the security-dev
mailing list