TRUNK-6550: Logging should not be vulnerable to injection attacks

api/src/main/java/org/openmrs/logging/OpenmrsLoggingUtil.java

@@ -198,4 +198,20 @@ public static void reloadLoggingConfiguration() {
 		((LoggerContext) LogManager.getContext(true)).reconfigure();
 	}
 
+	/**
+	 * Sanitizes a string for logging by replacing newline and carriage return characters with
+	 * underscores.
+	 *
+	 * @param value the string to sanitize
+	 * @return the sanitized string
+	 * @since 2.4.4, 2.5.1, 2.6.0
+	 */
+	@Logging(ignore = true)
+	public static String sanitize(Object value) {
+		if (value == null) {
+			return null;
+		}
+		return value.toString().replaceAll("[\n\r]", "_");
+	}
+
 }

api/src/main/resources/log4j2.xml

@@ -15,7 +15,7 @@
 	<Properties>
 		<!-- The default pattern is stored as a property so that it's only defined once.
 		     It's also quite challenging to escape using Log4j2's variable substitution. -->
-		<Property name="defaultPattern">%p - %C{1}.%M(%L) |%d{ISO8601}| %m%n</Property>
+		<Property name="defaultPattern">%p - %C{1}.%M(%L) |%d{ISO8601}| %replace{%m}{[\r\n]}{_}%n</Property>
 	</Properties>
 	<Appenders>
 		<!-- the console appender is not required but usually a good idea -->

api/src/test/java/org/openmrs/logging/OpenmrsLoggingUtilTest.java

@@ -0,0 +1,48 @@
+/**
+ * This Source Code Form is subject to the terms of the Mozilla Public License,
+ * v. 2.0. If a copy of the MPL was not distributed with this file, You can
+ * obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
+ * the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
+ *
+ * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
+ * graphic logo is a trademark of OpenMRS Inc.
+ */
+package org.openmrs.logging;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+/**
+ * Tests for {@link OpenmrsLoggingUtil}.
+ */
+public class OpenmrsLoggingUtilTest {
+
+	/**
+	 * @see OpenmrsLoggingUtil#sanitize(Object)
+	 */
+	@Test
+	public void sanitize_shouldReplaceNewlinesWithUnderscores() {
+		assertEquals("some_data", OpenmrsLoggingUtil.sanitize("some\ndata"));
+		assertEquals("some_data", OpenmrsLoggingUtil.sanitize("some\rdata"));
+		assertEquals("some__data", OpenmrsLoggingUtil.sanitize("some\r\ndata"));
+		assertEquals("no_newlines", OpenmrsLoggingUtil.sanitize("no_newlines"));
+	}
+
+	/**
+	 * @see OpenmrsLoggingUtil#sanitize(Object)
+	 */
+	@Test
+	public void sanitize_shouldHandleNullValue() {
+		assertNull(OpenmrsLoggingUtil.sanitize(null));
+	}
+
+	/**
+	 * @see OpenmrsLoggingUtil#sanitize(Object)
+	 */
+	@Test
+	public void sanitize_shouldHandleNonStringObjects() {
+		assertEquals("123", OpenmrsLoggingUtil.sanitize(123));
+	}
+}

liquibase/src/main/resources/log4j2.xml

@@ -14,7 +14,7 @@
 <Configuration xmlns="http://logging.apache.org/log4j/2.0/config">
 	<Appenders>
 		<Console name="CONSOLE" target="SYSTEM_OUT">
-			<PatternLayout pattern="%p - %C{1}.%M(%L) |%d{ISO8601}| %m%n" />
+			<PatternLayout pattern="%p - %C{1}.%M(%L) |%d{ISO8601}| %replace{%m}{[\r\n]}{_}%n" />
 		</Console>
 	</Appenders>
 	<Loggers>

web/src/main/java/org/openmrs/module/web/ModuleResourcesServlet.java

@@ -19,6 +19,8 @@
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 
+import org.apache.commons.lang3.StringUtils;
+import org.openmrs.logging.OpenmrsLoggingUtil;
 import org.openmrs.module.Module;
 import org.openmrs.module.ModuleUtil;
 import org.openmrs.util.OpenmrsUtil;
