[SECURITY-166] Prevent the creation of anonymous, system or unknown users.

These users can still be instantiated, as would happen if there is no security and an anonymous user triggers a build -- the anonymous user would correctly be created and added to the User list. This fix merely prevents the saving of that user, and therefore prevents them from logging in.

There may be some plugins which trigger a build as the SYSTEM user, and that is not prohibited here.

Also prevent full names of 'anonymous', 'system' or 'uknown'. As discussed on SECURITY-166 this may encumber auditing since full names are used in most places in the UI

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

@@ -34,6 +34,7 @@ import hudson.security.AccessControlled;
 import hudson.security.Permission;
 import hudson.security.SecurityRealm;
 import hudson.util.FormApply;
+import hudson.util.FormValidation;
 import hudson.util.RunList;
 import hudson.util.XStream2;
 import jenkins.model.Jenkins;
@@ -46,6 +47,8 @@ import org.acegisecurity.providers.UsernamePasswordAuthenticationToken;
 import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken;
 import org.acegisecurity.userdetails.UserDetails;
 import org.acegisecurity.userdetails.UsernameNotFoundException;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.springframework.dao.DataAccessException;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
@@ -102,7 +105,12 @@ import javax.annotation.Nonnull;
  */
 @ExportedBean
 public class User extends AbstractModelObject implements AccessControlled, DescriptorByNameOwner, Saveable, Comparable<User>, ModelObjectWithContextMenu {
-    
+
+    /**
+     * These usernames should not be used by real users logging into Jenkins. Therefore, we prevent
+     * users with these names from being saved.
+     */
+    public static final String[] ILLEGAL_PERSISTED_USERNAMES = new String[]{"anonymous", "system", "unknown"};
     private transient final String id;
 
     private volatile String fullName;
@@ -500,10 +508,36 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
         return new File(Jenkins.getInstance().getRootDir(), "users");
     }
 
+    /**
+     * Is the ID allowed? Some are prohibited for security reasons. See SECURITY-166.
+     * <p/>
+     * Note that this is only enforced when saving. These users are often created
+     * via the constructor (and even listed on /asynchPeople), but our goal is to
+     * prevent anyone from logging in as these users. Therefore, we prevent
+     * saving a User with one of these ids.
+     *
+     * @return true if the username or fullname is valid
+     */
+    //TODO: Remove Restricted and add @since when merging to stable-rc
+    @Restricted(NoExternalUse.class)
+    public static boolean isIdOrFullnameAllowed(String id) {
+        for (String invalidId : ILLEGAL_PERSISTED_USERNAMES) {
+            if (id.equalsIgnoreCase(invalidId))
+                return false;
+        }
+        return true;
+    }
+
     /**
      * Save the settings to a file.
      */
