Observable timing discrepancy allows determining username validity in Jenkins

@@ -55,6 +55,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Random;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -205,7 +206,14 @@ public Details load(String username) throws UsernameNotFoundException {
  
     @Override
     protected UserDetails authenticate2(String username, String password) throws AuthenticationException {
-        Details u = load(username);
+        Details u;
+        try {
+            u = load(username);
+        } catch (UsernameNotFoundException ex) {
+            // Waste time to prevent timing attacks distinguishing existing and non-existing user
+            PASSWORD_ENCODER.matches(password, ENCODED_INVALID_USER_PASSWORD);
+            throw ex;
+        }
         if (!u.isPasswordCorrect(password)) {
             throw new BadCredentialsException("Bad credentials");
         }
@@ -963,6 +971,19 @@ public boolean isPasswordHashed(String password) {
  
     public static final MultiPasswordEncoder PASSWORD_ENCODER = new MultiPasswordEncoder();
  
+    /**
+     * This value is used to prevent timing discrepancies when trying to authenticate with an invalid username
+     * compared to just a wrong password. If the user doesn't exist, compare the provided password with this value.
+     */
+    private static final String ENCODED_INVALID_USER_PASSWORD = PASSWORD_ENCODER.encode(generatePassword());
+ 
+    @SuppressFBWarnings(value = {"DMI_RANDOM_USED_ONLY_ONCE", "PREDICTABLE_RANDOM"}, justification = "https://github.com/spotbugs/spotbugs/issues/1539 and doesn't need to be secure, we're just not hardcoding a 'wrong' password")
+    private static String generatePassword() {
+        String password = new Random().ints(20, 33, 127).mapToObj(i -> (char) i)
+                .collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append).toString();
+        return password;
+    }
+ 
     @Extension @Symbol("local")
     public static final class DescriptorImpl extends Descriptor<SecurityRealm> {
         @NonNull

@@ -0,0 +1,89 @@
+package hudson.security;
+ 
+import hudson.ExtensionList;
+import java.lang.reflect.Field;
+import java.net.URL;
+import java.net.URLConnection;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.Base64;
+import jenkins.security.SecurityListener;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+ 
+public class HudsonPrivateSecurityRealmSEC2566Test {
+ 
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    private HudsonPrivateSecurityRealmTest.SpySecurityListenerImpl spySecurityListener;
+ 
+    @Before
+    public void linkExtension() {
+        spySecurityListener = ExtensionList.lookup(SecurityListener.class).get(HudsonPrivateSecurityRealmTest.SpySecurityListenerImpl.class);
+    }
+ 
+    @Before
+    public void setup() throws Exception {
+        Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX");
+        field.setAccessible(true);
+        field.set(null, null);
+    }
+ 
+    @Test
+    @Issue("SECURITY-2566")
+    // @Ignore // TODO: Is this test too fragile to run?
+    public void noTimingDifferenceForInternalSecurityRealm() throws Exception {
+        final HudsonPrivateSecurityRealm realm = new HudsonPrivateSecurityRealm(false, false, null);
+        j.jenkins.setSecurityRealm(realm);
+        realm.createAccount("admin", "admin");
+        final FullControlOnceLoggedInAuthorizationStrategy a = new FullControlOnceLoggedInAuthorizationStrategy();
+        a.setAllowAnonymousRead(false);
+        j.jenkins.setAuthorizationStrategy(a);
+ 
+        final URL url = j.getURL();
+ 
+        long[] correctUserTimings = new long[20];
+        long[] incorrectUserTimings = new long[20];
+ 
+        { // Authenticate with correct user, incorrect password
+            for (int i = 0; i < correctUserTimings.length; i++) {
+                final URLConnection urlConnection = url.openConnection();
+                urlConnection.setRequestProperty("Authorization", "Basic " + Base64.getEncoder().encodeToString("admin:wrong".getBytes(StandardCharsets.UTF_8)));
+                long start = System.nanoTime();
+                try {
+                    urlConnection.getContent(); // send request
+                } catch (Exception ex) {
+                    // don't care
+                }
+                long end = System.nanoTime();
+                correctUserTimings[i] = end - start;
+            }
+        }
+ 
+        { // Authenticate with wrong user
+            for (int i = 0; i < incorrectUserTimings.length; i++) {
+                final URLConnection urlConnection = url.openConnection();
+                urlConnection.setRequestProperty("Authorization", "Basic " + Base64.getEncoder().encodeToString("wrong:wrong".getBytes(StandardCharsets.UTF_8)));
+                long start = System.nanoTime();
+                try {
+                    urlConnection.getContent(); // send request
+                } catch (Exception ex) {
+                    // don't care
+                }
+                long end = System.nanoTime();
+                incorrectUserTimings[i] = end - start;
+            }
+        }
+ 
+        // Compute the averages, ignoring the 2 fastest and slowest times in an attempt to weed out outliers
+        double incorrectAvg = Arrays.stream(incorrectUserTimings).sorted().skip(2).limit(16).average().orElse(0.0);
+        double correctAvg = Arrays.stream(correctUserTimings).sorted().skip(2).limit(16).average().orElse(0.0);
+        // expect roughly the same average times
+        Assert.assertEquals(correctAvg, incorrectAvg, correctAvg * 0.1);
+    }
+}