From 0a11e21d14cb9f5aa1d97e158619db47db4cb971 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 19 Jan 2017 18:26:51 +0100 Subject: [PATCH 1/2] [SECURITY-406] Prevent user creation via GET /user/whatever --- core/src/main/java/hudson/model/User.java | 16 ++++++++++ core/src/main/java/jenkins/model/Jenkins.java | 2 +- .../test/java/jenkins/model/JenkinsTest.java | 30 +++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index b8c70b4555dc..dd3fd4444efb 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -1078,5 +1078,21 @@ public int getPriority() { * JENKINS-22346. */ public static boolean ALLOW_NON_EXISTENT_USER_TO_LOGIN = Boolean.getBoolean(User.class.getName()+".allowNonExistentUserToLogin"); + + + /** + * Jenkins historically created a (usually) ephemeral user record when an user with Overall/Administer permission + * accesses a /user/arbitraryName URL. + *

+ * Unfortunately this constitutes a CSRF vulnerability, as malicious users can make admins create arbitrary numbers + * of ephemeral user records, so the behavior was changed in Jenkins 2.TODO / 2.32.2. + *

+ * As some users may be relying on the previous behavior, setting this to true restores the previous behavior. This + * is not recommended. + * + * SECURITY-406. + */ + @Restricted(NoExternalUse.class) + public static boolean ALLOW_USER_CREATION_VIA_URL = Boolean.getBoolean(User.class.getName() + ".allowUserCreationViaUrl"); } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 5d4532700756..faa2da531481 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -2489,7 +2489,7 @@ private File getRootDirFor(String name) { * @see User#get(String,boolean), {@link User#getById(String, boolean)} */ public @CheckForNull User getUser(String name) { - return User.get(name,hasPermission(ADMINISTER)); + return User.get(name, User.ALLOW_USER_CREATION_VIA_URL && hasPermission(ADMINISTER)); } public synchronized TopLevelItem createProject( TopLevelItemDescriptor type, String name ) throws IOException { diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java index 6664193d57d7..fa5d77869296 100644 --- a/test/src/test/java/jenkins/model/JenkinsTest.java +++ b/test/src/test/java/jenkins/model/JenkinsTest.java @@ -26,6 +26,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertEquals; @@ -81,6 +82,35 @@ public class JenkinsTest { @Rule public JenkinsRule j = new JenkinsRule(); + @Issue("SECURITY-406") + @Test + public void testUserCreationFromUrlForAdmins() throws Exception { + WebClient wc = j.createWebClient(); + + assertNull("User not supposed to exist", User.getById("nonexistent", false)); + + try { + wc.goTo("user/nonexistent"); + fail("expected exception"); + } catch (FailingHttpStatusCodeException ex) { + assertEquals("expect 404", 404, ex.getStatusCode()); + } + + assertNull("User not supposed to exist", User.getById("nonexistent", false)); + + try { + User.ALLOW_USER_CREATION_VIA_URL = true; + + // expected to work + wc.goTo("user/nonexistent2"); + + assertNotNull("User supposed to exist", User.getById("nonexistent2", false)); + + } finally { + User.ALLOW_USER_CREATION_VIA_URL = false; + } + } + @Test public void testIsDisplayNameUniqueTrue() throws Exception { final String curJobName = "curJobName"; From 4036ca2fa00d204caffd58f030a9c1cf3bd2801a Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 19 Jan 2017 18:38:45 +0100 Subject: [PATCH 2/2] [SECURITY-406] Address review comments --- core/src/main/java/hudson/model/User.java | 1 + core/src/main/java/jenkins/model/Jenkins.java | 2 +- test/src/test/java/jenkins/model/JenkinsTest.java | 9 +-------- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index dd3fd4444efb..8f850bc3081d 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -1092,6 +1092,7 @@ public int getPriority() { * * SECURITY-406. */ + // TODO 2.4+ SystemProperties @Restricted(NoExternalUse.class) public static boolean ALLOW_USER_CREATION_VIA_URL = Boolean.getBoolean(User.class.getName() + ".allowUserCreationViaUrl"); } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index faa2da531481..4a433ce90d71 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -2485,7 +2485,7 @@ private File getRootDirFor(String name) { /** * Gets the user of the given name. * - * @return the user of the given name (which may or may not be an id), if that person exists or the invoker {@link #hasPermission} on {@link #ADMINISTER}; else null + * @return the user of the given name (which may or may not be an id), if that person exists; else null * @see User#get(String,boolean), {@link User#getById(String, boolean)} */ public @CheckForNull User getUser(String name) { diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java index fa5d77869296..201ce1585649 100644 --- a/test/src/test/java/jenkins/model/JenkinsTest.java +++ b/test/src/test/java/jenkins/model/JenkinsTest.java @@ -88,14 +88,7 @@ public void testUserCreationFromUrlForAdmins() throws Exception { WebClient wc = j.createWebClient(); assertNull("User not supposed to exist", User.getById("nonexistent", false)); - - try { - wc.goTo("user/nonexistent"); - fail("expected exception"); - } catch (FailingHttpStatusCodeException ex) { - assertEquals("expect 404", 404, ex.getStatusCode()); - } - + wc.assertFails("user/nonexistent", 404); assertNull("User not supposed to exist", User.getById("nonexistent", false)); try {