[SECURITY-243] Defend any victim known to the security realm, even if not otherwise loaded.

core/src/main/java/hudson/model/User.java

@@ -1024,27 +1024,30 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
     @Restricted(NoExternalUse.class)
     public static class UserIDCanonicalIdResolver extends User.CanonicalIdResolver {
 
+        private static final ThreadLocal<Boolean> resolving = new ThreadLocal<Boolean>() {
+            @Override
+            protected Boolean initialValue() {
+                return false;
+            }
+        };
+
         @Override
         public String resolveCanonicalId(String idOrFullName, Map<String, ?> context) {
-            // use the same as DefaultUserCanonicalIdResolver
-            String _id = idOrFullName.replace('\\', '_').replace('/', '_').replace('<','_')
-                                            .replace('>', '_');  // 4 replace() still faster than regex
-            Jenkins j = Jenkins.getInstance();
-            if (j == null) {
-                return null;
+            User existing = getById(idOrFullName, false);
+            if (existing != null) {
+                return existing.getId();
             }
-            IdStrategy userIdStrategy = j.getSecurityRealm().getUserIdStrategy();
-            Collection<User> users = User.getAll();
-            for (User u : users) {
-                if (userIdStrategy.equals(u.getId(), _id)) {
-                    return u.getId();
-                }
-            }
-
-            // XXX I have seen files created in AD that are in the form DOMAIN\User so not sure why they get through but lets also try those...
-            for (User u : users) {
-                if (userIdStrategy.equals(u.getId(), idOrFullName)) {
-                    return u.getId();
+            Jenkins j = Jenkins.getInstance();
+            if (j != null) {
+                if (!resolving.get()) {
+                    resolving.set(true);
+                    try {
+                        return j.getSecurityRealm().loadUserByUsername(idOrFullName).getUsername();
+                    } catch (UsernameNotFoundException x) {
+                        // not sure
+                    } finally {
+                        resolving.set(false);
+                    }
                 }
             }
             return null;

test/src/test/java/hudson/model/UserTest.java

@@ -29,16 +29,20 @@ import com.gargoylesoftware.htmlunit.WebAssert;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 
+import hudson.security.AbstractPasswordBasedSecurityRealm;
 import hudson.security.AccessDeniedException2;
 import hudson.security.GlobalMatrixAuthorizationStrategy;
+import hudson.security.GroupDetails;
 import hudson.security.HudsonPrivateSecurityRealm;
 import hudson.security.Permission;
+import hudson.security.UserMayOrMayNotExistException;
 import hudson.tasks.MailAddressResolver;
 import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Locale;
 
 import jenkins.model.IdStrategy;
 import jenkins.model.Jenkins;
@@ -46,14 +50,17 @@ import jenkins.security.ApiTokenProperty;
 
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
+import org.acegisecurity.AuthenticationException;
+import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
+import org.acegisecurity.userdetails.UserDetails;
+import org.acegisecurity.userdetails.UsernameNotFoundException;
 
 import static org.junit.Assert.*;
 import static org.junit.Assume.*;
 import org.junit.Rule;
 import org.junit.Test;
-import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.FakeChangeLogSCM;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -584,6 +591,52 @@ public class UserTest {
         assertEquals("'user2' should resolve to u2", u2.getId(), u.getId());
     }
 
+    @Issue("SECURITY-243")
+    @Test
+    public void resolveByUnloadedIdThenName() throws Exception {
+        j.jenkins.setSecurityRealm(new ExternalSecurityRealm());
+        // do *not* call this here: User.get("victim");
+        User attacker1 = User.get("attacker1");
+        attacker1.setFullName("victim1");
+        User victim1 = User.get("victim1");
+        assertEquals("victim1 is a real user ID, we must ignore the attacker1’s fullName", "victim1", victim1.getId());
+        assertEquals("a recursive call to User.get was OK", null, victim1.getProperty(MyViewsProperty.class).getPrimaryViewName());
+        assertEquals("(though the realm mistakenly added metadata to the attacker)", "victim1", attacker1.getProperty(MyViewsProperty.class).getPrimaryViewName());
+        User.get("attacker2").setFullName("nonexistent");
+        assertEquals("but if we cannot find such a user ID, allow the fullName", "attacker2", User.get("nonexistent").getId());
+        User.get("attacker3").setFullName("unknown");
+        assertEquals("or if we are not sure, allow the fullName", "attacker3", User.get("unknown").getId());
+        User.get("attacker4").setFullName("Victim2");
+        assertEquals("victim2 is a real (canonical) user ID", "victim2", User.get("Victim2").getId());
+
+    }
+    private static class ExternalSecurityRealm extends AbstractPasswordBasedSecurityRealm {
+        @Override
+        public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
+            if (username.equals("nonexistent")) {
+                throw new UsernameNotFoundException(username);
+            } else if (username.equals("unknown")) {
+                throw new UserMayOrMayNotExistException(username);
+            } else {
+                String canonicalName = username.toLowerCase(Locale.ENGLISH);
+                try {
+                    User.get(canonicalName).addProperty(new MyViewsProperty(canonicalName));
+                } catch (IOException x) {
+                    throw new RuntimeException(x);
+                }
+                return new org.acegisecurity.userdetails.User(canonicalName, "", true, true, true, true, new GrantedAuthority[] {AUTHENTICATED_AUTHORITY});
+            }
+        }
+        @Override
+        protected UserDetails authenticate(String username, String password) throws AuthenticationException {
+            return loadUserByUsername(username); // irrelevant
+        }
+        @Override
+        public GroupDetails loadGroupByGroupname(String groupname) throws UsernameNotFoundException {
+            throw new UsernameNotFoundException(groupname); // irrelevant
+        }
+    }
+
     @Test
     public void resolveById() throws Exception {
         User u1 = User.get("user1");