-    public synchronized void save() throws IOException {
+    public synchronized void save() throws IOException, FormValidation {
+        if (! isIdOrFullnameAllowed(id)) {
+            throw FormValidation.error(Messages.User_IllegalUsername(id));
+        }
+        if (! isIdOrFullnameAllowed(fullName)) {
+            throw FormValidation.error(Messages.User_IllegalFullname(fullName));
+        }
         if(BulkChange.contains(this))   return;
         getConfigFile().write(this);
         SaveableListener.fireOnChange(this, getConfigFile());

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

@@ -329,6 +329,14 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea
         if(si.email==null || !si.email.contains("@"))
             si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_InvalidEmailAddress();
 
+        if (! User.isIdOrFullnameAllowed(si.username)) {
+            si.errorMessage = hudson.model.Messages.User_IllegalUsername(si.username);
+        }
+
+        if (! User.isIdOrFullnameAllowed(si.fullname)) {
+            si.errorMessage = hudson.model.Messages.User_IllegalFullname(si.fullname);
+        }
+
         if(si.errorMessage!=null) {
             // failed. ask the user to try again.
             req.setAttribute("data",si);

core/src/main/resources/hudson/model/Messages.properties

@@ -357,3 +357,6 @@ Jenkins.CheckDisplayName.NameNotUniqueWarning=The display name, "{0}", is used a
 Jenkins.CheckDisplayName.DisplayNameNotUniqueWarning=The display name, "{0}", is already in use by another job and could cause confusion and delay.
 
 Jenkins.NotAllowedName="{0}" is not allowed name
+
+User.IllegalUsername="{0}" is prohibited as a username for security reasons.
+User.IllegalFullname="{0}" is prohibited as a full name for security reasons.
\ No newline at end of file

test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2Test.java

+package hudson.security;
+
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import hudson.model.User;
+import hudson.security.pages.SignupPage;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+
+import java.util.Collections;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assert.*;
+
+/**
+ * Tests the signup page of {@link hudson.security.HudsonPrivateSecurityRealm}
+ * TODO: Add to {@link hudson.security.HudsonPrivateSecurityRealmTest} going forward
+ */
+public class HudsonPrivateSecurityRealm2Test {
+
+
+    @Rule
+    public JenkinsRule rule = new JenkinsRule();
+
+    @Test
+    public void signup() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
+        rule.jenkins.setSecurityRealm(securityRealm);
+        JenkinsRule.WebClient wc = rule.createWebClient();
+        SignupPage signup = new SignupPage(wc.goTo("signup"));
+        signup.enterUsername("alice");
+        signup.enterPassword("alice");
+        signup.enterFullName("Alice User");
+        signup.enterEmail("alice@nowhere.com");
+        HtmlPage success = signup.submit(rule);
+        assertThat(success.getElementById("main-panel").getTextContent(), containsString("Success"));
+        assertThat(success.getElementById("login-field").getTextContent(), containsString("Alice User"));
+
+        assertEquals("Alice User", securityRealm.getUser("alice").getDisplayName());
+
+    }
+
+    // @Issue("SECURITY-166")
+    @Test
+    public void anonymousCantSignup() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
+        rule.jenkins.setSecurityRealm(securityRealm);
+        JenkinsRule.WebClient wc = rule.createWebClient();
+        SignupPage signup = new SignupPage(wc.goTo("signup"));
+        signup.enterUsername("anonymous");
+        signup.enterFullName("Bob");
+        signup.enterPassword("nothing");
+        signup.enterEmail("noone@nowhere.com");
+        signup = new SignupPage(signup.submit(rule));
+        signup.assertErrorContains("prohibited as a username");
+        assertNull(User.get("anonymous", false, Collections.emptyMap()));
+    }
+
+    // @Issue("SECURITY-166")
+    @Test
+    public void systemCantSignup() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
+        rule.jenkins.setSecurityRealm(securityRealm);
+        JenkinsRule.WebClient wc = rule.createWebClient();
+        SignupPage signup = new SignupPage(wc.goTo("signup"));
+        signup.enterUsername("system");
+        signup.enterFullName("Bob");
+        signup.enterPassword("nothing");
+        signup.enterEmail("noone@nowhere.com");
+        signup = new SignupPage(signup.submit(rule));
+        signup.assertErrorContains("prohibited as a username");
+        assertNull(User.get("system",false, Collections.emptyMap()));
+    }
+
+    /**
+     * We don't allow prohibited fullnames since this may encumber auditing.
+     */
+    // @Issue("SECURITY-166")
+    @Test
+    public void fullNameOfUnknownCantSignup() throws Exception {
+        HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
+        rule.jenkins.setSecurityRealm(securityRealm);
+        JenkinsRule.WebClient wc = rule.createWebClient();
+        SignupPage signup = new SignupPage(wc.goTo("signup"));
+        signup.enterUsername("unknown2");
+        signup.enterPassword("unknown2");
+        signup.enterFullName("unknown");
+        signup.enterEmail("noone@nowhere.com");
+        signup = new SignupPage(signup.submit(rule));
+        signup.assertErrorContains("prohibited as a full name");
+        assertNull(User.get("unknown2",false, Collections.emptyMap()));
+    }
+}

test/src/test/java/hudson/security/pages/SignupPage.java

+package hudson.security.pages;
+
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import org.jvnet.hudson.test.JenkinsRule;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assert.*;
+
+/**
+ * The the signup page for {@link hudson.security.HudsonPrivateSecurityRealm}
+ */
+public class SignupPage {
+
+    public HtmlForm signupForm;
+    public final HtmlPage signupPage;
+
+    public SignupPage(HtmlPage signupPage) {
+        this.signupPage = signupPage;
+
+        assertNotNull("The sign up page has a username field.", this.signupPage.getElementById("username"));
+        for (HtmlForm signupForm : this.signupPage.getForms()) {
+            if (signupForm.getInputsByName("username").size() == 0)
+                continue;
+            this.signupForm = signupForm;
+        }
+
+    }
+
+
+
+    public void enterUsername(String username) {
+        signupForm.getInputByName("username").setValueAttribute(username);
+    }
+
+    /**
+     * Enters the password in password1 and password2.
+     * You can then call {@link #enterPassword2(String)} if you want them to be different.
+     * @param password
+     */
+    public void enterPassword(String password) {
+        signupForm.getInputByName("password1").setValueAttribute(password);
+        signupForm.getInputByName("password2").setValueAttribute(password);
+    }
+
+    public void enterPassword2(String password2) {
+        signupForm.getInputByName("password2").setValueAttribute(password2);
+    }
+
+    public void enterFullName(String fullName) {
+        signupForm.getInputByName("fullname").setValueAttribute(fullName);
+    }
+
+    public void enterEmail(String email) {
+        signupForm.getInputByName("email").setValueAttribute(email);
+    }
+
+    public HtmlPage submit(JenkinsRule rule) throws Exception {
+        return rule.submit(signupForm);
+    }
+
+    public void assertErrorContains(String msg) {
+        assertThat(signupForm.getElementById("main-panel").getTextContent(),containsString(msg));
+    }
+}