BAH-4991 Remediate Security Findings in Bahmni-core

admin/src/main/java/org/bahmni/module/admin/csv/service/PatientMatchService.java

@@ -16,6 +16,8 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -25,7 +27,7 @@
     @Autowired
     private BahmniPatientService patientService;
 
-    private static final String PATIENT_MATCHING_ALGORITHM_DIRECTORY = "/patientMatchingAlgorithm/";
+    private static final String PATIENT_MATCHING_ALGORITHM_DIRECTORY_NAME = "patientMatchingAlgorithm";
     private static final Logger log = LogManager.getLogger(PatientMatchService.class);
 
     // Mujir - an implementation could use multiple patient matching algorithms
@@ -56,7 +58,13 @@
     }
 
     private String getAlgorithmClassPath(String matchingAlgorithmClassName) {
-        return OpenmrsUtil.getApplicationDataDirectory() + PATIENT_MATCHING_ALGORITHM_DIRECTORY + matchingAlgorithmClassName;
+        Path baseDir = Paths.get(OpenmrsUtil.getApplicationDataDirectory(), PATIENT_MATCHING_ALGORITHM_DIRECTORY_NAME).normalize();
+        Path resolvedPath = baseDir.resolve(matchingAlgorithmClassName).normalize();
+        if (!resolvedPath.startsWith(baseDir)) {
+            log.error("Rejected unsafe patientMatchingAlgorithm value: " + matchingAlgorithmClassName);
+            throw new IllegalArgumentException("Invalid patientMatchingAlgorithm: " + matchingAlgorithmClassName);
+        }
+        return resolvedPath.toString();
     }
 
 }

admin/src/test/java/org/bahmni/module/admin/csv/service/PatientMatchServiceTest.java

@@ -0,0 +1,80 @@
+package org.bahmni.module.admin.csv.service;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.File;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+import static org.junit.Assert.assertEquals;
+
+public class PatientMatchServiceTest {
+
+    private static final String APP_DATA_DIR_PROPERTY = "OPENMRS_APPLICATION_DATA_DIRECTORY";
+    private static final String ALGORITHM_DIR_NAME = "patientMatchingAlgorithm";
+
+    @Rule
+    public TemporaryFolder appDataDir = new TemporaryFolder();
+
+    @Rule
+    public ExpectedException expectedEx = ExpectedException.none();
+
+    private String originalAppDataDirProperty;
+    private Method getAlgorithmClassPath;
+
+    @Before
+    public void setUp() throws Exception {
+        originalAppDataDirProperty = System.getProperty(APP_DATA_DIR_PROPERTY);
+        System.setProperty(APP_DATA_DIR_PROPERTY, appDataDir.getRoot().getAbsolutePath());
+        appDataDir.newFolder(ALGORITHM_DIR_NAME);
+
+        getAlgorithmClassPath = PatientMatchService.class.getDeclaredMethod("getAlgorithmClassPath", String.class);
+        getAlgorithmClassPath.setAccessible(true);
+    }
+
+    @After
+    public void tearDown() {
+        if (originalAppDataDirProperty == null) {
+            System.clearProperty(APP_DATA_DIR_PROPERTY);
+        } else {
+            System.setProperty(APP_DATA_DIR_PROPERTY, originalAppDataDirProperty);
+        }
+    }
+
+    @Test
+    public void shouldResolveASimpleNameInsideTheAlgorithmDirectory() throws Exception {
+        String resolvedPath = invoke("SimpleAlgorithm.groovy");
+        String expectedPath = new File(new File(appDataDir.getRoot(), ALGORITHM_DIR_NAME), "SimpleAlgorithm.groovy").getPath();
+        assertEquals(expectedPath, resolvedPath);
+    }
+
+    @Test
+    public void shouldRejectRelativePathTraversalOutsideTheAlgorithmDirectory() throws Exception {
+        expectedEx.expect(IllegalArgumentException.class);
+        expectedEx.expectMessage("Invalid patientMatchingAlgorithm");
+        invoke("../../../../tmp/evil/Pwn.groovy");
+    }
+
+    @Test
+    public void shouldRejectAbsolutePathPayload() throws Exception {
+        expectedEx.expect(IllegalArgumentException.class);
+        expectedEx.expectMessage("Invalid patientMatchingAlgorithm");
+        invoke("/etc/passwd");
+    }
+
+    private String invoke(String matchingAlgorithmClassName) throws Exception {
+        try {
+            return (String) getAlgorithmClassPath.invoke(new PatientMatchService(), matchingAlgorithmClassName);
+        } catch (InvocationTargetException e) {
+            if (e.getCause() instanceof RuntimeException) {
+                throw (RuntimeException) e.getCause();
+            }
+            throw e;
+        }
+    }
+}

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImpl.java

