diff --git a/opennms-config/src/main/java/org/opennms/netmgt/config/UserManager.java b/opennms-config/src/main/java/org/opennms/netmgt/config/UserManager.java index f10649f716bc..05fadc9ded7d 100644 --- a/opennms-config/src/main/java/org/opennms/netmgt/config/UserManager.java +++ b/opennms-config/src/main/java/org/opennms/netmgt/config/UserManager.java @@ -995,6 +995,10 @@ public void renameUser(final String oldName, final String newName) throws Except m_users.remove(oldName); throw new Exception("UserFactory:rename the data contained for old user " + oldName + " is null"); } else { + if (m_users.containsKey(newName)) { + throw new Exception("UserFactory: cannot rename user " + oldName + ". An user with the given name " + newName + " already exists"); + } + // Rename the user in the user map. m_users.remove(oldName); data.setUserId(newName); diff --git a/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml b/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml index fb52db0a7992..af3f1cce4b84 100644 --- a/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml +++ b/opennms-webapp/src/main/webapp/WEB-INF/applicationContext-spring-security.xml @@ -122,6 +122,18 @@ + + + + + + + + + + + + @@ -158,7 +170,7 @@ - + diff --git a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/list.jsp b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/list.jsp index f82e15920e30..05082802a81e 100644 --- a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/list.jsp +++ b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/list.jsp @@ -102,6 +102,7 @@ +
diff --git a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/modifyGroup.jsp b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/modifyGroup.jsp index c70e61e3487f..f7d6eadda0bd 100644 --- a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/modifyGroup.jsp +++ b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/modifyGroup.jsp @@ -292,6 +292,7 @@ +
diff --git a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/newGroup.jsp b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/newGroup.jsp index 7f6d06db02ca..90e078543d09 100644 --- a/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/newGroup.jsp +++ b/opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/newGroup.jsp @@ -76,6 +76,7 @@
+
diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editDetails.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editDetails.jsp index 7aa44abc87b7..ce6047acb544 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editDetails.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editDetails.jsp @@ -70,6 +70,7 @@
+ diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editSpecific.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editSpecific.jsp index c086a2d1858e..185b17a8ac4b 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editSpecific.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/editSpecific.jsp @@ -59,7 +59,8 @@ - + +
diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/list.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/list.jsp index ef1112ffaaed..9420a7aa8c30 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/list.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/list.jsp @@ -93,6 +93,7 @@ +
@@ -148,6 +149,7 @@
+ diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/view.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/view.jsp index e68648cac2ec..d1554b4c6499 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/roles/view.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/roles/view.jsp @@ -157,10 +157,12 @@
+
+ @@ -220,6 +222,7 @@
+ diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/users/list.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/users/list.jsp index 1250fa5c1839..682adb4b46ba 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/users/list.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/users/list.jsp @@ -95,9 +95,19 @@ { document.allUsers.userID.value=userID; var newID = prompt("Enter new name for user.", userID); - - if (newID != null && newID != "") - { + + if (newID != null && newID != "") { + if (/.*[&<>"`']+.*/.test(newID)) { + alert("The user ID must not contain any HTML markup."); + return; + } + + var element = document.getElementById('users(' + _.escape(newID) + ').doModify'); + if (typeof(element) != 'undefined' && element != null) { + alert("A user with this ID already exist."); + return; + } + document.allUsers.newID.value = newID; document.allUsers.action="admin/userGroupView/users/renameUser"; document.allUsers.submit(); @@ -112,6 +122,7 @@ +

Click on the User ID link to view detailed information about a @@ -209,4 +220,8 @@

+ + + + diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/users/modifyUser.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/users/modifyUser.jsp index 2c02c352a6a4..b6f97ef6cf00 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/users/modifyUser.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/users/modifyUser.jsp @@ -256,6 +256,7 @@ +
diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/users/newPassword.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/users/newPassword.jsp index 877489f2176c..e12fabf20dff 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/users/newPassword.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/users/newPassword.jsp @@ -66,6 +66,8 @@
+ +
diff --git a/opennms-webapp/src/main/webapp/admin/userGroupView/users/newUser.jsp b/opennms-webapp/src/main/webapp/admin/userGroupView/users/newUser.jsp index b07ef034cc7e..7b862f85bef2 100644 --- a/opennms-webapp/src/main/webapp/admin/userGroupView/users/newUser.jsp +++ b/opennms-webapp/src/main/webapp/admin/userGroupView/users/newUser.jsp @@ -82,6 +82,7 @@
+
diff --git a/smoke-test/src/test/java/org/opennms/smoketest/UserIT.java b/smoke-test/src/test/java/org/opennms/smoketest/UserIT.java index 688e015883fa..8c9beaee4943 100644 --- a/smoke-test/src/test/java/org/opennms/smoketest/UserIT.java +++ b/smoke-test/src/test/java/org/opennms/smoketest/UserIT.java @@ -31,6 +31,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import java.time.Duration; +import java.time.temporal.ChronoUnit; import java.util.List; import org.junit.Before; @@ -158,4 +160,168 @@ public void testChangeAdminPassword() throws Exception { assertTrue(wait.until(pageContainsText("Password successfully changed"))); } + @Test + public void testInvalidUserIds() { + testInvalidUserId("JohnDoe",true); + testInvalidUserId("Jane'Doe'",true); + testInvalidUserId("John&Doe",true); + testInvalidUserId("Jane\"\"Doe",true); + } + + @Test + public void testValidUserIds() { + testInvalidUserId("John-Doe",false); + testInvalidUserId("Jane/Doe",false); + testInvalidUserId("John.Doe",false); + testInvalidUserId("Jane#Doe", false); + testInvalidUserId("John@Döe.com", false); + testInvalidUserId("JohnDoé", false); + } + + @Test + public void testInvalidGroupIds() { + testInvalidGroupId("JohnDoe",true); + testInvalidGroupId("Jane'Doe'",true); + testInvalidGroupId("John&Doe",true); + testInvalidGroupId("Jane\"\"Doe",true); + } + + @Test + public void testValidGroupIds() { + testInvalidGroupId("John-Doe",false); + testInvalidGroupId("Jane/Doe",false); + testInvalidGroupId("John.Doe",false); + testInvalidGroupId("Jane#Doe", false); + testInvalidGroupId("John@Döe.com", false); + testInvalidGroupId("JohnDoé", false); + } + + public void testInvalidUserId(final String userId, final boolean mustFail) { + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Users").click(); + findElementByLink("Add new user").click(); + + enterText(By.id("userID"), userId); + enterText(By.id("pass1"), "SmokeTestPassword"); + enterText(By.id("pass2"), "SmokeTestPassword"); + findElementByXpath("//button[@type='submit' and text()='OK']").click(); + + if (mustFail) { + try { + final Alert alert = wait.withTimeout(Duration.of(5, ChronoUnit.SECONDS)).until(ExpectedConditions.alertIsPresent()); + alert.dismiss(); + } catch (final Exception e) { + LOG.debug("Got an exception waiting for a 'invalid user ID' alert.", e); + throw e; + } + } else { + wait.until(ExpectedConditions.elementToBeClickable(By.name("finish"))); + } + } + + public void testInvalidGroupId(final String groupId, final boolean mustFail) { + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Groups").click(); + findElementByLink("Add new group").click(); + + enterText(By.id("groupName"), groupId); + enterText(By.id("groupComment"), "SmokeTestComment"); + findElementByXpath("//button[@type='submit' and text()='OK']").click(); + + if (mustFail) { + try { + final Alert alert = wait.withTimeout(Duration.of(5, ChronoUnit.SECONDS)).until(ExpectedConditions.alertIsPresent()); + alert.dismiss(); + } catch (final Exception e) { + LOG.debug("Got an exception waiting for a 'invalid group ID' alert.", e); + throw e; + } + } else { + wait.until(ExpectedConditions.elementToBeClickable(By.name("finish"))); + } + } + + /** + * see NMS-13124 + */ + @Test + public void testCsrfPrivilegeEscalation() { + // visit the admin's user page + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Users").click(); + findElementByLink("Add new user").click(); + + // add a new user 'user' + enterText(By.id("userID"), "user"); + enterText(By.id("pass1"), "pass"); + enterText(By.id("pass2"), "pass"); + findElementByXpath("//button[@type='submit' and text()='OK']").click(); + + // assign just the ROLE_USER + final Select select = new Select(driver.findElement(By.name("availableRoles"))); + select.selectByValue("ROLE_USER"); + findElementById("roles.doAdd").click(); + findElementById("saveUserButton").click(); + + // assert that this is correctly set + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Users").click(); + findElementById("users(user).doDetails").click(); + + assertTrue(wait.until(pageContainsText("ROLE_USER"))); + + // now construct an exploit to set ROLE_ADMIN for user 'user' + final String html = "" + + "" + + "" + + " " + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + ""; + String script = "var foo = document.createElement('div'); " + + "foo.innerHTML=\"" + html + "\"; " + + "document.body.appendChild(foo)"; + + // ...and execute it + driver.executeScript(script); + findElementById("submitIt").click(); + + // this should be denied due to CSRF protection + assertTrue(wait.until(pageContainsText("Access denied"))); + + // assure that the user's role is still ROLE_USER + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Users").click(); + findElementById("users(user).doDetails").click(); + assertTrue(wait.until(pageContainsText("ROLE_USER"))); + + // delete the user + adminPage(); + findElementByLink("Configure Users, Groups and On-Call Roles").click(); + findElementByLink("Configure Users").click(); + + findElementById("users(user).doDelete").click(); + handleAlert("Are you sure you want to delete the user user?"); + } }