From de7aaab441151fb1760855fec83681c6a8756a45 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:39:43 +0200 Subject: [PATCH] [SECURITY-786] --- .../security/HudsonPrivateSecurityRealm.java | 24 ++++ .../hudson/security/Messages.properties | 2 + .../HudsonPrivateSecurityRealmTest.java | 129 +++++++++++++++++- 3 files changed, 154 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index b82e8b70d0e3..c787e352f232 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -64,6 +64,7 @@ import org.mindrot.jbcrypt.BCrypt; import org.springframework.dao.DataAccessException; +import javax.annotation.Nonnull; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -95,6 +96,15 @@ * @author Kohsuke Kawaguchi */ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRealm implements ModelObject, AccessControlled { + private static /* not final */ String ID_REGEX = System.getProperty(HudsonPrivateSecurityRealm.class.getName() + ".ID_REGEX"); + + /** + * Default REGEX for the user ID check in case the ID_REGEX is not set + * It allows A-Za-z0-9 + "_-" + * in Java {@code \w} is equivalent to {@code [A-Za-z0-9_]} (take care of "_") + */ + private static final String DEFAULT_ID_REGEX = "^[\\w-]+$"; + /** * If true, sign up is not allowed. *

@@ -338,6 +348,12 @@ private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean self if(si.username==null || si.username.length()==0) si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameRequired(); + else if(!containsOnlyAcceptableCharacters(si.username)) + if(ID_REGEX == null){ + si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharacters(); + }else{ + si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharactersCustom(ID_REGEX); + } else { // do not create the user - we just want to check if the user already exists but is not a "login" user. User user = User.getById(si.username, false); @@ -385,6 +401,14 @@ private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean self return user; } + private boolean containsOnlyAcceptableCharacters(@Nonnull String value){ + if(ID_REGEX == null){ + return value.matches(DEFAULT_ID_REGEX); + }else{ + return value.matches(ID_REGEX); + } + } + @Restricted(NoExternalUse.class) public boolean isMailerPluginPresent() { try { diff --git a/core/src/main/resources/hudson/security/Messages.properties b/core/src/main/resources/hudson/security/Messages.properties index 086cc59afbd2..951a1bfbc5c6 100644 --- a/core/src/main/resources/hudson/security/Messages.properties +++ b/core/src/main/resources/hudson/security/Messages.properties @@ -37,6 +37,8 @@ HudsonPrivateSecurityRealm.CreateAccount.TextNotMatchWordInImage=Text didn't mat HudsonPrivateSecurityRealm.CreateAccount.PasswordNotMatch=Password didn't match HudsonPrivateSecurityRealm.CreateAccount.PasswordRequired=Password is required HudsonPrivateSecurityRealm.CreateAccount.UserNameRequired=User name is required +HudsonPrivateSecurityRealm.CreateAccount.UserNameInvalidCharacters=User name must only contain alphanumeric characters, underscore and dash +HudsonPrivateSecurityRealm.CreateAccount.UserNameInvalidCharactersCustom=User name must match the following expression: {0} HudsonPrivateSecurityRealm.CreateAccount.InvalidEmailAddress=Invalid e-mail address HudsonPrivateSecurityRealm.CreateAccount.UserNameAlreadyTaken=User name is already taken diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 258a47c4748d..db7666f6302f 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -2,15 +2,21 @@ import static hudson.security.HudsonPrivateSecurityRealm.CLASSIC; import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.xml.HasXPath.hasXPath; import java.io.UnsupportedEncodingException; +import java.lang.reflect.Field; +import hudson.security.pages.SignupPage; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -34,6 +40,13 @@ public class HudsonPrivateSecurityRealmTest { @Rule public JenkinsRule j = new JenkinsRule(); + @Before + public void setup() throws Exception { + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); + } + /** * Tests the data compatibility with Hudson before 1.283. * Starting 1.283, passwords are now stored hashed. @@ -170,5 +183,119 @@ private static final String basicHeader(String user, String pass) throws Unsuppo String authHeader = "Basic " + auth; return authHeader; } - + + @Issue("SECURITY-786") + @Test + public void controlCharacterAreNoMoreValid() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + String password = "testPwd"; + String email = "test@test.com"; + int i = 0; + + // regular case = only accepting a-zA-Z0-9 + "-_" + checkUserCanBeCreatedWith(securityRealm, "test" + i, password, "Test" + i, email); + assertNotNull(User.getById("test" + i, false)); + i++; + checkUserCanBeCreatedWith(securityRealm, "te-st_123" + i, password, "Test" + i, email); + assertNotNull(User.getById("te-st_123" + i, false)); + i++; + {// user id that contains invalid characters + checkUserCannotBeCreatedWith(securityRealm, "test " + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "te@st" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test.com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test,com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test,com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "testécom" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "Stargåte" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "te\u0000st" + i, password, "Test" + i, email); + i++; + } + } + + @Issue("SECURITY-786") + @Test + public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + String currentRegex = "^[A-Z]+[0-9]*$"; + + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, currentRegex); + + String password = "testPwd"; + String email = "test@test.com"; + int i = 0; + + // regular case = only accepting a-zA-Z0-9 + "-_" + checkUserCanBeCreatedWith(securityRealm, "TEST" + i, password, "Test" + i, email); + assertNotNull(User.getById("TEST" + i, false)); + i++; + checkUserCanBeCreatedWith(securityRealm, "TEST123" + i, password, "Test" + i, email); + assertNotNull(User.getById("TEST123" + i, false)); + i++; + {// user id that do not follow custom regex + checkUserCannotBeCreatedWith_custom(securityRealm, "test " + i, password, "Test" + i, email, currentRegex); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "@" + i, password, "Test" + i, email, currentRegex); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "T2A" + i, password, "Test" + i, email, currentRegex); + i++; + } + { // we can even change regex on the fly + currentRegex = "^[0-9]*$"; + field.set(null, currentRegex); + + checkUserCanBeCreatedWith(securityRealm, "125213" + i, password, "Test" + i, email); + assertNotNull(User.getById("125213" + i, false)); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "TEST12" + i, password, "Test" + i, email, currentRegex); + i++; + } + } + + private void checkUserCanBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), containsString("Success")); + } + + private void checkUserCannotBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), not(containsString("Success"))); + assertThat(success.getElementById("main-panel").getTextContent(), containsString(Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharacters())); + } + + private void checkUserCannotBeCreatedWith_custom(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email, String regex) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), not(containsString("Success"))); + assertThat(success.getElementById("main-panel").getTextContent(), containsString(regex)); + } }