Merge branch 'master' of github.com:jamesagnew/hapi-fhir

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
+	
+}

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. §

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);

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());

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});
-	}
 
 }

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("<reference value=\"Patient/phitcc_pat_normal\"/>"));
 		assertThat(str, containsString("<reference value=\"Observation/phitcc_obs_bp_dia\"/>"));
 	}
-	
+
 	@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 =
+			"<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" + 
+			"<!DOCTYPE foo [  \n" + 
+			"<!ELEMENT foo ANY >\n" + 
+			"<!ENTITY xxe SYSTEM \"file:///etc/passwd\" >]>" +
+			"<Patient xmlns=\"http://hl7.org/fhir\">" +
+				"<text>" + 
+					"<div xmlns=\"http://www.w3.org/1999/xhtml\">TEXT &xxe; TEXT</div>\n" + 
+				"</text>" +
+				"<address>" + 
+					"<line value=\"FOO\"/>" + 
+				"</address>" +
+			"</Patient>";
+		//@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();

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("<family value=\"FAMILY\""));
+		assertThat(responseContent, containsString("<fullUrl value=\"http://localhost:" + ourPort + "/Patient/1\"/>"));
+	}
+
+	@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("<family value=\"FAMILY\""));
+		assertThat(responseContent, containsString("<fullUrl value=\"http://example.com/fhir/base/Patient/1\"/>"));
+	}
+
+	
+	 @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<? extends IBaseResource> getResourceType() {
+			return Patient.class;
+		}
+
+		//@formatter:off
+		@Search()
+		public List<Patient> searchByIdentifier() {
+			ArrayList<Patient> retVal = new ArrayList<Patient>();
+			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
+
+
+	}
+
+}

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 =
+			"<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" + 
+			"<!DOCTYPE foo [  \n" + 
+			"<!ELEMENT foo ANY >\n" + 
+			"<!ENTITY xxe SYSTEM \"file:///etc/passwd\" >]>" +
+			"<Patient xmlns=\"http://hl7.org/fhir\">" +
+				"<text>" + 
+					"<status value=\"generated\"/>" +
+					"<div xmlns=\"http://www.w3.org/1999/xhtml\">TEXT &xxe; TEXT</div>\n" + 
+				"</text>" +
+				"<address>" + 
+					"<line value=\"FOO\"/>" + 
+				"</address>" +
+			"</Patient>";
+		//@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();
+	}
+}

src/changes/changes.xml

@@ -7,6 +7,13 @@
 	</properties>
 	<body>
 		<release version="1.5" date="TBD">
+			<action type="fix" issue="339">
+				Security Fix: XML parser was vulnerable to XXE (XML External Entity) 
+				processing, which could result in local files on disk being disclosed.
+				See <![CDATA[<a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">this page</a>]]>
+				for more information.
+				Thanks to Jim Steel for reporting!
+			</action>
 			<action type="add">
 				Bump the version of a few dependencies to the
 				latest versions (dependent HAPI modules listed in brackets):

src/site/xdoc/doc_rest_server_security.xml

@@ -0,0 +1,86 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<document xmlns="http://maven.apache.org/XDOC/2.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/XDOC/2.0 http://maven.apache.org/xsd/xdoc-2.0.xsd">
+
+	<properties>
+		<title>Server Security</title>
+	</properties>
+
+	<body>
+
+		<!-- The body of the document contains a number of sections -->
+		<section name="Server Security">
+
+			<p>
+				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.
+			</p>
+			
+			<p>
+				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.
+			</p>
+			
+			<subsection name="Authentication vs Authorization">
+			
+				<p>
+					Background reading: <a href="https://en.wikipedia.org/wiki/Authentication">Wikipedia - Authentication</a>
+				</p>
+				<p>
+					Server security is divided into two topics:
+				</p>
+				<ul>
+					<li>
+						<b>Authentication (AuthN):</b> 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.
+					</li>
+					<li>
+						<b>Authorization (AuthZ):</b> 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 <code>create</code> operation. This is
+						AuthN and AuthZ in action.
+					</li>
+				</ul>
+				
+			</subsection>
+			
+		</section>
+		
+		<section name="Authentication Interceptors">
+		
+			<p>
+				The <a href="./doc_rest_server_interceptor.html">Server Interceptor</a>
+				framework can provide an easy way to test for credentials. The following
+				example shows a simple interceptor which tests for HTTP Basic Auth.
+			</p>
+			
+			<macro name="snippet">
+				<param name="id" value="basicAuthInterceptor" />
+				<param name="file" value="examples/src/main/java/example/SecurityInterceptors.java" />
+			</macro>
+			
+		</section>
+		
+		<section name="Authorization Interceptor">
+			
+			<p>
+				HAPI FHIR 1.5 introduced a new interceptor, the
+				<a href="./apidocs/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptor.html">AuthorizationInterceptor</a>.
+			</p>
+			<p>
+				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
+			</p>
+						
+		</section>
+		
+	</body>
+
+</document>