diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5717-reflected-xss-testpage-overlay-vulnerability.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5717-reflected-xss-testpage-overlay-vulnerability.yaml new file mode 100644 index 00000000000..fdc50e6da87 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5717-reflected-xss-testpage-overlay-vulnerability.yaml @@ -0,0 +1,4 @@ +--- +type: security +issue: 5717 +title: "Fixed a potential XSS vulnerability in the HAPI FHIR Testpage Overlay module." diff --git a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java index 5db555798a5..f47c0cce446 100644 --- a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java +++ b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java @@ -36,6 +36,7 @@ import ca.uhn.fhir.to.model.ResourceRequest; import ca.uhn.fhir.to.model.TransactionRequest; import ca.uhn.fhir.to.util.HfqlRenderingUtil; import ca.uhn.fhir.util.StopWatch; +import ca.uhn.fhir.util.UrlUtil; import com.google.gson.stream.JsonWriter; import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; @@ -279,7 +280,7 @@ public class Controller extends BaseController { theModel.put("errorMsg", toDisplayError(e.toString(), e)); return "resource"; } - String id = sanitizeUrlPart(defaultString(theServletRequest.getParameter("id"))); + String id = sanitizeUrlPart(UrlUtil.unescape(theServletRequest.getParameter("id"))); if (StringUtils.isBlank(id)) { populateModelForResource(theServletRequest, theRequest, theModel); theModel.put("errorMsg", toDisplayError("No ID specified", null)); @@ -287,7 +288,7 @@ public class Controller extends BaseController { } ResultType returnsResource = ResultType.RESOURCE; - String versionId = sanitizeUrlPart(defaultString(theServletRequest.getParameter("vid"))); + String versionId = sanitizeUrlPart(UrlUtil.unescape((theServletRequest.getParameter("vid")))); String outcomeDescription; if (StringUtils.isBlank(versionId)) { versionId = null; @@ -800,9 +801,9 @@ public class Controller extends BaseController { final BindingResult theBindingResult, final ModelMap theModel) { - String instanceType = theReq.getParameter("instanceType"); - String instanceId = theReq.getParameter("instanceId"); - String operationName = theReq.getParameter("operationName"); + String instanceType = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("instanceType")))); + String instanceId = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("instanceId")))); + String operationName = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("operationName")))); boolean finished = false; diff --git a/hapi-fhir-testpage-overlay/src/test/java/ca/uhn/fhir/jpa/test/WebTest.java b/hapi-fhir-testpage-overlay/src/test/java/ca/uhn/fhir/jpa/test/WebTest.java index 8c39d6e7fb5..3e91826a324 100644 --- a/hapi-fhir-testpage-overlay/src/test/java/ca/uhn/fhir/jpa/test/WebTest.java +++ b/hapi-fhir-testpage-overlay/src/test/java/ca/uhn/fhir/jpa/test/WebTest.java @@ -31,6 +31,7 @@ import org.htmlunit.WebClient; import org.htmlunit.cssparser.parser.CSSErrorHandler; import org.htmlunit.html.HtmlAnchor; import org.htmlunit.html.HtmlButton; +import org.htmlunit.html.HtmlElement; import org.htmlunit.html.HtmlPage; import org.htmlunit.html.HtmlTable; import org.htmlunit.html.HtmlTableCell; @@ -41,12 +42,14 @@ import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.test.web.servlet.MockMvc; -import org.springframework.test.web.servlet.htmlunit.MockMvcWebConnection; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; import org.springframework.web.servlet.DispatcherServlet; @@ -57,10 +60,15 @@ import java.io.InputStream; import java.net.MalformedURLException; import java.net.URL; import java.util.List; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Stream; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; @@ -191,6 +199,47 @@ public class WebTest { assertThat(summaryPage.asNormalizedText(), containsString("Result Narrative\t\nHELLO WORLD DOCUMENT")); } + private static Stream getButtonMappingPredicates() { + Predicate readButtonPredicate = t -> t.getAttribute("value").equals("read"); + Predicate summaryButtonPredicate = t -> t.asNormalizedText().equals("$summary"); + return Stream.of(arguments(readButtonPredicate), arguments(summaryButtonPredicate)); + } + + @ParameterizedTest + @MethodSource(value = "getButtonMappingPredicates") + public void testInvokeOperation_withReflectedXssAttack_resultHtmlIsSanitized( + Predicate theButtonMappingPredicate) throws IOException { + + register5Patients(); + + HtmlPage searchResultPage = searchForPatients(); + HtmlTable controlsTable = searchResultPage.getHtmlElementById("resultControlsTable"); + List controlRows = controlsTable.getBodies().get(0).getRows(); + HtmlTableCell controlsCell = controlRows.get(0).getCell(0); + + // find the button + HtmlElement summaryButton = controlsCell + .getElementsByTagName("button") + .stream() + .filter(theButtonMappingPredicate) + .findFirst() + .orElseThrow(); + + // alter button attributes to imitate Reflected XSS attack + summaryButton.setAttribute("data2", "A0%3Cscript%3Ealert(2)%3C/script%3E"); + summaryButton.setAttribute("data3", "%24diff%3Cscript%3Ealert(1)%3C/script%3E"); + HtmlPage summaryPage = summaryButton.click(); + + // validate that there is no