From a695f8996c3fe0e531add3ea9abdec2027886210 Mon Sep 17 00:00:00 2001 From: e551763 Date: Thu, 6 Jun 2024 16:38:51 +0200 Subject: [PATCH] fix: security alerts fix (uncontrolled data usage, potential XSS vulnerabilities & inefficient regular expressions) --- .../extension/generic/GenericUiServlet.java | 49 ++++++++++++------- app/src/main/resources/js/common.js | 4 +- app/src/main/resources/js/custom-select.js | 14 +++++- .../generic/GenericUiServletTest.java | 35 ++++++++----- 4 files changed, 68 insertions(+), 34 deletions(-) diff --git a/app/src/main/java/ch/sbb/polarion/extension/generic/GenericUiServlet.java b/app/src/main/java/ch/sbb/polarion/extension/generic/GenericUiServlet.java index edbd4d4..a808f8e 100644 --- a/app/src/main/java/ch/sbb/polarion/extension/generic/GenericUiServlet.java +++ b/app/src/main/java/ch/sbb/polarion/extension/generic/GenericUiServlet.java @@ -1,5 +1,6 @@ package ch.sbb.polarion.extension.generic; +import com.polarion.alm.shared.util.Pair; import com.polarion.core.util.logging.Logger; import org.apache.commons.io.IOUtils; import org.jetbrains.annotations.NotNull; @@ -16,11 +17,22 @@ import java.io.Serial; import java.net.URI; import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.List; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; public abstract class GenericUiServlet extends HttpServlet { + private static final List> ALLOWED_FILE_TYPES = Arrays.asList( + Pair.of(".js", "text/javascript"), + Pair.of(".html", "text/html"), + Pair.of(".css", "text/css"), + Pair.of(".png", "image/png"), + Pair.of(".svg", "image/svg+xml"), + Pair.of(".gif", "image/gif") + ); + private static final Logger logger = Logger.getLogger(GenericUiServlet.class); @Serial @@ -34,36 +46,39 @@ protected GenericUiServlet(String webAppName) { @VisibleForTesting static void setContentType(@NotNull String uri, @NotNull HttpServletResponse response) { - if (uri.endsWith(".js")) { - response.setContentType("text/javascript"); - } else if (uri.endsWith(".html")) { - response.setContentType("text/html"); - } else if (uri.endsWith(".png")) { - response.setContentType("image/png"); - } else if (uri.endsWith(".css")) { - response.setContentType("text/css"); - } + response.setContentType(ALLOWED_FILE_TYPES.stream().filter(f -> uri.endsWith(f.left())).map(Pair::right).findFirst() + .orElseThrow(() -> new IllegalArgumentException("Unsupported file type"))); } @Override protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { - String uri = request.getRequestURI(); - String relativeUri = uri.substring("/polarion/".length()); + String resourceUri = getInternalResourcePath(request.getRequestURI()); try { - if (relativeUri.startsWith(webAppName + "/ui/generic/")) { - serveGenericResource(response, relativeUri.substring((webAppName + "/ui/generic/").length())); - } else if (relativeUri.startsWith(webAppName + "/ui/")) { - serveResource(response, relativeUri.substring((webAppName + "/ui").length())); + if (resourceUri.startsWith("generic/")) { + serveGenericResource(response, resourceUri.substring("generic/".length())); + } else { + serveResource(response, "/" + resourceUri); } } catch (IOException e) { - logger.error("Cannot copy resource '" + relativeUri + "': " + e.getMessage(), e); + logger.error("Cannot copy resource '" + resourceUri + "': " + e.getMessage(), e); throw e; } catch (Exception e) { - logger.error("Unexpected error by getting resource '" + relativeUri + "': " + e.getMessage(), e); + logger.error("Unexpected error by getting resource '" + resourceUri + "': " + e.getMessage(), e); throw new ServletException(e.getMessage(), e); } } + private String getInternalResourcePath(String fullUri) { + String acceptablePath = "/polarion/" + webAppName + "/ui/"; + if (!fullUri.startsWith(acceptablePath)) { + throw new IllegalArgumentException("Unsupported resource path"); + } + if (ALLOWED_FILE_TYPES.stream().noneMatch(t -> fullUri.endsWith(t.left()))) { + throw new IllegalArgumentException("Unsupported file type"); + } + return fullUri.substring(acceptablePath.length()); + } + @VisibleForTesting void serveGenericResource(@NotNull HttpServletResponse response, @NotNull String uri) throws IOException, URISyntaxException { final URI currentJar = GenericUiServlet.class.getProtectionDomain().getCodeSource().getLocation().toURI(); diff --git a/app/src/main/resources/js/common.js b/app/src/main/resources/js/common.js index f6a17ac..acccc58 100644 --- a/app/src/main/resources/js/common.js +++ b/app/src/main/resources/js/common.js @@ -16,9 +16,9 @@ const SbbCommon = { Prism.languages.properties = { comment: /^[ \t]*[#!].*$/m, //ORIGINAL value: {pattern: /(^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\\s:=])+(?: *[=:] *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"}, - value: {pattern: /(^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\=])+(?: *= *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"}, + value: {pattern: /(^[ \t]*(?:\\[\s\S]|[^\\=])+(?: *= *(?! )| ))(?:\\(?:\r\n|[\s\S])|[^\\\r\n])+/m, lookbehind: !0, alias: "attr-value"}, //ORIGINAL key: {pattern: /^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\\s:=])+(?= *[=:]| )/m, alias: "attr-name"}, - key: {pattern: /^[ \t]*(?:\\(?:\r\n|[\s\S])|[^\\=])+(?= *[=:]| )/m, alias: "attr-name"}, + key: {pattern: /^[ \t]*(?:\\[\s\S]|[^\\=])+(?= *[=:]| )/m, alias: "attr-name"}, punctuation: /[=:]/ }; } diff --git a/app/src/main/resources/js/custom-select.js b/app/src/main/resources/js/custom-select.js index 18f44c7..9eaa976 100644 --- a/app/src/main/resources/js/custom-select.js +++ b/app/src/main/resources/js/custom-select.js @@ -128,7 +128,7 @@ SbbCustomSelect.prototype.selectMultipleValues = function(values) { SbbCustomSelect.prototype.handleChange = function(event) { if (this.mutiselect) { - this.selectElement.innerHTML = ""; + this.setSelectedOptionValue(this.getSelectedText().join(", ")); if (this.changeListener) { this.changeListener(this.checkboxContainer.querySelectorAll('input[type="checkbox"]:checked')); } @@ -146,7 +146,7 @@ SbbCustomSelect.prototype.handleChange = function(event) { } }); - this.selectElement.innerHTML = ""; + this.setSelectedOptionValue(selectedCheckbox.parentElement.textContent); this.checkboxContainer.querySelectorAll('label').forEach(function (label) { label.classList.remove("selected"); if (label.textContent === selectedCheckbox.parentElement.textContent) { @@ -161,4 +161,14 @@ SbbCustomSelect.prototype.handleChange = function(event) { } this.checkboxContainer.style.display = "none"; } + + // Using code like: + // this.selectElement.innerHTML = "" + // results in XSS vulnerability. The code below solves this issue. + SbbCustomSelect.prototype.setSelectedOptionValue = function(optionText) { + const optionElement = document.createElement("option"); + optionElement.textContent = optionText; + this.selectElement.innerHTML = ''; + this.selectElement.appendChild(optionElement); + }; } \ No newline at end of file diff --git a/app/src/test/java/ch/sbb/polarion/extension/generic/GenericUiServletTest.java b/app/src/test/java/ch/sbb/polarion/extension/generic/GenericUiServletTest.java index 295a1fa..167f322 100644 --- a/app/src/test/java/ch/sbb/polarion/extension/generic/GenericUiServletTest.java +++ b/app/src/test/java/ch/sbb/polarion/extension/generic/GenericUiServletTest.java @@ -9,6 +9,7 @@ import javax.servlet.http.HttpServletResponse; import java.io.Serial; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.*; @@ -25,37 +26,45 @@ void testSetContentType() { GenericUiServlet.setContentType("/sub_path/file.html", response); verify(response, times(1)).setContentType("text/html"); + response = mock(HttpServletResponse.class); + GenericUiServlet.setContentType("https://localhost/styles.css", response); + verify(response, times(1)).setContentType("text/css"); + response = mock(HttpServletResponse.class); GenericUiServlet.setContentType("/img.png", response); verify(response, times(1)).setContentType("image/png"); response = mock(HttpServletResponse.class); - GenericUiServlet.setContentType("https://localhost/styles.css", response); - verify(response, times(1)).setContentType("text/css"); + GenericUiServlet.setContentType("/img.svg", response); + verify(response, times(1)).setContentType("image/svg+xml"); response = mock(HttpServletResponse.class); - GenericUiServlet.setContentType("unknown_file.xml", response); - verify(response, times(0)).setContentType(any()); + GenericUiServlet.setContentType("/img.gif", response); + verify(response, times(1)).setContentType("image/gif"); + + HttpServletResponse servletResponse = mock(HttpServletResponse.class); + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> GenericUiServlet.setContentType("unknown_file.xml", servletResponse)); + assertEquals("Unsupported file type", exception.getMessage()); } @Test @SneakyThrows void testService() { - // unreal case (at least we assume that uri will start with /polarion/) - assertThrows(StringIndexOutOfBoundsException.class, () -> callServlet("/badUrl")); + // error case (at least we assume that uri will start with /polarion/) + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> callServlet("/badUrl/someImg.png")); + assertEquals("Unsupported resource path", exception.getMessage()); - // does nothing coz we do not process sub-paths other than /ui/generic/ and /ui/ - TestServlet servlet = callServlet("/polarion/testServletName/unknownPath"); - verify(servlet, times(0)).serveGenericResource(any(), any()); - verify(servlet, times(0)).serveResource(any(), any()); + // we expect uri starting by /polarion/{web_app_name}/ui/ + exception = assertThrows(IllegalArgumentException.class, () -> callServlet("/polarion/testServletName/unknownPath")); + assertEquals("Unsupported resource path", exception.getMessage()); // generic resource - servlet = callServlet("/polarion/testServletName/ui/generic/genericUri"); - verify(servlet, times(1)).serveGenericResource(any(), eq("genericUri")); + TestServlet servlet = callServlet("/polarion/testServletName/ui/generic/genericUri/someImage.gif"); + verify(servlet, times(1)).serveGenericResource(any(), eq("genericUri/someImage.gif")); verify(servlet, times(0)).serveResource(any(), any()); // regular resource - servlet = callServlet("/polarion/testServletName/ui/regularUri"); + servlet = callServlet("/polarion/testServletName/ui/regularUri/someStyle.css"); verify(servlet, times(0)).serveGenericResource(any(), any()); verify(servlet, times(1)).serveResource(any(), any()); }