@@ -301,14 +301,15 @@ public List<Obs> getObsForFormBuilderForms(String patientUuid, List<String> form
                 "JOIN encounter ON encounter.encounter_id = obs.encounter_id AND encounter.voided = 0 " +
                 encounterFilter +
                 "JOIN visit ON visit.visit_id = encounter.visit_id AND visit.visit_id IN :visitIds ");
-        queryString.append(String.format("where obs.form_namespace_and_path REGEXP '%s' ", commaSeparatedFormNamesPattern(formNames)));
+        queryString.append("where obs.form_namespace_and_path REGEXP :formNamesPattern ");
         if (startDate != null) queryString.append("and obs.obs_datetime >= :startDate ");
         if (startDate != null && endDate != null) queryString.append("and obs.obs_datetime <= :endDate ");
         queryString.append("order by obs_datetime asc ");
         Query queryToGetObs = sessionFactory.getCurrentSession()
                 .createSQLQuery(queryString.toString()).addEntity(Obs.class);
         queryToGetObs.setParameter("patientUuid", patientUuid);
         queryToGetObs.setParameterList("visitIds", listOfVisitIds);
+        queryToGetObs.setParameter("formNamesPattern", commaSeparatedFormNamesPattern(formNames));
         if (nonNull(startDate))
             queryToGetObs.setParameter("startDate", new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(startDate));
         if (nonNull(endDate))
