diff --git a/examples/src/main/java/example/SecurityInterceptors.java b/examples/src/main/java/example/SecurityInterceptors.java new file mode 100644 index 00000000000..33e1df21b01 --- /dev/null +++ b/examples/src/main/java/example/SecurityInterceptors.java @@ -0,0 +1,56 @@ +package example; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.commons.codec.binary.Base64; + +import ca.uhn.fhir.rest.method.RequestDetails; +import ca.uhn.fhir.rest.server.exceptions.AuthenticationException; +import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter; + +public class SecurityInterceptors { + +// START SNIPPET: basicAuthInterceptor +public class BasicSecurityInterceptor extends InterceptorAdapter +{ + + /** + * This interceptor implements HTTP Basic Auth, which specifies that + * a username and password are provided in a header called Authorization. + */ + @Override + public boolean incomingRequestPostProcessed(RequestDetails theRequestDetails, HttpServletRequest theRequest, HttpServletResponse theResponse) throws AuthenticationException { + String authHeader = theRequest.getHeader("Authorization"); + + // The format of the header must be: + // Authorization: Basic [base64 of username:password] + if (authHeader == null || authHeader.startsWith("Basic ") == false) { + throw new AuthenticationException("Missing or invalid Authorization header"); + } + + String base64 = authHeader.substring("Basic ".length()); + String base64decoded = new String(Base64.decodeBase64(base64)); + String[] parts = base64decoded.split("\\:"); + + String username = parts[0]; + String password = parts[1]; + + /* + * Here we test for a hardcoded username & password. This is + * not typically how you would implement this in a production + * system of course.. + */ + if (!username.equals("someuser") || !password.equals("thepassword")) { + throw new AuthenticationException("Invalid username or password"); + } + + // Return true to allow the request to proceed + return true; + } + + +} +//END SNIPPET: basicAuthInterceptor + +} diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/XmlUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/XmlUtil.java index 0b5f94668d5..95f998693fa 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/XmlUtil.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/XmlUtil.java @@ -1562,6 +1562,17 @@ public class XmlUtil { logStaxImplementation(inputFactory.getClass()); } + /* + * These two properties disable external entity processing, which can + * be a security vulnerability. + * + * See https://github.com/jamesagnew/hapi-fhir/issues/339 + * https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing + */ + inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false); // This disables DTDs entirely for that factory + inputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false); // disable external entities + + /* * In the following few lines, you can uncomment the first and comment the second to disable automatic * parsing of extended entities, e.g. § diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/SchemaBaseValidator.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/SchemaBaseValidator.java index d6b7ab066f4..bb871540233 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/SchemaBaseValidator.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/SchemaBaseValidator.java @@ -93,11 +93,26 @@ public class SchemaBaseValidator implements IValidatorModule { encodedResource = theContext.getFhirContext().newXmlParser().encodeResourceToString((IBaseResource) theContext.getResource()); } + /* + * See https://github.com/jamesagnew/hapi-fhir/issues/339 + * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing + */ + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + validator.validate(new StreamSource(new StringReader(encodedResource))); + } catch (SAXParseException e) { + SingleValidationMessage message = new SingleValidationMessage(); + message.setLocationLine(e.getLineNumber()); + message.setLocationCol(e.getColumnNumber()); + message.setMessage(e.getLocalizedMessage()); + message.setSeverity(ResultSeverityEnum.FATAL); + theContext.addValidationMessage(message); } catch (SAXException e) { - throw new ConfigurationException("Could not apply schema file", e); + // Catch all + throw new ConfigurationException("Could not load/parse schema file", e); } catch (IOException e) { - // This shouldn't happen since we're using a string source + // Catch all throw new ConfigurationException("Could not load/parse schema file", e); } } @@ -117,6 +132,13 @@ public class SchemaBaseValidator implements IValidatorModule { schemaFactory.setResourceResolver(new MyResourceResolver()); try { + + /* + * See https://github.com/jamesagnew/hapi-fhir/issues/339 + * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing + */ + schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + schema = schemaFactory.newSchema(new Source[] { baseSource }); } catch (SAXException e) { throw new ConfigurationException("Could not load/parse schema file: " + theSchemaName, e); diff --git a/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/ValidateCommand.java b/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/ValidateCommand.java index 8554cd09dc9..5a5359e6320 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/ValidateCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/ValidateCommand.java @@ -119,7 +119,8 @@ public class ValidateCommand extends BaseCommand { org.hl7.fhir.instance.hapi.validation.ValidationSupportChain validationSupport = new org.hl7.fhir.instance.hapi.validation.ValidationSupportChain( new org.hl7.fhir.instance.hapi.validation.DefaultProfileValidationSupport()); if (localProfileResource != null) { - instanceValidator.setStructureDefintion((org.hl7.fhir.instance.model.StructureDefinition) localProfileResource); + org.hl7.fhir.instance.model.StructureDefinition convertedSd = FhirContext.forDstu2Hl7Org().newXmlParser().parseResource(org.hl7.fhir.instance.model.StructureDefinition.class, ctx.newXmlParser().encodeResourceToString(localProfileResource)); + instanceValidator.setStructureDefintion(convertedSd); } if (theCommandLine.hasOption("r")) { validationSupport.addValidationSupport(new LoadingValidationSupportDstu2()); diff --git a/hapi-fhir-cli/hapi-fhir-cli-app/src/test/java/ValidateTest.java b/hapi-fhir-cli/hapi-fhir-cli-app/src/test/java/ValidateTest.java index 998cd727642..e43c4295a70 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-app/src/test/java/ValidateTest.java +++ b/hapi-fhir-cli/hapi-fhir-cli-app/src/test/java/ValidateTest.java @@ -1,5 +1,4 @@ import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import ca.uhn.fhir.cli.App; @@ -13,7 +12,6 @@ public class ValidateTest { } @Test - @Ignore public void testValidateLocalProfile() { String profilePath = ValidateTest.class.getResource("/uslab-patient.profile.xml").getFile(); String resourcePath = ValidateTest.class.getResource("/patient-uslab-example1.xml").getFile(); @@ -23,15 +21,5 @@ public class ValidateTest { App.main(new String[] {"validate", "-p", "-n", resourcePath, "-l", profilePath}); } - @Test - @Ignore - public void testValidateLocalProfileWithReferenced() { - String profilePath = ValidateTest.class.getResource("/nl/nl-core-patient.dstu2.xml").getFile(); - String resourcePath = ValidateTest.class.getResource("/nl/patient-example-a.xml").getFile(); - ourLog.info(profilePath); - ourLog.info(resourcePath); - - App.main(new String[] {"validate", "-p", "-n", resourcePath, "-l", profilePath}); - } } diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java index caebc2d48b8..82cb1b4d4e1 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java @@ -22,9 +22,9 @@ import java.io.IOException; import java.io.StringReader; import java.util.ArrayList; import java.util.Arrays; -import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.UUID; import org.apache.commons.io.IOUtils; @@ -146,7 +146,7 @@ public class XmlParserDstu3Test { assertArrayEquals(new byte[] { 1, 2, 3, 4 }, bin.getContent()); } - + @Test public void testContainedResourceInExtensionUndeclared() { Patient p = new Patient(); @@ -1139,8 +1139,6 @@ public class XmlParserDstu3Test { assertThat(encoded, containsString("maritalStatus")); } - - @Test public void testEncodeNonContained() { // Create an organization @@ -1181,6 +1179,8 @@ public class XmlParserDstu3Test { } + + /** * See #312 */ @@ -1236,7 +1236,7 @@ public class XmlParserDstu3Test { assertThat(str, containsString("")); assertThat(str, containsString("")); } - + @Test public void testEncodeSummary() { Patient patient = new Patient(); @@ -1254,7 +1254,7 @@ public class XmlParserDstu3Test { assertThat(encoded, containsString("family")); assertThat(encoded, not(containsString("maritalStatus"))); } - + @Test public void testEncodeSummary2() { Patient patient = new Patient(); @@ -2536,6 +2536,38 @@ public class XmlParserDstu3Test { assertEquals("Patient", reincarnatedPatient.getIdElement().getResourceType()); } + /** + * See #339 + * + * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing + */ + @Test + public void testXxe() { + //@formatter:off + String input = + "\n" + + "\n" + + "]>" + + "" + + "" + + "
TEXT &xxe; TEXT
\n" + + "" + + "
" + + "" + + "
" + + ""; + //@formatter:on + + try { + ourCtx.newXmlParser().parseResource(Patient.class, input); + fail(); + } catch (DataFormatException e) { + assertThat(e.toString(), containsString("Undeclared general entity")); + } + + } + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchWithServerAddressStrategyDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchWithServerAddressStrategyDstu3Test.java new file mode 100644 index 00000000000..613c80134e9 --- /dev/null +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchWithServerAddressStrategyDstu3Test.java @@ -0,0 +1,125 @@ +package ca.uhn.fhir.rest.server; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import org.apache.commons.io.IOUtils; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.hl7.fhir.dstu3.model.HumanName; +import org.hl7.fhir.dstu3.model.Patient; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.annotation.RequiredParam; +import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.param.TokenParam; +import ca.uhn.fhir.util.PortUtil; +import ca.uhn.fhir.util.TestUtil; + +public class SearchWithServerAddressStrategyDstu3Test { + + private static CloseableHttpClient ourClient; + private static FhirContext ourCtx = FhirContext.forDstu3(); + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SearchWithServerAddressStrategyDstu3Test.class); + private static int ourPort; + private static Server ourServer; + private static RestfulServer ourServlet; + + @Test + public void testIncomingRequestAddressStrategy() throws Exception { + ourServlet.setServerAddressStrategy(new IncomingRequestAddressStrategy()); + + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, containsString("")); + } + + @Test + public void testHardcodedAddressStrategy() throws Exception { + ourServlet.setServerAddressStrategy(new HardcodedServerAddressStrategy("http://example.com/fhir/base")); + + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, containsString("")); + } + + + @AfterClass + public static void afterClassClearContext() throws Exception { + ourServer.stop(); + TestUtil.clearAllStaticFieldsForUnitTest(); + } + + + + @BeforeClass + public static void beforeClass() throws Exception { + ourPort = PortUtil.findFreePort(); + ourServer = new Server(ourPort); + + DummyPatientResourceProvider patientProvider = new DummyPatientResourceProvider(); + + ServletHandler proxyHandler = new ServletHandler(); + ourServlet = new RestfulServer(ourCtx); + ourServlet.setPagingProvider(new FifoMemoryPagingProvider(10)); + + ourServlet.setResourceProviders(patientProvider); + ServletHolder servletHolder = new ServletHolder(ourServlet); + proxyHandler.addServletWithMapping(servletHolder, "/*"); + ourServer.setHandler(proxyHandler); + ourServer.start(); + + PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(5000, TimeUnit.MILLISECONDS); + HttpClientBuilder builder = HttpClientBuilder.create(); + builder.setConnectionManager(connectionManager); + ourClient = builder.build(); + + } + + public static class DummyPatientResourceProvider implements IResourceProvider { + + @Override + public Class getResourceType() { + return Patient.class; + } + + //@formatter:off + @Search() + public List searchByIdentifier() { + ArrayList retVal = new ArrayList(); + retVal.add((Patient) new Patient().addName(new HumanName().addFamily("FAMILY")).setId("1")); + retVal.add((Patient) new Patient().addName(new HumanName().addFamily("FAMILY")).setId("2")); + return retVal; + } + //@formatter:on + + + } + +} diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/validation/SchemaValidationTestDstu3.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/validation/SchemaValidationTestDstu3.java new file mode 100644 index 00000000000..dbde0cb424f --- /dev/null +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/validation/SchemaValidationTestDstu3.java @@ -0,0 +1,63 @@ +package ca.uhn.fhir.validation; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.*; + +import java.util.Locale; + +import org.hl7.fhir.dstu3.model.Patient; +import org.junit.AfterClass; +import org.junit.Test; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.parser.DataFormatException; +import ca.uhn.fhir.util.TestUtil; + +public class SchemaValidationTestDstu3 { + + private static FhirContext ourCtx = FhirContext.forDstu3(); + + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SchemaValidationTestDstu3.class); + + /** + * See #339 + * + * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing + */ + @Test + public void testXxe() { + //@formatter:off + String input = + "\n" + + "\n" + + "]>" + + "" + + "" + + "" + + "
TEXT &xxe; TEXT
\n" + + "" + + "
" + + "" + + "
" + + ""; + //@formatter:on + + FhirValidator val = ourCtx.newValidator(); + val.setValidateAgainstStandardSchema(true); + val.setValidateAgainstStandardSchematron(false); + ValidationResult result = val.validateWithResult(input); + + String encoded = ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(result.toOperationOutcome()); + ourLog.info(encoded); + + assertFalse(result.isSuccessful()); + assertThat(encoded, containsString("passwd")); + assertThat(encoded, containsString("accessExternalDTD")); + } + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 80c90b067fc..a5ebaf0f66f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -7,6 +7,13 @@ + + Security Fix: XML parser was vulnerable to XXE (XML External Entity) + processing, which could result in local files on disk being disclosed. + See this page]]> + for more information. + Thanks to Jim Steel for reporting! + Bump the version of a few dependencies to the latest versions (dependent HAPI modules listed in brackets): diff --git a/src/site/xdoc/doc_rest_server_security.xml b/src/site/xdoc/doc_rest_server_security.xml new file mode 100644 index 00000000000..a46fc69d269 --- /dev/null +++ b/src/site/xdoc/doc_rest_server_security.xml @@ -0,0 +1,86 @@ + + + + + Server Security + + + + + +
+ +

+ Security is a complex topic which goes far beyond the scope of HAPI FHIR. + HAPI does provide mechanisms which can be used to implement security in + your server however. +

+ +

+ Because HAPI FHIR's REST server is based on the Servlet API, you may use any + security mechanism which works in that environment. Some serlvet containers + may provide security layers you can plug into. The rest of this page + does not explore that method, but rather looks at HAPI FHIR hooks that can + be used to implement FHIR specific security. +

+ + + +

+ Background reading: Wikipedia - Authentication +

+

+ Server security is divided into two topics: +

+
    +
  • + Authentication (AuthN): Is verifying that the user is who they say they + are. This is typically accomplished by testing a username/password in the request, + or by checking a "bearer token" in the request. +
    • +
  • + Authorization (AuthZ): Is verifying that the user is allowed to perform + the given action. For example, in a FHIR application you might use AuthN to test that + the user making a request to the FHIR server is allowed to access the server, but + that test might determine that the requesting user is not permitted to perform + write operations and therefore block a FHIR create operation. This is + AuthN and AuthZ in action. +
    • +
+ + + +
+ +
+ +

+ The Server Interceptor + framework can provide an easy way to test for credentials. The following + example shows a simple interceptor which tests for HTTP Basic Auth. +

+ + + + + + +
+ +
+ +

+ HAPI FHIR 1.5 introduced a new interceptor, the + AuthorizationInterceptor. +

+

+ This interceptor can help with the complicated task of determining whether a user + has the appropriate permission to perform a given task on a FHIR server. This is + done by declaring +

+ +
+ + + +