RHPAM-4719: Persistent Cross-Site Scripting (XSS) (#1393) (#1402)

* RHPAM-4719: Persistent Cross-Site Scripting (XSS) Fixes RHPAM-4716 & RHPAM-4717 by using StringEscapeUtils:: escapeHtml4() method in ProjectResource and by implementing helper method, using escapeHtml4(), to escape conrtributors names in OrganizationalUnitServiceImpl * RHPAM-4719: Add unit test cases for XSS data * RHPAM-4719: Replace single qoute with nothing * RHPAM-4917: Expand escaping to RepositoryService Refactors unit tests to use same methods as in main classes Add some unit tests * Fix code duplication Moves methods for escaping out of services * Increase coverage and remove code smells Co-authored-by: Dominik Hanák <[email protected]>
kiegroup · Aug 11, 2023 · eb4125d · eb4125d
1 parent f90fc31
commit eb4125d
Show file tree

Hide file tree

Showing 10 changed files with 306 additions and 6 deletions.
diff --git a/uberfire-rest/uberfire-rest-backend/pom.xml b/uberfire-rest/uberfire-rest-backend/pom.xml
@@ -72,6 +72,11 @@
       <artifactId>slf4j-api</artifactId>
     </dependency>
 
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-text</artifactId>
+    </dependency>
+
     <!-- REST dependencies -->
     <dependency>
       <groupId>jakarta.annotation</groupId>

diff --git a/...ire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java b/...ire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java
@@ -46,6 +46,7 @@
 import javax.ws.rs.core.UriInfo;
 import javax.ws.rs.core.Variant;
 
+import org.apache.commons.text.StringEscapeUtils;
 import org.guvnor.common.services.project.model.WorkspaceProject;
 import org.guvnor.common.services.project.service.WorkspaceProjectService;
 import org.guvnor.rest.client.AddBranchJobRequest;
@@ -373,7 +374,7 @@ public Response addBranch(@PathParam("spaceName") String spaceName,
         jobRequest.setJobId(id);
         jobRequest.setSpaceName(spaceName);
         jobRequest.setProjectName(projectName);
-        jobRequest.setNewBranchName(addBranchRequest.getNewBranchName());
+        jobRequest.setNewBranchName(escapeHtmlInput(addBranchRequest.getNewBranchName()));
         jobRequest.setBaseBranchName(addBranchRequest.getBaseBranchName());
         jobRequest.setUserIdentifier(sessionInfo.getIdentity().getIdentifier());
         addAcceptedJobResult(id);
@@ -454,6 +455,16 @@ private ProjectResponse getProjectResponse(WorkspaceProject workspaceProject) {
         return projectResponse;
     }
 
+    private String escapeHtmlInput(String input) {
+        if (input != null) {
+            String escapedInput = StringEscapeUtils.escapeHtml4(input);
+            escapedInput = escapedInput.replace("'", "");
+            return escapedInput;
+        } else {
+            return null;
+        }
+    }
+
     @POST
     @Produces(MediaType.APPLICATION_JSON)
     @Path("/spaces/{spaceName}/projects/{projectName}/maven/compile")
@@ -684,7 +695,7 @@ public Response createSpace(Space space) {
         jobRequest.setJobId(id);
         jobRequest.setSpaceName(space.getName());
         jobRequest.setDescription(space.getDescription());
-        jobRequest.setOwner(space.getOwner());
+        jobRequest.setOwner(escapeHtmlInput(space.getOwner()));
         jobRequest.setDefaultGroupId(space.getDefaultGroupId());
         addAcceptedJobResult(id);
 
@@ -709,7 +720,7 @@ public Response updateSpace(Space space) {
         jobRequest.setJobId(id);
         jobRequest.setSpaceName(space.getName());
         jobRequest.setDescription(space.getDescription());
-        jobRequest.setOwner(space.getOwner());
+        jobRequest.setOwner(escapeHtmlInput(space.getOwner()));
         jobRequest.setDefaultGroupId(space.getDefaultGroupId());
         addAcceptedJobResult(id);
 

diff --git a/...t/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java b/...t/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java
@@ -315,6 +315,28 @@ public void updateSpace() throws Exception {
         assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
     }
 
+    @Test
+    public void updateSpaceWithXSSOwer() throws Exception {
+        String xssOwner = "<img/src/onerror=alert(\"XSS\")>";
+        Space testedSpace = new Space();
+        testedSpace.setOwner(xssOwner);
+        projectResource.updateSpace(testedSpace);
+
+        verify(jobManager).putJob(jobResultArgumentCaptor.capture());
+        assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
+    }
+
+    @Test
+    public void createSpaceWithXSSOwner() throws Exception {
+        String xssOwner = "<img/src/onerror=alert(document.cookie)>";
+        Space testedSpace = new Space();
+        testedSpace.setOwner(xssOwner);
+        projectResource.createSpace(testedSpace);
+
+        verify(jobManager).putJob(jobResultArgumentCaptor.capture());
+        assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
+    }
+
     @Test
     public void deleteSpace() throws Exception {
 
@@ -334,6 +356,19 @@ public void addBranch() {
         assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
     }
 
+    @Test
+    public void addBranchWithXSSName() {
+        AddBranchRequest addBranchRequest = new AddBranchRequest();
+        addBranchRequest.setNewBranchName("<img/src/onerror=alert(\"Xss\")>");
+
+        projectResource.addBranch("spaceName",
+                                  "projectName",
+                                  addBranchRequest);
+
+        verify(jobManager).putJob(jobResultArgumentCaptor.capture());
+        assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
+    }
+
     @Test
     public void removeBranch() {
         projectResource.removeBranch("spaceName",

diff --git a/uberfire-structure/uberfire-structure-backend/pom.xml b/uberfire-structure/uberfire-structure-backend/pom.xml
@@ -140,6 +140,11 @@
       <artifactId>commons-fileupload</artifactId>
     </dependency>
 
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-text</artifactId>
+    </dependency>
+
     <dependency>
       <groupId>org.uberfire</groupId>
       <artifactId>uberfire-security-management-api</artifactId>

diff --git a/...erfire-structure-backend/src/main/java/org/guvnor/structure/backend/InputEscapeUtils.java b/...erfire-structure-backend/src/main/java/org/guvnor/structure/backend/InputEscapeUtils.java
@@ -0,0 +1,53 @@
+/*
+ * Copyright 2023 Red Hat, Inc. and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.guvnor.structure.backend;
+
+import java.util.ArrayList;
+import java.util.Collection;
+
+import org.apache.commons.text.StringEscapeUtils;
+import org.uberfire.security.Contributor;
+
+/**
+ * Utility class to provide input escaping or other
+ * functionality needed by backend services to sanitize inputs.
+ */
+public class InputEscapeUtils {
+
+    private InputEscapeUtils() {
+        // non-instantiable class
+    }
+
+    public static Collection<Contributor> escapeContributorsNames(Collection<Contributor> contributors) {
+        Collection<Contributor> escapedContributors = new ArrayList<>();
+        contributors.forEach((contributor -> {
+            String escapedName = escapeHtmlInput(contributor.getUsername());
+            escapedContributors.add(new Contributor(escapedName, contributor.getType()));
+        }));
+        return escapedContributors;
+    }
+
+    public static String escapeHtmlInput(String input) {
+        if (input != null) {
+            String escapedInput = StringEscapeUtils.escapeHtml4(input);
+            escapedInput = escapedInput.replace("'", "");
+            return escapedInput;
+        } else {
+            return null;
+        }
+    }
+
+}
diff --git a/...n/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java b/...n/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java
@@ -64,6 +64,8 @@
 import org.uberfire.spaces.Space;
 import org.uberfire.spaces.SpacesAPI;
 
+import static org.guvnor.structure.backend.InputEscapeUtils.escapeContributorsNames;
+
 @Service
 @ApplicationScoped
 public class OrganizationalUnitServiceImpl implements OrganizationalUnitService {
@@ -320,7 +322,7 @@ public OrganizationalUnit createOrganizationalUnit(final String name,
             final SpaceInfo spaceInfo = new SpaceInfo(name,
                                                       description,
                                                       _defaultGroupId,
-                                                      contributors,
+                                                      escapeContributorsNames(contributors),
                                                       getRepositoryAliases(repositories),
                                                       Collections.emptyList());
             spaceConfigStorageRegistry.get(name).saveSpaceInfo(spaceInfo);
@@ -378,7 +380,7 @@ public OrganizationalUnit updateOrganizationalUnit(String name,
                         spaceInfo.setDefaultGroupId(_defaultGroupId);
 
                         if (contributors != null) {
-                            spaceInfo.setContributors(contributors);
+                            spaceInfo.setContributors(escapeContributorsNames(contributors));
                         }
 
                         if (description != null) {

diff --git a/...ackend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java b/...ackend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java
@@ -67,6 +67,7 @@
 import org.uberfire.spaces.Space;
 import org.uberfire.spaces.SpacesAPI;
 
+import static org.guvnor.structure.backend.InputEscapeUtils.escapeContributorsNames;
 import static org.guvnor.structure.repositories.EnvironmentParameters.CRYPT_PREFIX;
 import static org.guvnor.structure.repositories.EnvironmentParameters.SECURE_PREFIX;
 import static org.guvnor.structure.repositories.EnvironmentParameters.SCHEME;
@@ -539,7 +540,7 @@ public void updateContributors(final Repository repository,
 
         thisRepositoryConfig.ifPresent(config -> {
             config.getConfiguration().add("contributors",
-                                          contributors);
+                                          escapeContributorsNames(contributors));
             this.saveRepositoryConfig(repository.getSpace().getName(),
                                       config);
             repositoryContributorsUpdatedEvent.fire(new RepositoryContributorsUpdatedEvent(getRepositoryFromSpace(repository.getSpace(),

diff --git a/...re-structure-backend/src/test/java/org/guvnor/structure/backend/InputEscapeUtilsTest.java b/...re-structure-backend/src/test/java/org/guvnor/structure/backend/InputEscapeUtilsTest.java
@@ -0,0 +1,33 @@
+package org.guvnor.structure.backend;
+
+import org.apache.commons.text.StringEscapeUtils;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+import static org.guvnor.structure.backend.InputEscapeUtils.escapeHtmlInput;
+
+public class InputEscapeUtilsTest {
+
+    @Test
+    public void testEscapeHtmlInput() {
+        final String xssString = "<img/src/onerror=alert(\"XSS\")>";
+        final String expectedResult = StringEscapeUtils.escapeHtml4(xssString).replace("'", "");
+        final String result = escapeHtmlInput(xssString);
+        Assertions.assertThat(result).isEqualTo(expectedResult);
+    }
+
+    @Test
+    public void testEscapeHtmlInputWithSingleQoutes() {
+        final String xssString = "<img/src/onerror=alert('XSS')>";
+        final String expectedResult = StringEscapeUtils.escapeHtml4(xssString).replace("'", "");
+        final String result = escapeHtmlInput(xssString);
+        Assertions.assertThat(result).isEqualTo(expectedResult);
+    }
+
+    @Test
+    public void testEscapeHtmlInputNull() {
+        final String xssString = null;
+        final String result = escapeHtmlInput(xssString);
+        Assertions.assertThat(result).isNull();
+    }
+}
diff --git a/...t/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java b/...t/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java
@@ -63,6 +63,7 @@
 import org.uberfire.spaces.Space;
 import org.uberfire.spaces.SpacesAPI;
 
+import static org.guvnor.structure.backend.InputEscapeUtils.escapeHtmlInput;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -268,6 +269,78 @@ public void createValidOrganizationalUnitTest() {
         assertEquals(SPACE_DESCRIPTION, ou.getDescription());
         assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId());
         assertEquals(contributors, ou.getContributors());
+        Assertions.assertThat(ou.getContributors()).hasSize(1);
+        Assertions.assertThat(ou.getContributors()).hasOnlyOneElementSatisfying((contributor) -> {
+            contributor.getUsername().equals(escapeHtmlInput(ADMIN));
+        });
+    }
+
+    @Test
+    public void createOrganizationalUnitWithPersistentXssInContributorTest() {
+        final String persistentXssContributor = "<img/src/onerror=alert(\"XSS\")>";
+        final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor);
+
+        List<Contributor> contributors = new ArrayList<>();
+        contributors.add(new Contributor(persistentXssContributor,
+                                         ContributorType.ADMIN));
+
+        setOUCreationPermission(true);
+
+        final OrganizationalUnit ou = organizationalUnitService.createOrganizationalUnit(SPACE_NAME,
+                                                                                         DEFAULT_GROUP_ID,
+                                                                                         new ArrayList<>(),
+                                                                                         contributors,
+                                                                                         SPACE_DESCRIPTION);
+
+        assertNotNull(ou);
+        verify(organizationalUnitFactory).newOrganizationalUnit(any());
+        assertEquals(SPACE_NAME, ou.getName());
+        assertEquals(SPACE_DESCRIPTION, ou.getDescription());
+        assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId());
+
+        Assertions.assertThat(ou.getContributors()).hasSize(1);
+        Assertions.assertThat(ou.getContributors()).hasOnlyOneElementSatisfying((contributor) -> {
+            contributor.getUsername().equals(escapedPersistentXssContributor);
+        });
+    }
+
+    @Test
+    public void createOrganizationalUnitWithPersistentXssAndValidContributorTest() {
+        final String persistentXssContributor = "<img/src/onerror=alert(\"XSS\")>";
+        final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor);
+        final String escapedAdminContributor = escapeHtmlInput(ADMIN);
+        final String regularContributor = "head_technician_junior-intern";
+        final String escapedRegularContributor = escapeHtmlInput(regularContributor);
+
+        List<Contributor> contributors = new ArrayList<>();
+        contributors.add(new Contributor(persistentXssContributor,
+                                         ContributorType.CONTRIBUTOR));
+        contributors.add(new Contributor(ADMIN,
+                                         ContributorType.ADMIN));
+        contributors.add(new Contributor(regularContributor,
+                                         ContributorType.OWNER));
+
+        setOUCreationPermission(true);
+
+        final OrganizationalUnit ou = organizationalUnitService.createOrganizationalUnit(SPACE_NAME,
+                                                                                         DEFAULT_GROUP_ID,
+                                                                                         new ArrayList<>(),
+                                                                                         contributors,
+                                                                                         SPACE_DESCRIPTION);
+
+        assertNotNull(ou);
+        verify(organizationalUnitFactory).newOrganizationalUnit(any());
+        assertEquals(SPACE_NAME, ou.getName());
+        assertEquals(SPACE_DESCRIPTION, ou.getDescription());
+        assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId());
+
+        Assertions.assertThat(ou.getContributors()).hasSize(3);
+        Assertions.assertThat(ou.getContributors()).containsExactly(new Contributor(escapedPersistentXssContributor,
+                                                                                    ContributorType.CONTRIBUTOR),
+                                                                    new Contributor(escapedAdminContributor,
+                                                                                    ContributorType.ADMIN),
+                                                                    new Contributor(escapedRegularContributor,
+                                                                                    ContributorType.OWNER));
     }
 
     @Test
@@ -326,6 +399,40 @@ public void testUpdateOrganizationalUnit() {
         verify(spaceConfigStorage).endBatch();
     }
 
+    @Test
+    public void testContributorsPersistentXssOnUpdateOrganizationalUnit() {
+        final String persistentXssContributor = "<img/src/onerror=alert(\"XSS\")>";
+        final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor);
+
+        OrganizationalUnit organizationalUnit =
+                organizationalUnitService.updateOrganizationalUnit(SPACE_NAME,
+                                                                   DEFAULT_GROUP_ID,
+                                                                   Collections.singletonList(
+                                                                           new Contributor(
+                                                                                   persistentXssContributor,
+                                                                                   ContributorType.ADMIN
+                                                                           )
+                                                                   )
+                );
+
+        Assertions.assertThat(organizationalUnit)
+                .hasFieldOrPropertyWithValue("name", SPACE_NAME)
+                .hasFieldOrPropertyWithValue("defaultGroupId", DEFAULT_GROUP_ID);
+
+        Assertions.assertThat(organizationalUnit.getContributors()).hasSize(1);
+        Assertions.assertThat(organizationalUnit.getContributors()).hasOnlyOneElementSatisfying((contributor) -> {
+            contributor.getUsername().equals(escapedPersistentXssContributor);
+        });
+
+        Assertions.assertThat(spaceInfo)
+                .hasFieldOrPropertyWithValue("name", SPACE_NAME)
+                .hasFieldOrPropertyWithValue("defaultGroupId", DEFAULT_GROUP_ID);
+
+        verify(spaceConfigStorage).startBatch();
+        verify(spaceConfigStorage).saveSpaceInfo(eq(spaceInfo));
+        verify(spaceConfigStorage).endBatch();
+    }
+
     @Test
     public void testCheckChildrenRepositoryContributors() {
         OrganizationalUnit organizationalUnit = new OrganizationalUnitImpl(SPACE_NAME, DEFAULT_GROUP_ID);