NMS-13124: Fixed user deletion by renaming bug and CSRF privilege esc…

…alation issue
OpenNMS · Mar 12, 2021 · 607151e · 607151e
1 parent bfa48e1
commit 607151e
Show file tree

Hide file tree

Showing 14 changed files with 216 additions and 5 deletions.
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
@@ -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
@@ -122,6 +122,18 @@
   <!-- Allow anonymous access to the webstart portion of the app -->
   <http pattern="/webstart/**" security="none"/>
 
+  <http pattern="/admin/userGroupView/**" use-expressions="true" access-denied-page="/accessDenied.jsp" realm="OpenNMS Realm" auto-config="false" entry-point-ref="loginUrlAuthenticationEntryPoint">
+    <!-- see NMS-13124 -->
+    <csrf />
+    <intercept-url pattern="/admin/userGroupView/**" access="hasAnyRole('ROLE_ADMIN')" />
+    <http-basic entry-point-ref="xRequestedWithAwareBasicAuthEntryPoint" />
+    <logout logout-success-url="/" />
+    <custom-filter position="FORM_LOGIN_FILTER" ref="onmsUsernamePasswordAuthenticationFilter" />
+    <custom-filter position="PRE_AUTH_FILTER" ref="attributePreAuthFilter"/>
+    <custom-filter after="PRE_AUTH_FILTER" ref="headerPreAuthFilter"/>
+    <custom-filter position="LAST" ref="authFilterEnabler"/>
+  </http>
+
   <!-- Only one <http> section can match the implicit '/**' pattern -->
   <http pattern="/**" use-expressions="true" access-denied-page="/accessDenied.jsp" realm="OpenNMS Realm" auto-config="false" entry-point-ref="loginUrlAuthenticationEntryPoint">
     <intercept-url pattern="/" access="hasAnyRole('ROLE_ANONYMOUS','ROLE_USER','ROLE_DASHBOARD')" />
@@ -158,7 +170,7 @@
     <intercept-url pattern="/graph/graph.png" access="hasAnyRole('ROLE_USER','ROLE_DASHBOARD')" />
     <intercept-url pattern="/dashboard/**" access="hasAnyRole('ROLE_USER','ROLE_DASHBOARD')" />
     <intercept-url pattern="/coreweb/**" access="hasAnyRole('ROLE_USER','ROLE_DASHBOARD')" />
-    
+
     <!-- DO NOT ALLOW ACCESS TO /osgi -->
     <intercept-url pattern="/osgi/**" access="denyAll" />
 

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
@@ -102,6 +102,7 @@
   <input type="hidden" name="operation"/>
   <input type="hidden" name="groupName"/>
   <input type="hidden" name="newName"/>
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
   <div class="card">
     <table class="table table-sm table-bordered">

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
@@ -292,6 +292,7 @@
 <form role="form" class="form" method="post" id="modifyGroup" name="modifyGroup">
   <input type="hidden" name="groupName" value="<%=group.getName()%>"/>
   <input type="hidden" name="operation"/>
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
     <div class="card">
         <div class="card-header">

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
@@ -76,6 +76,7 @@
   <div class="card-body">
     <form role="form" class="form" id="newGroupForm" method="post" name="newGroupForm" onsubmit="return validateFormInput();">
       <input type="hidden" name="operation" />
+      <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
       <div class="form-group">
         <label for="groupName" class="">Group Name</label>

diff --git 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 @@
   </div>
   <div class="card-body">
     <form role="form" class="form" action="<c:url value='${reqUrl}'/>" method="post" name="editForm">
+      <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
       <input type="hidden" name="operation" value="saveDetails"/>
       <input type="hidden" name="role" value="${fn:escapeXml(role.name)}"/>
 

diff --git 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 @@
       <input type="hidden" name="operation" value="saveEntry"/>
       <input type="hidden" name="role" value="${fn:escapeXml(role.name)}"/>
       <input type="hidden" name="schedIndex" value="${schedIndex}"/>
-      <input type="hidden" name="timeIndex" value="${timeIndex}" /> 
+      <input type="hidden" name="timeIndex" value="${timeIndex}" />
+      <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
       <div class="form-group form-row">
         <label class="col-sm-2">On-Call Role</label>

diff --git 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 @@
 <form action="<c:url value='${reqUrl}'/>" method="post" name="roleForm">
 	<input type="hidden" name="operation" />
 	<input type="hidden" name="role" />