@@ -319,10 +320,14 @@ public List<Obs> getObsForFormBuilderForms(String patientUuid, List<String> form
 
     private String commaSeparatedFormNamesPattern(List<String> formNames) {
         ArrayList<String> formPatterns = new ArrayList<>();
-        formNames.forEach(form -> formPatterns.add("\\\\^" + form + "\\\\."));
+        formNames.forEach(form -> formPatterns.add("\\^" + escapeRegexMetacharacters(form) + "\\."));
         return StringUtils.join(formPatterns, OR);
     }
 
+    private String escapeRegexMetacharacters(String value) {
+        return value.replaceAll("([\\\\^$.|?*+()\\[\\]{}])", "\\\\$1");
+    }
+
     private String commaSeparatedEncounterIds(Collection<Encounter> encounters) {
         ArrayList<String> encounterIds = new ArrayList<>();
         for (Encounter encounter : encounters) {
@@ -362,10 +367,10 @@ public List<Obs> getObsByPatientProgramUuidAndConceptNames(String patientProgram
                 "AND cn.concept_name_type='FULLY_SPECIFIED' " +
                 "AND cn.name IN (:conceptNames) " +
                 "AND cn.locale = :locale");
-        if(null != startDate) {
+        if (null != startDate) {
             queryString.append(" AND o.obs_datetime >= STR_TO_DATE(:startDate, '%Y-%m-%d')");
         }
-        if(null != endDate) {
+        if (null != endDate) {
             queryString.append(" AND o.obs_datetime <= STR_TO_DATE(:endDate, '%Y-%m-%d')");
         }
         if (sortOrder == OrderBy.ASC) {
@@ -380,10 +385,10 @@ public List<Obs> getObsByPatientProgramUuidAndConceptNames(String patientProgram
         queryToGetObs.setParameterList("conceptNames", conceptNames);
         queryToGetObs.setString("patientProgramUuid", patientProgramUuid);
         queryToGetObs.setString("locale", Context.getLocale().getLanguage());
-        if(null != startDate) {
+        if (null != startDate) {
             queryToGetObs.setString("startDate", dateFormat.format(startDate));
         }
-        if(null != endDate) {
+        if (null != endDate) {
             queryToGetObs.setString("endDate", dateFormat.format(endDate));
         }
 

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/extensions/BahmniExtensions.java

@@ -1,6 +1,7 @@
 package org.bahmni.module.bahmnicore.extensions;
 
 import groovy.lang.GroovyClassLoader;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.bahmni.module.bahmnicore.dao.ApplicationDataDirectory;
@@ -9,6 +10,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
 
 @Component
 public class BahmniExtensions {
@@ -26,8 +28,23 @@
     }
 
     public Object getExtension(String directory, String fileName) {
-        File groovyFile = applicationDataDirectory
-                .getFileFromConfig("openmrs" + File.separator + directory + File.separator + fileName);
+        if (StringUtils.isBlank(fileName)) {
+            log.error("Extension file name is required for directory " + directory);
+            return null;
+        }
+
+        Path extensionDir = applicationDataDirectory
+                .getFileFromConfig("openmrs" + File.separator + directory).toPath().normalize();
+        Path groovyFilePath = applicationDataDirectory
+                .getFileFromConfig("openmrs" + File.separator + directory + File.separator + fileName)
+                .toPath().normalize();
+
+        if (!groovyFilePath.startsWith(extensionDir)) {
+            log.error("Rejected extension file resolving outside of the allowed directory for extension type " + directory);
+            return null;
+        }
+
+        File groovyFile = groovyFilePath.toFile();
         if (!groovyFile.exists()) {
             log.error("File not found " + groovyFile.getAbsolutePath());
         } else {

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniEncounterModifierServiceImpl.java

@@ -14,6 +14,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -57,8 +58,16 @@ private EncounterModifier loadGroovyClass(String encounterModifierClassName) thr
         return (EncounterModifier) clazz.newInstance();
     }
 
-    private String getEncounterModifierClassPath(String encounterModifierClassName) {
-        return OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY + encounterModifierClassName ;
+    private String getEncounterModifierClassPath(String encounterModifierClassName) throws IOException {
+        Path encounterModifierDirectory = new File(OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY)
+                .toPath().normalize();
+        Path encounterModifierClassFile = new File(OpenmrsUtil.getApplicationDataDirectory() + ENCOUNTER_MODIFIER_ALGORITHM_DIRECTORY + encounterModifierClassName)
+                .toPath().normalize();
+
+        if (!encounterModifierClassFile.startsWith(encounterModifierDirectory)) {
+            throw new IOException("Rejected encounter modifier class name resolving outside of the allowed directory: " + encounterModifierClassName);
+        }
+        return encounterModifierClassFile.toString();
     }
 
     private boolean isGroovyCachingAllowed(){

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/PatientDocumentServiceImpl.java

@@ -101,12 +101,19 @@ private String getBasePath() {
         return BahmniCoreProperties.getProperty("bahmnicore.documents.baseDirectory");
     }
 
+    private String sanitizePathComponent(String value) {
+        if (value == null) return null;
+        return value.replaceAll("[/\\\\]", "").replace("..", "");
+    }
+
     private String createFileName(Integer patientId, String encounterTypeName, Object format, String originalFileName) {
         String uuid = UUID.randomUUID().toString();
+        String safeEncounterTypeName = sanitizePathComponent(encounterTypeName);
+        String safeFormat = sanitizePathComponent(String.valueOf(format));
         if (StringUtils.isNotBlank(originalFileName)) {
             originalFileName = "__" + originalFileName;
         }
-        return String.format("%s-%s-%s%s.%s", patientId, encounterTypeName, uuid, originalFileName, format);
+        return String.format("%s-%s-%s%s.%s", patientId, safeEncounterTypeName, uuid, originalFileName, safeFormat);
     }
 
     protected String createFilePath(String basePath, Integer patientId, String encounterTypeName, String format, String originalFileName) {
@@ -253,15 +260,26 @@ private void deleteFile(File file) {
     }
 
     private File getPatientImageFile(String patientUuid) {
-        File file = new File(String.format("%s/%s.%s", BahmniCoreProperties.getProperty("bahmnicore.images.directory"), patientUuid, patientImagesFormat));
-        if (file.exists() && file.isFile()) {
+        File file = resolveContainedImageFile(patientUuid);
+        if (file != null && file.exists() && file.isFile()) {
             return file;
         }
         return new File(BahmniCoreProperties.getProperty("bahmnicore.images.directory.defaultImage"));
     }
 
     private File getPatientImageFileWithoutDefault(String patientUuid) {
-        return new File(String.format("%s/%s.%s", BahmniCoreProperties.getProperty("bahmnicore.images.directory"), patientUuid, patientImagesFormat));
+        File file = resolveContainedImageFile(patientUuid);
+        return file != null ? file : new File("");
+    }
+
+    private File resolveContainedImageFile(String patientUuid) {
+        String imagesDir = BahmniCoreProperties.getProperty("bahmnicore.images.directory");
+        Path base = Paths.get(imagesDir).normalize();
+        Path candidate = Paths.get(imagesDir, String.format("%s.%s", patientUuid, patientImagesFormat)).normalize();
+        if (!candidate.startsWith(base)) {
+            return null;
+        }
+        return candidate.toFile();
     }
 
     private ResponseEntity<Object> readImage(File file) {

bahmnicore-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/ObsDaoImplIT.java

@@ -36,6 +36,7 @@
     public void setUp() throws Exception {
         executeDataSet("obsTestData.xml");
         executeDataSet("patientProgramTestData.xml");
+        executeDataSet("formBuilderFormNamesTestData.xml");
 
         conceptToObsMap.put(9012, 5);
         conceptToObsMap.put(9011, 4);
@@ -178,4 +179,46 @@
         assertEquals("2015-08-18 15:09:05.0", observations.get(0).getObsDatetime().toString());
         assertEquals("2016-08-18 15:09:05.0", observations.get(1).getObsDatetime().toString());
     }
+
+    @Test
+    public void shouldReturnObsMatchingBenignFormNamesFilter() throws Exception {
+        String patientUuid = "86526ed5-3c11-11de-a0ba-001e378eb67a";
+        List<Integer> listOfVisitIds = Collections.singletonList(902);
+
+        List<Obs> observations = obsDao.getObsForFormBuilderForms(
+                patientUuid, Arrays.asList("Vitals"), listOfVisitIds,
+                Collections.EMPTY_LIST, null, null);
+
+        assertEquals(1, observations.size());
+        assertEquals("Bahmni^Vitals.1/5-0", observations.get(0).getFormNamespaceAndPath());
+
+        List<Obs> bloodSampleObs = obsDao.getObsForFormBuilderForms(
+                patientUuid, Arrays.asList("BloodSample"), listOfVisitIds,
+                Collections.EMPTY_LIST, null, null);
+        assertEquals(1, bloodSampleObs.size());
+        assertEquals("Bahmni^BloodSample.2/1-0", bloodSampleObs.get(0).getFormNamespaceAndPath());
+
+        List<Obs> bothForms = obsDao.getObsForFormBuilderForms(
+                patientUuid, Arrays.asList("Vitals", "BloodSample"), listOfVisitIds,
+                Collections.EMPTY_LIST, null, null);
+        assertEquals(2, bothForms.size());
+    }
+
+    @Test
+    public void shouldTreatMaliciousFormNamesPayloadAsInertDataNotSql() throws Exception {
+        String patientUuid = "86526ed5-3c11-11de-a0ba-001e378eb67a";
+        List<Integer> listOfVisitIds = Collections.singletonList(902);
+
+        String maliciousPayload = "x' OR '1'='1' -- ";
+        List<Obs> observations = obsDao.getObsForFormBuilderForms(
+                patientUuid, Arrays.asList(maliciousPayload), listOfVisitIds,
+                Collections.EMPTY_LIST, null, null);
+        assertEquals(0, observations.size());
+
+        String unionPayload = "x' UNION SELECT username, password, salt FROM users -- ";
+        List<Obs> unionAttempt = obsDao.getObsForFormBuilderForms(
+                patientUuid, Arrays.asList(unionPayload), listOfVisitIds,
+                Collections.EMPTY_LIST, null, null);
+        assertEquals(0, unionAttempt.size());
+    }
 }