@@ -52,7 +54,7 @@ protected long getLastModified(HttpServletRequest req) {
 	@Override
 	protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
 
-		log.debug("In service method for module servlet: " + request.getPathInfo());
+		log.debug("In service method for module servlet: " + OpenmrsLoggingUtil.sanitize(request.getPathInfo()));
 
 		File f = getFile(request);
 		if (f == null) {
@@ -82,10 +84,13 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
 	protected File getFile(HttpServletRequest request) {
 
 		String path = request.getPathInfo();
+		if (StringUtils.isBlank(path) || !path.startsWith("/") || path.lastIndexOf("/") <= 0) {
+			return null;
+		}
 
 		Module module = ModuleUtil.getModuleForPath(path);
 		if (module == null) {
-			log.warn("No module handles the path: " + path);
+			log.warn("No module handles the path: " + OpenmrsLoggingUtil.sanitize(path));
 			return null;
 		}
 
@@ -107,13 +112,14 @@ protected File getFile(HttpServletRequest request) {
 		Path normalizedPath = Path.of(realPath).normalize();
 		Path normalizedBase = Path.of(basePath).normalize();
 		if (!normalizedPath.startsWith(normalizedBase)) {
-			log.warn("Detected attempted directory traversal with path: " + path);
+			log.warn("Detected attempted directory traversal with path: " + OpenmrsLoggingUtil.sanitize(path));
 			return null;
 		}
 
 		File f = normalizedPath.toFile();
 		if (!f.exists()) {
-			log.warn("No file with path '" + normalizedPath + "' exists for module '" + module.getModuleId() + "'");
+			log.warn("No file with path '" + OpenmrsLoggingUtil.sanitize(normalizedPath) + "' exists for module '"
+			        + OpenmrsLoggingUtil.sanitize(module.getModuleId()) + "'");
 			return null;
 		}
 

web/src/main/java/org/openmrs/module/web/ModuleServlet.java

@@ -21,6 +21,8 @@
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 
+import org.apache.commons.lang3.StringUtils;
+import org.openmrs.logging.OpenmrsLoggingUtil;
 import org.openmrs.module.Module;
 import org.openmrs.module.ModuleFactory;
 import org.slf4j.Logger;
@@ -34,22 +36,29 @@ public class ModuleServlet extends HttpServlet {
 
 	@Override
 	protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
-		log.debug("In service method for module servlet: " + request.getPathInfo());
-		String servletName = request.getPathInfo();
+		log.debug("In service method for module servlet: " + OpenmrsLoggingUtil.sanitize(request.getPathInfo()));
+		String path = request.getPathInfo();
+		if (StringUtils.isBlank(path) || "/".equals(path)) {
+			response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+			return;
+		}
+
+		String servletName = path;
 		int end = servletName.indexOf("/", 1);
 
 		String moduleId = null;
 		if (end > 0) {
 			moduleId = servletName.substring(1, end);
 		}
 
-		log.debug("ModuleId: " + moduleId);
+		log.debug("ModuleId: " + OpenmrsLoggingUtil.sanitize(moduleId));
 		Module mod = ModuleFactory.getModuleById(moduleId);
 
 		// where in the path to start trimming
 		int start = 1;
 		if (mod != null) {
-			log.debug("Module with id " + moduleId + " found.  Looking for servlet name after " + moduleId + " in url path");
+			log.debug("Module with id " + OpenmrsLoggingUtil.sanitize(moduleId) + " found.  Looking for servlet name after "
+			        + OpenmrsLoggingUtil.sanitize(moduleId) + " in url path");
 			start = moduleId.length() + 2;
 			// this skips over the moduleId that is in the path
 		}
@@ -60,12 +69,12 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
 		}
 		servletName = servletName.substring(start, end);
 
-		log.debug("Servlet name: " + servletName);
+		log.debug("Servlet name: " + OpenmrsLoggingUtil.sanitize(servletName));
 
 		HttpServlet servlet = WebModuleUtil.getServlet(servletName);
 
 		if (servlet == null) {
-			log.warn("No servlet with name: " + servletName + " was found");
+			log.warn("No servlet with name: " + OpenmrsLoggingUtil.sanitize(servletName) + " was found");
 			response.setStatus(HttpServletResponse.SC_NOT_FOUND);
 			return;
 		}

web/src/main/java/org/openmrs/web/filter/initialization/InitializationFilter.java

@@ -56,6 +56,7 @@
 import org.openmrs.liquibase.ChangeLogDetective;
 import org.openmrs.liquibase.ChangeLogVersionFinder;
 import org.openmrs.liquibase.ChangeSetExecutorCallback;
+import org.openmrs.logging.OpenmrsLoggingUtil;
 import org.openmrs.module.MandatoryModuleException;
 import org.openmrs.module.web.WebModuleUtil;
 import org.openmrs.util.DatabaseUpdateException;
@@ -346,7 +347,8 @@ protected void initializeWizardFromResolvedPropertiesIfPresent() {
 		if (log.isDebugEnabled()) {
 			for (String key : script.stringPropertyNames()) {
 				String value = script.getProperty(key);
-				log.debug("{} = {}", key, key.toLowerCase().contains("password") ? "*******" : value);
+				log.debug("{} = {}", OpenmrsLoggingUtil.sanitize(key),
+				    key.toLowerCase().contains("password") ? "*******" : OpenmrsLoggingUtil.sanitize(value));
 			}
 		}
 
@@ -481,7 +483,8 @@ protected void doPost(HttpServletRequest httpRequest, HttpServletResponse httpRe
 			checkLocaleAttributes(httpRequest);
 			referenceMap.put(FilterUtil.LOCALE_ATTRIBUTE,
 			    httpRequest.getSession().getAttribute(FilterUtil.LOCALE_ATTRIBUTE));
-			log.info("Locale stored in session is " + httpRequest.getSession().getAttribute(FilterUtil.LOCALE_ATTRIBUTE));
+			log.info("Locale stored in session is "
+			        + OpenmrsLoggingUtil.sanitize(httpRequest.getSession().getAttribute(FilterUtil.LOCALE_ATTRIBUTE)));
 
 			httpResponse.setContentType("text/html");
 			// otherwise do step one of the wizard
@@ -976,7 +979,7 @@ private void checkLocaleAttributes(HttpServletRequest httpRequest) {
 			// if user has changed locale parameter to new one
 			// or chooses it parameter at first page loading
 			if (storedLocale == null || !storedLocale.equals(localeParameter)) {
-				log.info("Stored locale parameter to session " + localeParameter);
+				log.info("Stored locale parameter to session " + OpenmrsLoggingUtil.sanitize(localeParameter));
 				httpRequest.getSession().setAttribute(FilterUtil.LOCALE_ATTRIBUTE, localeParameter);
 			}
 			if (rememberLocale) {
@@ -1892,7 +1895,7 @@ public static String loadDriver(String connection, String databaseDriver) {
 		String loadedDriverString = null;
 		try {
 			loadedDriverString = DatabaseUtil.loadDatabaseDriver(connection, databaseDriver);
-			log.info("using database driver :" + loadedDriverString);
+			log.info("using database driver :" + OpenmrsLoggingUtil.sanitize(loadedDriverString));
 		} catch (ClassNotFoundException e) {
 			log.error("The given database driver class was not found. "
 			        + "Please ensure that the database driver jar file is on the class path "

web/src/main/java/org/openmrs/web/filter/initialization/TestInstallUtil.java

@@ -35,6 +35,7 @@
 import org.openmrs.api.APIAuthenticationException;
 import org.openmrs.api.APIException;
 import org.openmrs.api.context.Context;
+import org.openmrs.logging.OpenmrsLoggingUtil;
 import org.openmrs.module.ModuleConstants;
 import org.openmrs.util.OpenmrsConstants;
 import org.openmrs.util.OpenmrsUtil;
@@ -104,7 +105,7 @@ protected static boolean addTestData(String host, int port, String databaseName,
 
 			//print out the error messages from the process
 			if (StringUtils.isNotBlank(errorMsg)) {
-				log.error(errorMsg);
+				log.error("{}", OpenmrsLoggingUtil.sanitize(errorMsg));
 			}
 
 			if (proc.waitFor() == 0) {
@@ -147,7 +148,7 @@ protected static boolean addZippedTestModules(InputStream in) {
 			while (entries.hasMoreElements()) {
 				ZipEntry entry = (ZipEntry) entries.nextElement();
 				if (entry.isDirectory()) {
-					log.debug("Skipping directory: {}", entry.getName());
+					log.debug("Skipping directory: {}", OpenmrsLoggingUtil.sanitize(entry.getName()));
 					continue;
 				}
 
@@ -159,7 +160,7 @@ protected static boolean addZippedTestModules(InputStream in) {
 						fileName = new File(entry.getName()).getName();
 					}
 
-					log.debug("Extracting module file: {}", fileName);
+					log.debug("Extracting module file: {}", OpenmrsLoggingUtil.sanitize(fileName));
 
 					//use the module repository folder GP value if specified
 					String moduleRepositoryFolder = FilterUtil
@@ -191,7 +192,7 @@ protected static boolean addZippedTestModules(InputStream in) {
 					OpenmrsUtil.copyFile(zipFile.getInputStream(entry),
 					    new BufferedOutputStream(new FileOutputStream(zipEntryFile)));
 				} else {
-					log.debug("Ignoring file that is not a .omod '{}'", fileName);
+					log.debug("Ignoring file that is not a .omod '{}'", OpenmrsLoggingUtil.sanitize(fileName));
 				}
 			}
 		} catch (IOException e) {
@@ -255,7 +256,8 @@ protected static InputStream getResourceInputStream(String url, String openmrsUs
 		out.flush();
 		out.close();
 
-		log.info("Http response message: {}, Code: {}", connection.getResponseMessage(), connection.getResponseCode());
+		log.info("Http response message: {}, Code: {}", OpenmrsLoggingUtil.sanitize(connection.getResponseMessage()),
+		    connection.getResponseCode());
 
 		if (connection.getResponseCode() == HttpURLConnection.HTTP_UNAUTHORIZED) {
 			throw new APIAuthenticationException("Invalid username or password");

web/src/main/java/org/openmrs/web/filter/update/UpdateFilter.java

@@ -37,6 +37,7 @@
 import org.openmrs.liquibase.ChangeLogDetective;
 import org.openmrs.liquibase.ChangeLogVersionFinder;
 import org.openmrs.liquibase.ChangeSetExecutorCallback;
+import org.openmrs.logging.OpenmrsLoggingUtil;
 import org.openmrs.util.DatabaseUpdateException;
 import org.openmrs.util.DatabaseUpdater;
 import org.openmrs.util.DatabaseUpdaterLiquibaseProvider;
@@ -154,7 +155,7 @@ protected synchronized void doPost(HttpServletRequest httpRequest, HttpServletRe
 			String username = httpRequest.getParameter("username");
 			String password = httpRequest.getParameter("password");
 
-			log.debug("Attempting to authenticate user: " + username);
+			log.debug("Attempting to authenticate user: " + OpenmrsLoggingUtil.sanitize(username));
 			if (authenticateAsSuperUser(username, password)) {
 				log.debug("Authentication successful.  Redirecting to 'reviewupdates' page.");
 				// set a variable so we know that the user started here
@@ -292,13 +293,13 @@ public void checkLocaleAttributesForFirstTime(HttpServletRequest httpRequest) {
 		String systemDefaultLocale = FilterUtil.readSystemDefaultLocale(null);
 		if (CustomResourceLoader.getInstance(httpRequest).getAvailablelocales().contains(locale)) {
 			httpRequest.getSession().setAttribute(FilterUtil.LOCALE_ATTRIBUTE, locale.toString());
-			log.info("Used client's locale " + locale.toString());
+			log.info("Used client's locale " + OpenmrsLoggingUtil.sanitize(locale));
 		} else if (StringUtils.isNotBlank(systemDefaultLocale)) {
 			httpRequest.getSession().setAttribute(FilterUtil.LOCALE_ATTRIBUTE, systemDefaultLocale);
-			log.info("Used system default locale " + systemDefaultLocale);
+			log.info("Used system default locale " + OpenmrsLoggingUtil.sanitize(systemDefaultLocale));
 		} else {
 			httpRequest.getSession().setAttribute(FilterUtil.LOCALE_ATTRIBUTE, Locale.ENGLISH.toString());
-			log.info("Used default locale " + Locale.ENGLISH.toString());
+			log.info("Used default locale " + OpenmrsLoggingUtil.sanitize(Locale.ENGLISH));
 		}
 	}