+	<input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 </form>
 
 <div class="card">
@@ -148,6 +149,7 @@
 </div> <!-- panel -->
 
 <form action="<c:url value='${reqUrl}'/>" method="post" name="newForm">
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
   <input name="operation" type="hidden" value="new"/>
   <button type="submit" class="btn btn-secondary">Add New On-Call Role</button>
 </form>

diff --git 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 @@
 <form action="<c:url value='${reqUrl}'/>" method="post" name="editForm">
   <input type="hidden" name="operation" value="editDetails"/>
   <input type="hidden" name="role" value="${fn:escapeXml(role.name)}"/>
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
   <button type="submit" class="btn btn-secondary">Value Details</button>
 </form>
 
 <form action="<c:url value='${reqUrl}'/>" method="post" name="doneForm" class="my-4">
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
   <button type="submit" class="btn btn-secondary">Done</button>
 </form>
 
@@ -220,6 +222,7 @@
 </div>
 
 <form action="<c:url value='${reqUrl}'/>" method="post" name="doneForm" class="mb-4">
+  <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
   <button type="submit" class="btn btn-secondary">Done</button>
 </form>
 

diff --git 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 @@
 <input type="hidden" name="userID"/>
 <input type="hidden" name="newID"/>
 <input type="hidden" name="password"/>
+<input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
 <p>
   Click on the <i>User ID</i> link to view detailed information about a
@@ -209,4 +220,8 @@
   </div> <!-- panel -->
 </form>
 
+<jsp:include page="/assets/load-assets.jsp" flush="false">
+    <jsp:param name="asset" value="underscore-js" />
+</jsp:include>
+
 <jsp:include page="/includes/bootstrap-footer.jsp" flush="false" />
diff --git 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 @@
 <input id="userID" type="hidden" name="userID" value="<%=user.getUserId()%>"/>
 <input id="password" type="hidden" name="password"/>
 <input id="redirect" type="hidden" name="redirect"/>
+<input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
 
 <div class="row">
   <div class="col-md-6">

diff --git 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 @@
   </div>
   <div class="card-body">
     <form role="form" class="form" method="post" name="goForm">
+      <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
+
       <div class="form-group">
         <label for="pass1" class="">Password</label>
         <input type="password" class="form-control" id="pass1" name="pass1">

diff --git 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 @@
   </div>
   <div class="card-body">
     <form class="form" role="form" id="newUserForm" method="post" name="newUserForm" onsubmit="return validateFormInput();">
+      <input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}"/>
       <div class="form-group">
         <label for="userID" class="">User ID</label>
         <input id="userID" type="text" name="userID" class="form-control">

diff --git 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("John<b>Doe</b>",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("John<b>Doe</b>",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 = "<form action='" + stack.opennms().getBaseUrlInternal() + "opennms/admin/userGroupView/users/updateUser' method='POST' enctype='application/x-www-form-urlencoded'>" +
+                "<input type='hidden' name='userID' value='user' />" +
+                "<input type='hidden' name='password' value=' ' />" +
+                "<input type='hidden' name='redirect' value='/admin/userGroupView/users/saveUser' /> <input type='hidden' name='fullName' value=' ' />" +
+                "<input type='hidden' name='userComments' value=' ' />" +
+                "<input type='hidden' name='configuredRoles' value='ROLE_ADMIN' />" +
+                "<input type='hidden' name='email' value=' ' />" +
+                "<input type='hidden' name='pemail' value=' ' />" +
+                "<input type='hidden' name='xmppAddress' value=' ' />" +
+                "<input type='hidden' name='microblog' value=' ' />" +
+                "<input type='hidden' name='numericalService' value=' ' />" +
+                "<input type='hidden' name='numericalPin' value=' ' />" +
+                "<input type='hidden' name='textService' value=' ' />" +
+                "<input type='hidden' name='textPin' value=' ' />" +
+                "<input type='hidden' name='workPhone' value=' ' />" +
+                "<input type='hidden' name='mobilePhone' value=' ' />" +
+                "<input type='hidden' name='homePhone' value=' ' />" +
+                "<input type='hidden' name='tuiPin' value=' ' />" +
+                "<input type='hidden' name='timeZoneId' value=' ' />" +
+                "<input type='hidden' name='dutySchedules' value='0' />" +
+                "<input type='hidden' name='numSchedules' value='1' />" +
+                "<input type='submit' id='submitIt' />" +
+                "</form>";
+        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?");
+    }
 }