# HG changeset patch # User valeriep # Date 1332281173 25200 # Node ID d77ed23f4992efc1ae890e9ee1e6023a889044d3 # Parent d935c2f4aeae5cb7b2f5343d62009998b6661673 7146728: Inconsistent length for the generated secret using DH key agreement impl from SunJCE and PKCS11 Summary: Always return the secret in the same length as the modulus. Reviewed-by: wetmore diff -r d935c2f4aeae -r d77ed23f4992 jdk/src/share/classes/com/sun/crypto/provider/DHKeyAgreement.java --- a/jdk/src/share/classes/com/sun/crypto/provider/DHKeyAgreement.java Tue Mar 20 12:48:48 2012 +0100 +++ b/jdk/src/share/classes/com/sun/crypto/provider/DHKeyAgreement.java Tue Mar 20 15:06:13 2012 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2009, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -33,6 +33,7 @@ import java.security.Key; import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; +import java.security.ProviderException; import java.security.spec.AlgorithmParameterSpec; import java.security.spec.InvalidKeySpecException; import javax.crypto.KeyAgreementSpi; @@ -234,31 +235,14 @@ protected byte[] engineGenerateSecret() throws IllegalStateException { - if (generateSecret == false) { - throw new IllegalStateException - ("Key agreement has not been completed yet"); + int expectedLen = (init_p.bitLength() + 7) >>> 3; + byte[] result = new byte[expectedLen]; + try { + engineGenerateSecret(result, 0); + } catch (ShortBufferException sbe) { + // should never happen since length are identical } - - // Reset the key agreement here (in case anything goes wrong) - generateSecret = false; - - // get the modulus - BigInteger modulus = init_p; - - BigInteger tmpResult = y.modPow(x, modulus); - byte[] secret = tmpResult.toByteArray(); - - /* - * BigInteger.toByteArray will sometimes put a sign byte up front, but - * we NEVER want one. - */ - if ((tmpResult.bitLength() % 8) == 0) { - byte retval[] = new byte[secret.length - 1]; - System.arraycopy(secret, 1, retval, 0, retval.length); - return retval; - } else { - return secret; - } + return result; } /** @@ -301,39 +285,51 @@ } BigInteger modulus = init_p; - byte[] secret = this.y.modPow(this.x, modulus).toByteArray(); - - // BigInteger.toByteArray will sometimes put a sign byte up front, - // but we NEVER want one. - if ((secret.length << 3) != modulus.bitLength()) { - if ((sharedSecret.length - offset) < (secret.length - 1)) { - throw new ShortBufferException + int expectedLen = (modulus.bitLength() + 7) >>> 3; + if ((sharedSecret.length - offset) < expectedLen) { + throw new ShortBufferException ("Buffer too short for shared secret"); - } - System.arraycopy(secret, 1, sharedSecret, offset, - secret.length - 1); + } - // Reset the key agreement here (not earlier!), so that people - // can recover from ShortBufferException above without losing - // internal state - generateSecret = false; + // Reset the key agreement after checking for ShortBufferException + // above, so user can recover w/o losing internal state + generateSecret = false; - return secret.length - 1; - + /* + * NOTE: BigInteger.toByteArray() returns a byte array containing + * the two's-complement representation of this BigInteger with + * the most significant byte is in the zeroth element. This + * contains the minimum number of bytes required to represent + * this BigInteger, including at least one sign bit whose value + * is always 0. + * + * Keys are always positive, and the above sign bit isn't + * actually used when representing keys. (i.e. key = new + * BigInteger(1, byteArray)) To obtain an array containing + * exactly expectedLen bytes of magnitude, we strip any extra + * leading 0's, or pad with 0's in case of a "short" secret. + */ + byte[] secret = this.y.modPow(this.x, modulus).toByteArray(); + if (secret.length == expectedLen) { + System.arraycopy(secret, 0, sharedSecret, offset, + secret.length); } else { - if ((sharedSecret.length - offset) < secret.length) { - throw new ShortBufferException - ("Buffer too short to hold shared secret"); + // Array too short, pad it w/ leading 0s + if (secret.length < expectedLen) { + System.arraycopy(secret, 0, sharedSecret, + offset + (expectedLen - secret.length), + secret.length); + } else { + // Array too long, check and trim off the excess + if ((secret.length == (expectedLen+1)) && secret[0] == 0) { + // ignore the leading sign byte + System.arraycopy(secret, 1, sharedSecret, offset, expectedLen); + } else { + throw new ProviderException("Generated secret is out-of-range"); + } } - System.arraycopy(secret, 0, sharedSecret, offset, secret.length); - - // Reset the key agreement here (not earlier!), so that people - // can recover from ShortBufferException above without losing - // internal state - generateSecret = false; - - return secret.length; } + return expectedLen; } /** diff -r d935c2f4aeae -r d77ed23f4992 jdk/src/share/classes/sun/security/pkcs11/P11KeyAgreement.java --- a/jdk/src/share/classes/sun/security/pkcs11/P11KeyAgreement.java Tue Mar 20 12:48:48 2012 +0100 +++ b/jdk/src/share/classes/sun/security/pkcs11/P11KeyAgreement.java Tue Mar 20 15:06:13 2012 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2012, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -198,8 +198,22 @@ token.p11.C_GetAttributeValue(session.id(), keyID, attributes); byte[] secret = attributes[0].getByteArray(); token.p11.C_DestroyObject(session.id(), keyID); - // trim leading 0x00 bytes per JCE convention - return P11Util.trimZeroes(secret); + // Some vendors, e.g. NSS, trim off the leading 0x00 byte(s) from + // the generated secret. Thus, we need to check the secret length + // and trim/pad it so the returned value has the same length as + // the modulus size + if (secret.length == secretLen) { + return secret; + } else { + if (secret.length > secretLen) { + // Shouldn't happen; but check just in case + throw new ProviderException("generated secret is out-of-range"); + } + byte[] newSecret = new byte[secretLen]; + System.arraycopy(secret, 0, newSecret, secretLen - secret.length, + secret.length); + return newSecret; + } } catch (PKCS11Exception e) { throw new ProviderException("Could not derive key", e); } finally { diff -r d935c2f4aeae -r d77ed23f4992 jdk/test/com/sun/crypto/provider/KeyAgreement/DHKeyAgreement2.java --- a/jdk/test/com/sun/crypto/provider/KeyAgreement/DHKeyAgreement2.java Tue Mar 20 12:48:48 2012 +0100 +++ b/jdk/test/com/sun/crypto/provider/KeyAgreement/DHKeyAgreement2.java Tue Mar 20 15:06:13 2012 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2007, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -23,7 +23,7 @@ /* * @test - * @bug 0000000 + * @bug 7146728 * @summary DHKeyAgreement2 * @author Jan Luehe */ @@ -52,15 +52,12 @@ public class DHKeyAgreement2 { + private static final String SUNJCE = "SunJCE"; private DHKeyAgreement2() {} public static void main(String argv[]) throws Exception { String mode = "USE_SKIP_DH_PARAMS"; - // Add JCE to the list of providers - SunJCE jce = new SunJCE(); - Security.addProvider(jce); - DHKeyAgreement2 keyAgree = new DHKeyAgreement2(); if (argv.length > 1) { @@ -86,7 +83,7 @@ // Some central authority creates new DH parameters System.err.println("Creating Diffie-Hellman parameters ..."); AlgorithmParameterGenerator paramGen - = AlgorithmParameterGenerator.getInstance("DH"); + = AlgorithmParameterGenerator.getInstance("DH", SUNJCE); paramGen.init(512); AlgorithmParameters params = paramGen.generateParameters(); dhSkipParamSpec = (DHParameterSpec)params.getParameterSpec @@ -103,7 +100,7 @@ * above */ System.err.println("ALICE: Generate DH keypair ..."); - KeyPairGenerator aliceKpairGen = KeyPairGenerator.getInstance("DH"); + KeyPairGenerator aliceKpairGen = KeyPairGenerator.getInstance("DH", SUNJCE); aliceKpairGen.initialize(dhSkipParamSpec); KeyPair aliceKpair = aliceKpairGen.generateKeyPair(); System.out.println("Alice DH public key:\n" + @@ -112,14 +109,14 @@ aliceKpair.getPrivate().toString()); DHParameterSpec dhParamSpec = ((DHPublicKey)aliceKpair.getPublic()).getParams(); - AlgorithmParameters algParams = AlgorithmParameters.getInstance("DH"); + AlgorithmParameters algParams = AlgorithmParameters.getInstance("DH", SUNJCE); algParams.init(dhParamSpec); System.out.println("Alice DH parameters:\n" + algParams.toString()); // Alice executes Phase1 of her version of the DH protocol System.err.println("ALICE: Execute PHASE1 ..."); - KeyAgreement aliceKeyAgree = KeyAgreement.getInstance("DH"); + KeyAgreement aliceKeyAgree = KeyAgreement.getInstance("DH", SUNJCE); aliceKeyAgree.init(aliceKpair.getPrivate()); // Alice encodes her public key, and sends it over to Bob. @@ -130,7 +127,7 @@ * in encoded format. * He instantiates a DH public key from the encoded key material. */ - KeyFactory bobKeyFac = KeyFactory.getInstance("DH"); + KeyFactory bobKeyFac = KeyFactory.getInstance("DH", SUNJCE); X509EncodedKeySpec x509KeySpec = new X509EncodedKeySpec (alicePubKeyEnc); PublicKey alicePubKey = bobKeyFac.generatePublic(x509KeySpec); @@ -144,7 +141,7 @@ // Bob creates his own DH key pair System.err.println("BOB: Generate DH keypair ..."); - KeyPairGenerator bobKpairGen = KeyPairGenerator.getInstance("DH"); + KeyPairGenerator bobKpairGen = KeyPairGenerator.getInstance("DH", SUNJCE); bobKpairGen.initialize(dhParamSpec); KeyPair bobKpair = bobKpairGen.generateKeyPair(); System.out.println("Bob DH public key:\n" + @@ -154,7 +151,7 @@ // Bob executes Phase1 of his version of the DH protocol System.err.println("BOB: Execute PHASE1 ..."); - KeyAgreement bobKeyAgree = KeyAgreement.getInstance("DH"); + KeyAgreement bobKeyAgree = KeyAgreement.getInstance("DH", SUNJCE); bobKeyAgree.init(bobKpair.getPrivate()); // Bob encodes his public key, and sends it over to Alice. @@ -166,7 +163,7 @@ * Before she can do so, she has to instanticate a DH public key * from Bob's encoded key material. */ - KeyFactory aliceKeyFac = KeyFactory.getInstance("DH"); + KeyFactory aliceKeyFac = KeyFactory.getInstance("DH", SUNJCE); x509KeySpec = new X509EncodedKeySpec(bobPubKeyEnc); PublicKey bobPubKey = aliceKeyFac.generatePublic(x509KeySpec); System.err.println("ALICE: Execute PHASE2 ..."); @@ -187,49 +184,32 @@ byte[] aliceSharedSecret = aliceKeyAgree.generateSecret(); int aliceLen = aliceSharedSecret.length; + // check if alice's key agreement has been reset afterwards + try { + aliceKeyAgree.generateSecret(); + throw new Exception("Error: alice's KeyAgreement not reset"); + } catch (IllegalStateException e) { + System.out.println("EXPECTED: " + e.getMessage()); + } + byte[] bobSharedSecret = new byte[aliceLen]; int bobLen; try { // provide output buffer that is too short bobLen = bobKeyAgree.generateSecret(bobSharedSecret, 1); - - /* - * Gatekeeper's note: - * We should not be getting here, but every so often, we - * get a failure, either a "ShortBufferException" or - * "Key agreement has not been completed yet" in the - * generateSecret(bobSharedSecret, 0) below. - * - * This will help to figure out why we're dropping through - * and not failing. - */ - System.out.println("NIGHTLY: Should *NOT* be here!!!\n" + - "aliceLen = " + aliceLen + "\n" + - "Alice's shared secret"); - - try { - HexDumpEncoder hd = new HexDumpEncoder(); - - hd.encodeBuffer( - new ByteArrayInputStream(aliceSharedSecret), System.out); - } catch (IOException e) { } - - System.out.println("bobLen = " + bobLen); - - try { - HexDumpEncoder hd = new HexDumpEncoder(); - - hd.encodeBuffer( - new ByteArrayInputStream(bobSharedSecret), System.out); - } catch (IOException e) { } - - throw new Exception("Shouldn't be succeeding."); } catch (ShortBufferException e) { System.out.println("EXPECTED: " + e.getMessage()); } + // retry w/ output buffer of required size + bobLen = bobKeyAgree.generateSecret(bobSharedSecret, 0); - // provide output buffer of required size - bobLen = bobKeyAgree.generateSecret(bobSharedSecret, 0); + // check if bob's key agreement has been reset afterwards + try { + bobKeyAgree.generateSecret(bobSharedSecret, 0); + throw new Exception("Error: bob's KeyAgreement not reset"); + } catch (IllegalStateException e) { + System.out.println("EXPECTED: " + e.getMessage()); + } System.out.println("Alice secret: " + toHexString(aliceSharedSecret)); System.out.println("Bob secret: " + toHexString(bobSharedSecret)); diff -r d935c2f4aeae -r d77ed23f4992 jdk/test/sun/security/pkcs11/KeyAgreement/TestInterop.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/jdk/test/sun/security/pkcs11/KeyAgreement/TestInterop.java Tue Mar 20 15:06:13 2012 -0700 @@ -0,0 +1,143 @@ +/* + * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/** + * @test + * @bug 7146728 + * @summary Interop test for DH with secret that has a leading 0x00 byte + * @library .. + */ +import java.math.BigInteger; +import java.util.*; + +import java.security.*; + +import javax.crypto.*; +import javax.crypto.spec.*; + +public class TestInterop extends PKCS11Test { + + private final static BigInteger p = new BigInteger + ("171718397966129586011229151993178480901904202533705695869569760169920539" + + "80807543778874708672297590042574075430109846864794139516459381007417046" + + "27996080624930219892858374168155487210358743785481212360509485282294161" + + "39585571568998066586304075565145536350296006867635076744949977849997684" + + "222020336013226588207303"); + + private final static BigInteger g = new BigInteger("2"); + + private final static BigInteger ya = new BigInteger + ("687709211571508809414670982463565909269384277848448625781941269577397703" + + "73675199968849153119146758339814638228795348558483510369322822476757204" + + "22158455966026517829008713407587339322132253724742557954802911059639161" + + "24827916158465757962384625410294483756242900146397201260757102085985457" + + "09397033481077351036224"); + + private final static BigInteger xa = new BigInteger + ("104917367119952955556289227181599819745346393858545449202252025137706135" + + "98100778613457655440586438263591136003106529323555991109623536177695714" + + "66884181531401472902830508361532232717792847436112280721439936797741371" + + "245140912614191507"); + + private final static BigInteger yb = new BigInteger + ("163887874871842952463100699681506173424091615364591742415764095471629919" + + "08421025296419917755446931473037086355546823601999684501737493240373415" + + "65608293667837249198973539289354492348897732633852665609611113031379864" + + "58514616034107537409230452318065341748503347627733368519091332060477528" + + "173423377887175351037810"); + + private final static BigInteger xb = new BigInteger + ("127757517533485947079959908591028646859165238853082197617179368337276371" + + "51601819447716934542027725311863797141734616730248519214531856941516613" + + "30313414180008978013330410484011186019824874948204261839391153650949864" + + "429505597086564709"); + + public void main(Provider prov) throws Exception { + if (prov.getService("KeyAgreement", "DH") == null) { + System.out.println("DH not supported, skipping"); + return; + } + try { + System.out.println("testing generateSecret()"); + + DHPublicKeySpec publicSpec; + DHPrivateKeySpec privateSpec; + KeyFactory kf = KeyFactory.getInstance("DH"); + KeyAgreement ka = KeyAgreement.getInstance("DH", prov); + KeyAgreement kbSunJCE = KeyAgreement.getInstance("DH", "SunJCE"); + DHPrivateKeySpec privSpecA = new DHPrivateKeySpec(xa, p, g); + DHPublicKeySpec pubSpecA = new DHPublicKeySpec(ya, p, g); + PrivateKey privA = kf.generatePrivate(privSpecA); + PublicKey pubA = kf.generatePublic(pubSpecA); + + DHPrivateKeySpec privSpecB = new DHPrivateKeySpec(xb, p, g); + DHPublicKeySpec pubSpecB = new DHPublicKeySpec(yb, p, g); + PrivateKey privB = kf.generatePrivate(privSpecB); + PublicKey pubB = kf.generatePublic(pubSpecB); + + ka.init(privA); + ka.doPhase(pubB, true); + byte[] n1 = ka.generateSecret(); + + kbSunJCE.init(privB); + kbSunJCE.doPhase(pubA, true); + byte[] n2 = kbSunJCE.generateSecret(); + + if (Arrays.equals(n1, n2) == false) { + throw new Exception("values mismatch!"); + } else { + System.out.println("values: same"); + } + + System.out.println("testing generateSecret(byte[], int)"); + byte[] n3 = new byte[n1.length]; + ka.init(privB); + ka.doPhase(pubA, true); + int n3Len = ka.generateSecret(n3, 0); + if (n3Len != n3.length) { + throw new Exception("PKCS11 Length mismatch!"); + } else System.out.println("PKCS11 Length: ok"); + byte[] n4 = new byte[n2.length]; + kbSunJCE.init(privA); + kbSunJCE.doPhase(pubB, true); + int n4Len = kbSunJCE.generateSecret(n4, 0); + if (n4Len != n4.length) { + throw new Exception("SunJCE Length mismatch!"); + } else System.out.println("SunJCE Length: ok"); + + if (Arrays.equals(n3, n4) == false) { + throw new Exception("values mismatch! "); + } else { + System.out.println("values: same"); + } + } catch (Exception ex) { + System.out.println("Unexpected ex: " + ex); + ex.printStackTrace(); + throw ex; + } + } + + public static void main(String[] args) throws Exception { + main(new TestInterop()); + } +} diff -r d935c2f4aeae -r d77ed23f4992 jdk/test/sun/security/pkcs11/KeyAgreement/TestShort.java --- a/jdk/test/sun/security/pkcs11/KeyAgreement/TestShort.java Tue Mar 20 12:48:48 2012 +0100 +++ b/jdk/test/sun/security/pkcs11/KeyAgreement/TestShort.java Tue Mar 20 15:06:13 2012 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2012, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -23,7 +23,7 @@ /** * @test - * @bug 4942494 + * @bug 4942494 7146728 * @summary KAT test for DH (normal and with secret that has leading a 0x00 byte) * @author Andreas Sterbenz * @library .. @@ -66,7 +66,7 @@ ("433011588852527167500079509018272713204454720683"); private final static byte[] s2 = parse - ("19:c7:f1:bb:2e:3d:93:fa:02:d2:e9:9f:75:32:b9:e6:7a:a0:4a:10:45:81:d4:2b:" + ("00:19:c7:f1:bb:2e:3d:93:fa:02:d2:e9:9f:75:32:b9:e6:7a:a0:4a:10:45:81:d4:2b:" + "e2:77:4c:70:41:39:7c:19:fa:65:64:47:49:8a:ad:0a:fa:9d:e9:62:68:97:c5:52" + ":b1:37:03:d9:cd:aa:e1:bd:7e:71:0c:fc:15:a1:95"); @@ -88,31 +88,36 @@ System.out.println("DH not supported, skipping"); return; } - DHPublicKeySpec publicSpec; - DHPrivateKeySpec privateSpec; - KeyFactory kf = KeyFactory.getInstance("DH", provider); - KeyAgreement ka = KeyAgreement.getInstance("DH", provider); -// KeyAgreement ka = KeyAgreement.getInstance("DH"); + try { + DHPublicKeySpec publicSpec; + DHPrivateKeySpec privateSpec; + KeyFactory kf = KeyFactory.getInstance("DH", provider); + KeyAgreement ka = KeyAgreement.getInstance("DH", provider); - PrivateKey pr1 = kf.generatePrivate(new DHPrivateKeySpec(x1, p, g)); - PublicKey pu2 = kf.generatePublic(new DHPublicKeySpec(y2, p, g)); - PublicKey pu3 = kf.generatePublic(new DHPublicKeySpec(y3, p, g)); + PrivateKey pr1 = kf.generatePrivate(new DHPrivateKeySpec(x1, p, g)); + PublicKey pu2 = kf.generatePublic(new DHPublicKeySpec(y2, p, g)); + PublicKey pu3 = kf.generatePublic(new DHPublicKeySpec(y3, p, g)); - ka.init(pr1); - ka.doPhase(pu2, true); - byte[] n2 = ka.generateSecret(); - if (Arrays.equals(s2, n2) == false) { - throw new Exception("mismatch 2"); - } - System.out.println("short ok"); + ka.init(pr1); + ka.doPhase(pu2, true); + byte[] n2 = ka.generateSecret(); + if (Arrays.equals(s2, n2) == false) { + throw new Exception("mismatch 2"); + } + System.out.println("short ok"); - ka.init(pr1); - ka.doPhase(pu3, true); - byte[] n3 = ka.generateSecret(); - if (Arrays.equals(s3, n3) == false) { - throw new Exception("mismatch 3"); + ka.init(pr1); + ka.doPhase(pu3, true); + byte[] n3 = ka.generateSecret(); + if (Arrays.equals(s3, n3) == false) { + throw new Exception("mismatch 3"); + } + System.out.println("normal ok"); + } catch (Exception ex) { + System.out.println("Unexpected Exception: " + ex); + ex.printStackTrace(); + throw ex; } - System.out.println("normal ok"); /* KeyPairGenerator kpg = KeyPairGenerator.getInstance("DH", provider);