[JENKINS-20108]

Prefer SHA1PRNG, which avoids VM-wide global lock on Unix. Another way to fix this is to avoid repeated calls to "rnd.nextInt()" in TransportConnection.sendMessage() and instead get all the padding in one call. That would require another byte array as there's no SecureRandom.nextBytes(byte[],int,int), but that'd be a monitor cost in the face of VM-wide lock across all the SecureRandom instances. The amount of padding is fairly small, so yet another way to fix this is to buffer random bytes by hitting SecureRandom in a bigger block (thus infrequentl) and save the rest at Connection.
jenkinsci · Oct 12, 2014 · 961adfb · 961adfb
1 parent 93e10b1
commit 961adfb
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 2 deletions.
diff --git a/src/com/trilead/ssh2/Connection.java b/src/com/trilead/ssh2/Connection.java
@@ -22,6 +22,7 @@
 import java.io.InputStream;
 import java.net.InetSocketAddress;
 import java.net.SocketTimeoutException;
+import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.util.Vector;
 
@@ -1105,7 +1106,7 @@ public synchronized boolean isAuthMethodAvailable(String user, String method) th
 	private final SecureRandom getOrCreateSecureRND()
 	{
 		if (generator == null)
-			generator = new SecureRandom();
+            generator = RandomFactory.create();
 
 		return generator;
 	}

diff --git a/src/com/trilead/ssh2/KnownHosts.java b/src/com/trilead/ssh2/KnownHosts.java
@@ -52,6 +52,7 @@ public class KnownHosts
 	public static final int HOSTKEY_IS_OK = 0;
 	public static final int HOSTKEY_IS_NEW = 1;
 	public static final int HOSTKEY_HAS_CHANGED = 2;
+    private static final SecureRandom SECURE_RANDOM = RandomFactory.create();
 
 	private class KnownHostsEntry
 	{
@@ -151,7 +152,7 @@ public static final String createHashedHostname(String hostname)
 
 		byte[] salt = new byte[sha1.getDigestLength()];
 
-		new SecureRandom().nextBytes(salt);
+		SECURE_RANDOM.nextBytes(salt);
 
 		byte[] hash = hmacSha1Hash(salt, hostname);
 

diff --git a/src/com/trilead/ssh2/RandomFactory.java b/src/com/trilead/ssh2/RandomFactory.java
@@ -0,0 +1,31 @@
+package com.trilead.ssh2;
+
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+
+/**
+ * Creates {@link SecureRandom}
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class RandomFactory {
+    static SecureRandom create() {
+        try {
+            // JENKINS-20108
+            // on Unix, "new SecureRandom()" uses NativePRNG that uses a VM-wide lock, which results in
+            // SecureRandom.nextInt() contention when there are lots of concurrent connections.
+            // SHA1PRNG avoids this problem. This PRNG still gets seeded from (blocking) /dev/random,
+            // which assures security.
+            //
+            // note that SHA1PRNG is not a standard. See http://security.stackexchange.com/questions/47871/
+            //
+            // there's also http://coding.tocea.com/scertify-code/dont-use-the-sha1-prng-randomness-generator/
+            // which claims SHA1PRNG has "statistical defects" without details. I discount the credibility of
+            // this claim based on the lack of details, and that this is not reported as a vulnerability upstream.
+            return SecureRandom.getInstance("SHA1PRNG");
+        } catch (NoSuchAlgorithmException e) {
+            // fall back
+            return new SecureRandom();
+        }
+    }
+}