7004967: SecureRandom should be more explicit about threading
8169312: SecureRandom::getSeed(num) not specified if num is negative
Reviewed-by: mullan, xuelei
--- a/jdk/src/java.base/share/classes/java/security/SecureRandom.java Thu Nov 17 11:55:59 2016 +0000
+++ b/jdk/src/java.base/share/classes/java/security/SecureRandom.java Thu Nov 17 21:02:39 2016 +0800
@@ -120,6 +120,24 @@
* gathered, for example, if the entropy source is /dev/random on various
* Unix-like operating systems.
*
+ * <h2> Thread safety </h2>
+ * {@code SecureRandom} objects are safe for use by multiple concurrent threads.
+ *
+ * @implSpec
+ * A {@code SecureRandom} service provider can advertise that it is thread-safe
+ * by setting the <a href=
+ * "{@docRoot}/../technotes/guides/security/StandardNames.html#Service">service
+ * provider attribute</a> "ThreadSafe" to "true" when registering the provider.
+ * Otherwise, this class will instead synchronize access to the following
+ * methods of the {@code SecureRandomSpi} implementation:
+ * <ul>
+ * <li>{@link SecureRandomSpi#engineSetSeed(byte[])}
+ * <li>{@link SecureRandomSpi#engineNextBytes(byte[])}
+ * <li>{@link SecureRandomSpi#engineNextBytes(byte[], SecureRandomParameters)}
+ * <li>{@link SecureRandomSpi#engineGenerateSeed(int)}
+ * <li>{@link SecureRandomSpi#engineReseed(SecureRandomParameters)}
+ * </ul>
+ *
* @see java.security.SecureRandomSpi
* @see java.util.Random
*
@@ -150,6 +168,14 @@
*/
private SecureRandomSpi secureRandomSpi = null;
+ /**
+ * Thread safety.
+ *
+ * @serial
+ * @since 9
+ */
+ private final boolean threadSafe;
+
/*
* The algorithm name of null if unknown.
*
@@ -189,6 +215,16 @@
*/
super(0);
getDefaultPRNG(false, null);
+ this.threadSafe = getThreadSafe();
+ }
+
+ private boolean getThreadSafe() {
+ if (provider == null || algorithm == null) {
+ return false;
+ } else {
+ return Boolean.parseBoolean(provider.getProperty(
+ "SecureRandom." + algorithm + " ThreadSafe", "false"));
+ }
}
/**
@@ -217,6 +253,7 @@
public SecureRandom(byte[] seed) {
super(0);
getDefaultPRNG(true, seed);
+ this.threadSafe = getThreadSafe();
}
private void getDefaultPRNG(boolean setSeed, byte[] seed) {
@@ -269,6 +306,7 @@
this.secureRandomSpi = secureRandomSpi;
this.provider = provider;
this.algorithm = algorithm;
+ this.threadSafe = getThreadSafe();
if (!skipDebug && pdebug != null) {
pdebug.println("SecureRandom." + algorithm +
@@ -653,8 +691,14 @@
*
* @see #getSeed
*/
- public synchronized void setSeed(byte[] seed) {
- secureRandomSpi.engineSetSeed(seed);
+ public void setSeed(byte[] seed) {
+ if (threadSafe) {
+ secureRandomSpi.engineSetSeed(seed);
+ } else {
+ synchronized (this) {
+ secureRandomSpi.engineSetSeed(seed);
+ }
+ }
}
/**
@@ -679,7 +723,7 @@
* yet been initialized at that point.
*/
if (seed != 0) {
- this.secureRandomSpi.engineSetSeed(longToByteArray(seed));
+ setSeed(longToByteArray(seed));
}
}
@@ -690,7 +734,13 @@
*/
@Override
public void nextBytes(byte[] bytes) {
- secureRandomSpi.engineNextBytes(bytes);
+ if (threadSafe) {
+ secureRandomSpi.engineNextBytes(bytes);
+ } else {
+ synchronized (this) {
+ secureRandomSpi.engineNextBytes(bytes);
+ }
+ }
}
/**
@@ -707,12 +757,19 @@
*
* @since 9
*/
- public synchronized void nextBytes(
- byte[] bytes, SecureRandomParameters params) {
+ public void nextBytes(byte[] bytes, SecureRandomParameters params) {
if (params == null) {
throw new IllegalArgumentException("params cannot be null");
}
- secureRandomSpi.engineNextBytes(Objects.requireNonNull(bytes), params);
+ if (threadSafe) {
+ secureRandomSpi.engineNextBytes(
+ Objects.requireNonNull(bytes), params);
+ } else {
+ synchronized (this) {
+ secureRandomSpi.engineNextBytes(
+ Objects.requireNonNull(bytes), params);
+ }
+ }
}
/**
@@ -756,6 +813,7 @@
*
* @param numBytes the number of seed bytes to generate.
*
+ * @throws IllegalArgumentException if {@code numBytes} is negative
* @return the seed bytes.
*
* @see #setSeed
@@ -782,7 +840,13 @@
if (numBytes < 0) {
throw new IllegalArgumentException("numBytes cannot be negative");
}
- return secureRandomSpi.engineGenerateSeed(numBytes);
+ if (threadSafe) {
+ return secureRandomSpi.engineGenerateSeed(numBytes);
+ } else {
+ synchronized (this) {
+ return secureRandomSpi.engineGenerateSeed(numBytes);
+ }
+ }
}
/**
@@ -917,8 +981,14 @@
*
* @since 9
*/
- public synchronized void reseed() {
- secureRandomSpi.engineReseed(null);
+ public void reseed() {
+ if (threadSafe) {
+ secureRandomSpi.engineReseed(null);
+ } else {
+ synchronized (this) {
+ secureRandomSpi.engineReseed(null);
+ }
+ }
}
/**
@@ -937,11 +1007,17 @@
*
* @since 9
*/
- public synchronized void reseed(SecureRandomParameters params) {
+ public void reseed(SecureRandomParameters params) {
if (params == null) {
throw new IllegalArgumentException("params cannot be null");
}
- secureRandomSpi.engineReseed(params);
+ if (threadSafe) {
+ secureRandomSpi.engineReseed(params);
+ } else {
+ synchronized (this) {
+ secureRandomSpi.engineReseed(params);
+ }
+ }
}
// Declare serialVersionUID to be compatible with JDK1.1
--- a/jdk/src/java.base/share/classes/java/security/SecureRandomSpi.java Thu Nov 17 11:55:59 2016 +0000
+++ b/jdk/src/java.base/share/classes/java/security/SecureRandomSpi.java Thu Nov 17 21:02:39 2016 +0800
@@ -58,6 +58,26 @@
* a {@code SecureRandomParameters} argument will never
* return an instance of this implementation. The
* {@link #engineGetParameters()} method must return {@code null}.
+ * <p>
+ * See {@link SecureRandom} for additional details on thread safety. By
+ * default, a {@code SecureRandomSpi} implementation is considered to be
+ * not safe for use by multiple concurrent threads and {@code SecureRandom}
+ * will synchronize access to each of the applicable engine methods
+ * (see {@link SecureRandom} for the list of methods). However, if a
+ * {@code SecureRandomSpi} implementation is thread-safe, the <a href=
+ * "{@docRoot}/../technotes/guides/security/StandardNames.html#Service">
+ * service provider attribute</a> "ThreadSafe" should be set to "true" during
+ * its registration, as follows:
+ * <blockquote><pre>
+ * put("SecureRandom.AlgName ThreadSafe", "true");</pre>
+ * </blockquote>
+ * or
+ * <blockquote><pre>
+ * putService(new Service(this, "SecureRandom", "AlgName", className,
+ * null, Map.of("ThreadSafe", "true")));</pre>
+ * </blockquote>
+ * {@code SecureRandom} will call the applicable engine methods
+ * without any synchronization.
*
* @since 1.2
*/
--- a/jdk/src/java.base/share/classes/sun/security/provider/SunEntries.java Thu Nov 17 11:55:59 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/provider/SunEntries.java Thu Nov 17 21:02:39 2016 +0800
@@ -96,25 +96,32 @@
if (nativeAvailable && useNativePRNG) {
map.put("SecureRandom.NativePRNG",
"sun.security.provider.NativePRNG");
+ map.put("SecureRandom.NativePRNG ThreadSafe", "true");
}
map.put("SecureRandom.DRBG", "sun.security.provider.DRBG");
+ map.put("SecureRandom.DRBG ThreadSafe", "true");
map.put("SecureRandom.SHA1PRNG",
"sun.security.provider.SecureRandom");
+
+ map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
if (nativeAvailable && !useNativePRNG) {
map.put("SecureRandom.NativePRNG",
"sun.security.provider.NativePRNG");
+ map.put("SecureRandom.NativePRNG ThreadSafe", "true");
}
if (NativePRNG.Blocking.isAvailable()) {
map.put("SecureRandom.NativePRNGBlocking",
"sun.security.provider.NativePRNG$Blocking");
+ map.put("SecureRandom.NativePRNGBlocking ThreadSafe", "true");
}
if (NativePRNG.NonBlocking.isAvailable()) {
map.put("SecureRandom.NativePRNGNonBlocking",
"sun.security.provider.NativePRNG$NonBlocking");
+ map.put("SecureRandom.NativePRNGNonBlocking ThreadSafe", "true");
}
/*
@@ -329,6 +336,7 @@
map.put("AlgorithmParameters.DSA ImplementedIn", "Software");
map.put("KeyFactory.DSA ImplementedIn", "Software");
map.put("SecureRandom.SHA1PRNG ImplementedIn", "Software");
+ map.put("SecureRandom.DRBG ImplementedIn", "Software");
map.put("CertificateFactory.X.509 ImplementedIn", "Software");
map.put("KeyStore.JKS ImplementedIn", "Software");
map.put("CertPathValidator.PKIX ImplementedIn", "Software");
--- a/jdk/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/SunMSCAPI.java Thu Nov 17 11:55:59 2016 +0000
+++ b/jdk/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/SunMSCAPI.java Thu Nov 17 21:02:39 2016 +0800
@@ -33,6 +33,8 @@
import java.security.ProviderException;
import java.util.HashMap;
import java.util.Arrays;
+import java.util.Map;
+
import static sun.security.util.SecurityConstants.PROVIDER_VER;
/**
@@ -133,8 +135,11 @@
/*
* Secure random
*/
+ HashMap<String, String> srattrs = new HashMap<>(1);
+ srattrs.put("ThreadSafe", "true");
putService(new ProviderService(p, "SecureRandom",
- "Windows-PRNG", "sun.security.mscapi.PRNG"));
+ "Windows-PRNG", "sun.security.mscapi.PRNG",
+ null, srattrs));
/*
* Key store
--- a/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java Thu Nov 17 11:55:59 2016 +0000
+++ b/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java Thu Nov 17 21:02:39 2016 +0800
@@ -987,7 +987,8 @@
P11Service(Token token, String type, String algorithm,
String className, String[] al, long mechanism) {
- super(token.provider, type, algorithm, className, toList(al), null);
+ super(token.provider, type, algorithm, className, toList(al),
+ type.equals(SR) ? Map.of("ThreadSafe", "true") : null);
this.token = token;
this.mechanism = mechanism & 0xFFFFFFFFL;
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/security/SecureRandom/NoSync.java Thu Nov 17 21:02:39 2016 +0800
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+import java.security.Provider;
+import java.security.SecureRandom;
+import java.security.Security;
+import java.util.Date;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/*
+ * @test
+ * @bug 7004967
+ * @run main/othervm NoSync
+ * @summary SecureRandom should be more explicit about threading
+ */
+public class NoSync {
+ public static void main(String[] args) throws Exception {
+ for (Provider p : Security.getProviders()) {
+ for (Provider.Service s : p.getServices()) {
+ if (s.getType().equals("SecureRandom") &&
+ !s.getAlgorithm().contains("Block")) {
+ test(SecureRandom.getInstance(s.getAlgorithm(), p));
+ }
+ }
+ }
+ Security.setProperty("securerandom.drbg.config", "HMAC_DRBG");
+ test(SecureRandom.getInstance("DRBG"));
+ Security.setProperty("securerandom.drbg.config", "CTR_DRBG");
+ test(SecureRandom.getInstance("DRBG"));
+ }
+
+ static void test(SecureRandom sr) throws Exception {
+ test(sr, 20, 3000);
+ // All out-of-box impl should have the ThreadSafe attribute
+ String attr = sr.getProvider().getProperty("SecureRandom."
+ + sr.getAlgorithm() + " ThreadSafe");
+ if (!"true".equals(attr)) {
+ throw new Exception("Not ThreadSafe: " + attr);
+ }
+ }
+
+ public static void test(SecureRandom sr, int tnum, int rnum)
+ throws Exception {
+
+ System.out.println(sr);
+ System.out.println(sr.getAlgorithm() + " " + sr.getProvider().getName());
+
+ System.out.println(new Date());
+ boolean reseed = sr.getParameters() != null;
+ Thread[] threads = new Thread[tnum];
+ AtomicBoolean failed = new AtomicBoolean(false);
+ Thread.UncaughtExceptionHandler h = (t, e) -> {
+ failed.set(true);
+ e.printStackTrace();
+ };
+ for (int i = 0; i < threads.length; i++) {
+ threads[i] = new Thread() {
+ @Override
+ public void run() {
+ for (int j = 0; j < rnum; j++) {
+ sr.nextBytes(new byte[j%100+100]);
+ sr.setSeed((long)j);
+ if (reseed) {
+ sr.reseed();
+ }
+ }
+ }
+ };
+ threads[i].setUncaughtExceptionHandler(h);
+ threads[i].start();
+ }
+ for (int i = 0; i < threads.length; i++) {
+ threads[i].join();
+ }
+ System.out.println(new Date());
+ System.out.println();
+ if (failed.get()) {
+ throw new RuntimeException("Failed");
+ }
+ }
+}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/security/SecureRandom/ThreadSafe.java Thu Nov 17 21:02:39 2016 +0800
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+import java.security.Provider;
+import java.security.SecureRandom;
+import java.security.SecureRandomSpi;
+import java.util.Map;
+
+/*
+ * @test
+ * @bug 7004967
+ * @summary SecureRandom should be more explicit about threading
+ */
+public class ThreadSafe {
+ public static void main(String[] args) throws Exception {
+ Provider p = new P();
+ NoSync.test(SecureRandom.getInstance("S1", p), 5, 5);
+ try {
+ NoSync.test(SecureRandom.getInstance("S2", p), 5, 5);
+ throw new Exception("Failed");
+ } catch (RuntimeException re) {
+ // Good
+ }
+ NoSync.test(SecureRandom.getInstance("S3", p), 5, 5);
+ try {
+ NoSync.test(SecureRandom.getInstance("S4", p), 5, 5);
+ throw new Exception("Failed");
+ } catch (RuntimeException re) {
+ // Good
+ }
+ }
+
+ public static class P extends Provider {
+ public P() {
+
+ super("P", 1.0d, "Haha");
+
+ // Good. No attribute.
+ put("SecureRandom.S1", S.class.getName());
+
+ // Bad. Boasting ThreadSafe but isn't
+ put("SecureRandom.S2", S.class.getName());
+ put("SecureRandom.S2 ThreadSafe", "true");
+
+ // Good. No attribute.
+ putService(new Service(this, "SecureRandom", "S3",
+ S.class.getName(), null, null));
+
+ // Bad. Boasting ThreadSafe but isn't
+ putService(new Service(this, "SecureRandom", "S4",
+ S.class.getName(), null, Map.of("ThreadSafe", "true")));
+ }
+ }
+
+ // This implementation is not itself thread safe.
+ public static class S extends SecureRandomSpi {
+ @java.lang.Override
+ protected void engineSetSeed(byte[] seed) {
+ return;
+ }
+
+ private volatile boolean inCall = false;
+ @Override
+ protected void engineNextBytes(byte[] bytes) {
+ if (inCall) {
+ throw new RuntimeException("IN CALL");
+ }
+ inCall = true;
+ try {
+ Thread.sleep(100);
+ } catch (Exception e) {
+ // OK
+ }
+ inCall = false;
+ }
+
+ @Override
+ protected byte[] engineGenerateSeed(int numBytes) {
+ return new byte[numBytes];
+ }
+ }
+}