8246613: Choose the default SecureRandom algo based on registration ordering

Summary: Fixed java.security.Provider and SecureRandom to use the 1st registered SecureRandom service
Reviewed-by: weijun, mullan

src/java.base/share/classes/java/security/Provider.java

@@ -857,10 +857,18 @@ public abstract class Provider extends Properties {
     // serviceMap changed since last call to getServices()
     private volatile transient boolean servicesChanged;
 
+    // Map<String,String> used to keep track of legacy registration
+    private transient Map<String,String> legacyStrings;
+
     // Map<ServiceKey,Service>
     // used for services added via putService(), initialized on demand
     private transient Map<ServiceKey,Service> serviceMap;
 
+    // For backward compatibility, the registration ordering of
+    // SecureRandom (RNG) algorithms needs to be preserved for
+    // "new SecureRandom()" calls when this provider is used
+    private transient Set<Service> prngServices;
+
     // Map<ServiceKey,Service>
     // used for services added via legacy methods, init on demand
     private transient Map<ServiceKey,Service> legacyMap;
@@ -911,12 +919,18 @@ public abstract class Provider extends Properties {
         putAll(copy);
     }
 
-    private static boolean isProviderInfo(Object key) {
+    // check whether to update 'legacyString' with the specified key
+    private boolean checkLegacy(Object key) {
         String keyString = (String)key;
         if (keyString.startsWith("Provider.")) {
-            return true;
+            return false;
+        }
+
+        legacyChanged = true;
+        if (legacyStrings == null) {
+            legacyStrings = new LinkedHashMap<>();
         }
-        return false;
+        return true;
     }
 
     /**
@@ -932,20 +946,20 @@ public abstract class Provider extends Properties {
 
     private Object implRemove(Object key) {
         if (key instanceof String) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.remove((String)key);
         }
         return super.remove(key);
     }
 
     private boolean implRemove(Object key, Object value) {
         if (key instanceof String && value instanceof String) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return false;
             }
-            legacyChanged = true;
+            legacyStrings.remove((String)key, (String)value);
         }
         return super.remove(key, value);
     }
@@ -953,20 +967,21 @@ public abstract class Provider extends Properties {
     private boolean implReplace(Object key, Object oldValue, Object newValue) {
         if ((key instanceof String) && (oldValue instanceof String) &&
                 (newValue instanceof String)) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return false;
             }
-            legacyChanged = true;
+            legacyStrings.replace((String)key, (String)oldValue,
+                    (String)newValue);
         }
         return super.replace(key, oldValue, newValue);
     }
 
     private Object implReplace(Object key, Object value) {
         if ((key instanceof String) && (value instanceof String)) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.replace((String)key, (String)value);
         }
         return super.replace(key, value);
     }
@@ -975,17 +990,26 @@ public abstract class Provider extends Properties {
     private void implReplaceAll(BiFunction<? super Object, ? super Object,
             ? extends Object> function) {
         legacyChanged = true;
+        if (legacyStrings == null) {
+            legacyStrings = new LinkedHashMap<>();
+        } else {
+            legacyStrings.replaceAll((BiFunction<? super String, ? super String,
+                    ? extends String>) function);
+        }
         super.replaceAll(function);
     }
 
     @SuppressWarnings("unchecked") // Function must actually operate over strings
-    private Object implMerge(Object key, Object value, BiFunction<? super Object,
-            ? super Object, ? extends Object> remappingFunction) {
+    private Object implMerge(Object key, Object value,
+            BiFunction<? super Object, ? super Object, ? extends Object>
+            remappingFunction) {
         if ((key instanceof String) && (value instanceof String)) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.merge((String)key, (String)value,
+                    (BiFunction<? super String, ? super String,
+                    ? extends String>) remappingFunction);
         }
         return super.merge(key, value, remappingFunction);
     }
@@ -994,10 +1018,12 @@ public abstract class Provider extends Properties {
     private Object implCompute(Object key, BiFunction<? super Object,
             ? super Object, ? extends Object> remappingFunction) {
         if (key instanceof String) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.compute((String) key,
+                    (BiFunction<? super String,? super String,
+                    ? extends String>) remappingFunction);
         }
         return super.compute(key, remappingFunction);
     }
@@ -1006,10 +1032,12 @@ public abstract class Provider extends Properties {
     private Object implComputeIfAbsent(Object key, Function<? super Object,
             ? extends Object> mappingFunction) {
         if (key instanceof String) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.computeIfAbsent((String) key,
+                    (Function<? super String, ? extends String>)
+                    mappingFunction);
         }
         return super.computeIfAbsent(key, mappingFunction);
     }
@@ -1018,35 +1046,40 @@ public abstract class Provider extends Properties {
     private Object implComputeIfPresent(Object key, BiFunction<? super Object,
             ? super Object, ? extends Object> remappingFunction) {
         if (key instanceof String) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.computeIfPresent((String) key,
+                    (BiFunction<? super String, ? super String,
+                    ? extends String>) remappingFunction);
         }
         return super.computeIfPresent(key, remappingFunction);
     }
 
     private Object implPut(Object key, Object value) {
         if ((key instanceof String) && (value instanceof String)) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.put((String)key, (String)value);
         }
         return super.put(key, value);
     }
 
     private Object implPutIfAbsent(Object key, Object value) {
         if ((key instanceof String) && (value instanceof String)) {
-            if (isProviderInfo(key)) {
+            if (!checkLegacy(key)) {
                 return null;
             }
-            legacyChanged = true;
+            legacyStrings.putIfAbsent((String)key, (String)value);
         }
         return super.putIfAbsent(key, value);
     }
 
     private void implClear() {
+        if (legacyStrings != null) {
+            legacyStrings.clear();
+        }
         if (legacyMap != null) {
             legacyMap.clear();
         }
@@ -1054,6 +1087,7 @@ public abstract class Provider extends Properties {
         legacyChanged = false;
         servicesChanged = false;
         serviceSet = null;
+        prngServices = null;
         super.clear();
         putId();
     }
@@ -1093,7 +1127,7 @@ public abstract class Provider extends Properties {
      * service objects.
      */
     private void ensureLegacyParsed() {
-        if (legacyChanged == false) {
+        if (legacyChanged == false || (legacyStrings == null)) {
             return;
         }
         serviceSet = null;
@@ -1102,7 +1136,7 @@ public abstract class Provider extends Properties {
         } else {
             legacyMap.clear();
         }
-        for (Map.Entry<?,?> entry : super.entrySet()) {
+        for (Map.Entry<String,String> entry : legacyStrings.entrySet()) {
             parseLegacyPut(entry.getKey(), entry.getValue());
         }
         removeInvalidServices(legacyMap);
@@ -1123,12 +1157,12 @@ public abstract class Provider extends Properties {
         }
     }
 
-    private String[] getTypeAndAlgorithm(String key) {
+    private static String[] getTypeAndAlgorithm(String key) {
         int i = key.indexOf('.');
         if (i < 1) {
             if (debug != null) {
-                debug.println("Ignoring invalid entry in provider "
-                        + name + ":" + key);
+                debug.println("Ignoring invalid entry in provider: "
+                        + key);
             }
             return null;
         }
@@ -1141,15 +1175,7 @@ public abstract class Provider extends Properties {
     private static final String ALIAS_PREFIX_LOWER = "alg.alias.";
     private static final int ALIAS_LENGTH = ALIAS_PREFIX.length();
 
-    private void parseLegacyPut(Object k, Object v) {
-        if (!(k instanceof String) || !(v instanceof String)) {
-            return;
-        }
-        String name = (String) k;
-        String value = (String) v;
-        if (isProviderInfo(name)) {
-            return;
-        }
+    private void parseLegacyPut(String name, String value) {
         if (name.toLowerCase(ENGLISH).startsWith(ALIAS_PREFIX_LOWER)) {
             // e.g. put("Alg.Alias.MessageDigest.SHA", "SHA-1");
             // aliasKey ~ MessageDigest.SHA
@@ -1191,6 +1217,10 @@ public abstract class Provider extends Properties {
                     legacyMap.put(key, s);
                 }
                 s.className = className;
+
+                if (type.equals("SecureRandom")) {
+                    updateSecureRandomEntries(true, s);
+                }
             } else { // attribute
                 // e.g. put("MessageDigest.SHA-1 ImplementedIn", "Software");
                 String attributeValue = value;
@@ -1350,9 +1380,46 @@ public abstract class Provider extends Properties {
         servicesChanged = true;
         synchronized (this) {
             putPropertyStrings(s);
+            if (type.equals("SecureRandom")) {
+                updateSecureRandomEntries(true, s);
+            }
         }
     }
 
+    private void updateSecureRandomEntries(boolean doAdd, Service s) {
+        Objects.requireNonNull(s);
+        if (doAdd) {
+            if (prngServices == null) {
+                prngServices = new LinkedHashSet<Service>();
+            }
+            prngServices.add(s);
+        } else {
+            prngServices.remove(s);
+        }
+
+        if (debug != null) {
+            debug.println((doAdd? "Add":"Remove") + " SecureRandom algo " +
+                s.getAlgorithm());
+        }
+    }
+
+    // used by new SecureRandom() to find out the default SecureRandom
+    // service for this provider
+    synchronized Service getDefaultSecureRandomService() {
+        checkInitialized();
+
+        if (legacyChanged) {
+            prngServices = null;
+            ensureLegacyParsed();
+        }
+
+        if (prngServices != null && !prngServices.isEmpty()) {
+            return prngServices.iterator().next();
+        }
+
+        return null;
+    }
+
     /**
      * Put the string properties for this Service in this Provider's
      * Hashtable.
@@ -1446,6 +1513,9 @@ public abstract class Provider extends Properties {
         }
         synchronized (this) {
             removePropertyStrings(s);
+            if (type.equals("SecureRandom")) {
+                updateSecureRandomEntries(false, s);
+            }
         }
     }
 

src/java.base/share/classes/java/security/SecureRandom.java

 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2020, 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
@@ -259,35 +259,51 @@ public class SecureRandom extends java.util.Random {
     }
 
     private void getDefaultPRNG(boolean setSeed, byte[] seed) {
-        String prng = getPrngAlgorithm();
-        if (prng == null) {
-            // bummer, get the SUN implementation
-            prng = "SHA1PRNG";
+        Service prngService = null;
+        String prngAlgorithm = null;
+        for (Provider p : Providers.getProviderList().providers()) {
+            // SUN provider uses the SunEntries.DEF_SECURE_RANDOM_ALGO
+            // as the default SecureRandom algorithm; for other providers,
+            // Provider.getDefaultSecureRandom() will use the 1st
+            // registered SecureRandom algorithm
+            if (p.getName().equals("SUN")) {
+                prngAlgorithm = SunEntries.DEF_SECURE_RANDOM_ALGO;
+                prngService = p.getService("SecureRandom", prngAlgorithm);
+                break;
+            } else {
+                prngService = p.getDefaultSecureRandomService();
+                if (prngService != null) {
+                    prngAlgorithm = prngService.getAlgorithm();
+                    break;
+                }
+            }
+        }
+        // per javadoc, if none of the Providers support a RNG algorithm,
+        // then an implementation-specific default is returned.
+        if (prngService == null) {
+            prngAlgorithm = "SHA1PRNG";
             this.secureRandomSpi = new sun.security.provider.SecureRandom();
             this.provider = Providers.getSunProvider();
-            if (setSeed) {
-                this.secureRandomSpi.engineSetSeed(seed);
-            }
         } else {
             try {
-                SecureRandom random = SecureRandom.getInstance(prng);
-                this.secureRandomSpi = random.getSecureRandomSpi();
-                this.provider = random.getProvider();
-                if (setSeed) {
-                    this.secureRandomSpi.engineSetSeed(seed);
-                }
+                this.secureRandomSpi = (SecureRandomSpi)
+                    prngService.newInstance(null);
+                this.provider = prngService.getProvider();
             } catch (NoSuchAlgorithmException nsae) {
-                // never happens, because we made sure the algorithm exists
+                // should not happen
                 throw new RuntimeException(nsae);
             }
         }
+        if (setSeed) {
+            this.secureRandomSpi.engineSetSeed(seed);
+        }
         // JDK 1.1 based implementations subclass SecureRandom instead of
         // SecureRandomSpi. They will also go through this code path because
         // they must call a SecureRandom constructor as it is their superclass.
         // If we are dealing with such an implementation, do not set the
         // algorithm value as it would be inaccurate.
         if (getClass() == SecureRandom.class) {
-            this.algorithm = prng;
+            this.algorithm = prngAlgorithm;
         }
     }
 
@@ -620,13 +636,6 @@ public class SecureRandom extends java.util.Random {
                 instance.provider, algorithm);
     }
 
-    /**
-     * Returns the {@code SecureRandomSpi} of this {@code SecureRandom} object.
-     */
-    SecureRandomSpi getSecureRandomSpi() {
-        return secureRandomSpi;
-    }
-
     /**
      * Returns the provider of this {@code SecureRandom} object.
      *
@@ -868,30 +877,6 @@ public class SecureRandom extends java.util.Random {
         return retVal;
     }
 
-    /**
-     * Gets a default PRNG algorithm by looking through all registered
-     * providers. Returns the first PRNG algorithm of the first provider that
-     * has registered a {@code SecureRandom} implementation, or null if none of
-     * the registered providers supplies a {@code SecureRandom} implementation.
-     */
-    private static String getPrngAlgorithm() {
-        for (Provider p : Providers.getProviderList().providers()) {
-            // For SUN provider, we use SunEntries.DEFF_SECURE_RANDOM_ALGO
-            // as the default SecureRandom algorithm; for other providers,
-            // we continue to iterate through to the 1st SecureRandom
-            // service
-            if (p.getName().equals("SUN")) {
-                return SunEntries.DEF_SECURE_RANDOM_ALGO;
-            }
-            for (Service s : p.getServices()) {
-                if (s.getType().equals("SecureRandom")) {
-                    return s.getAlgorithm();
-                }
-            }
-        }
-        return null;
-    }
-
     /*
      * Lazily initialize since Pattern.compile() is heavy.
      * Effective Java (2nd Edition), Item 71.

test/jdk/java/security/SecureRandom/DefaultAlgo.java

 /*
- * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2020, 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
@@ -22,33 +22,96 @@
  */
 
 import static java.lang.System.out;
+import java.security.Provider;
+import java.security.Security;
 import java.security.SecureRandom;
+import java.security.Provider.Service;
+import java.util.Objects;
+import java.util.Arrays;
 import sun.security.provider.SunEntries;
 
 /**
  * @test
- * @bug 8228613
- * @summary Ensure that the default SecureRandom algo matches
- *     SunEntries.DEF_SECURE_RANDOM_ALGO when SUN provider is used
+ * @bug 8228613 8246613
+ * @summary Ensure that the default SecureRandom algo used is based
+ *     on the registration ordering, and falls to next provider
+ *     if none are found
  * @modules java.base/sun.security.provider
  */
 public class DefaultAlgo {
 
     public static void main(String[] args) throws Exception {
-        SecureRandom sr = new SecureRandom();
-        String actualAlg = sr.getAlgorithm();
-        out.println("Default SecureRandom algo: " + actualAlg);
-        if (sr.getProvider().getName().equals("SUN")) {
-            // when using Sun provider, compare and check if the algorithm
-            // matches SunEntries.DEF_SECURE_RANDOM_ALGO
-            if (actualAlg.equals(SunEntries.DEF_SECURE_RANDOM_ALGO)) {
-                out.println("Test Passed");
-            } else {
-                throw new RuntimeException("Failed: Expected " +
+        String[] algos = { "A", "B", "C" };
+        test3rdParty(algos);
+        // reverse the order and re-check
+        String[] algosReversed = { "C", "B", "A" };
+        test3rdParty(algosReversed);
+    }
+
+    private static void test3rdParty(String[] algos) {
+        Provider[] provs = {
+            new SampleLegacyProvider(algos),
+            new SampleServiceProvider(algos)
+        };
+        for (Provider p : provs) {
+            checkDefault(p, algos);
+        }
+    }
+
+    // validate the specified SecureRandom obj to be from the specified
+    // provider and matches the specified algorithm
+    private static void validate(SecureRandom sr, String pName, String algo) {
+        if (!sr.getProvider().getName().equals(pName)) {
+            throw new RuntimeException("Failed provider check, exp: " +
+                    pName + ", got " + sr.getProvider().getName());
+        }
+        if (!sr.getAlgorithm().equals(algo)) {
+            throw new RuntimeException("Failed algo check, exp: " +
+                    algo + ", got " + sr.getAlgorithm());
+        }
+    }
+
+    private static void checkDefault(Provider p, String ... algos) {
+        out.println(p.getName() + " with " + Arrays.toString(algos));
+        int pos = Security.insertProviderAt(p, 1);
+        String pName = p.getName();
+        boolean isLegacy = pName.equals("SampleLegacy");
+        try {
+            if (isLegacy) {
+                for (String s : algos) {
+                    validate(new SecureRandom(), pName, s);
+                    p.remove("SecureRandom." + s);
+                    out.println("removed "  + s);
+                }
+                validate(new SecureRandom(), "SUN",
                         SunEntries.DEF_SECURE_RANDOM_ALGO);
+            } else {
+                validate(new SecureRandom(), pName, algos[0]);
+            }
+            out.println("=> Test Passed");
+        } finally {
+            if (pos != -1) {
+                Security.removeProvider(p.getName());
+            }
+        }
+    }
+
+    private static class SampleLegacyProvider extends Provider {
+        SampleLegacyProvider(String[] listOfSupportedRNGs) {
+            super("SampleLegacy", "1.0", "test provider using legacy put");
+            for (String s : listOfSupportedRNGs) {
+                put("SecureRandom." + s, "sun.security.provider.SecureRandom");
+            }
+        }
+    }
+
+    private static class SampleServiceProvider extends Provider {
+        SampleServiceProvider(String[] listOfSupportedRNGs) {
+            super("SampleService", "1.0", "test provider using putService");
+            for (String s : listOfSupportedRNGs) {
+                putService(new Provider.Service(this, "SecureRandom", s,
+                        "sun.security.provider.SecureRandom", null, null));
             }
-        } else {
-            out.println("Skip test for non-Sun provider: " + sr.getProvider());
         }
     }
 }