CVE-2022-24830: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: OpenClinica is an open source software for Electronic Data Capture (EDC) and Clinical Data Management (CDM). OpenClinica prior to version 3.16 is vulnerable to path traversal in multiple endpoints, leading to arbitrary file read/write, and potential remote code execution. There are no known workarounds. This issue has been patched and users are recommended to upgrade.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 9.8
Published: 2022-05-14T00:15Z
Repository: https://github.com/OpenClinica/OpenClinica
Language: Java
Files changed: web/src/main/java/org/akaza/openclinica/control/admin/DownloadVersionSpreadSheetServlet.java, web/src/main/java/org/akaza/openclinica/control/submit/DownloadAttachedFileServlet.java, web/src/main/java/org/akaza/openclinica/controller/BatchCRFMigrationController.java, web/src/main/java/org/akaza/openclinica/controller/openrosa/OpenRosaSubmissionController.java
Commit date: 2022-02-03 18:05:59 -0500
Commit message: OC-17139: code  changes after code review

Diff: diff --git a/web/src/main/java/org/akaza/openclinica/control/admin/DownloadVersionSpreadSheetServlet.java b/web/src/main/java/org/akaza/openclinica/control/admin/DownloadVersionSpreadSheetServlet.java
index 0be56117d..23b43b895 100755
--- a/web/src/main/java/org/akaza/openclinica/control/admin/DownloadVersionSpreadSheetServlet.java
+++ b/web/src/main/java/org/akaza/openclinica/control/admin/DownloadVersionSpreadSheetServlet.java
@@ -93,18 +93,26 @@ public class DownloadVersionSpreadSheetServlet extends SecureController {
         } else {
         	// fix path traversal issue
             excelFile = new File(dir,excelFileName);
             // backwards compat
             File oldExcelFile = new File(dir, oldExcelFileName);            
-       
-        	if (oldExcelFile.exists() && oldExcelFile.length() > 0) {
-                 if (!excelFile.exists() || excelFile.length() <= 0) {
-                     // if the old name exists and the new name does not...
-                     excelFile = oldExcelFile;
-                     excelFileName = oldExcelFileName;
-                 }
-             }                  
+            
+            String canonicalPath1= excelFile.getCanonicalPath();
+            String canonicalPath2= oldExcelFile.getCanonicalPath();
+            if (canonicalPath1.startsWith(dir) && canonicalPath2.startsWith(dir)) {
+            	if (oldExcelFile.exists() && oldExcelFile.length() > 0) {
+                    if (!excelFile.exists() || excelFile.length() <= 0) {
+                        // if the old name exists and the new name does not...
+                        excelFile = oldExcelFile;
+                        excelFileName = oldExcelFileName;
+                    }
+                }     
+            }else {
+            	 addPageMessage(respage.getString("the_excel_is_not_available_on_server_contact"));
+                 forwardPage(Page.CRF_LIST_SERVLET);
+            }
+        	             
 
         }
         logger.info("looking for : " + excelFile.getName());
         if (!excelFile.exists() || excelFile.length() <= 0) {
             addPageMessage(respage.getString("the_excel_is_not_available_on_server_contact"));
diff --git a/web/src/main/java/org/akaza/openclinica/control/submit/DownloadAttachedFileServlet.java b/web/src/main/java/org/akaza/openclinica/control/submit/DownloadAttachedFileServlet.java
index 8a710adf2..afcff9b30 100644
--- a/web/src/main/java/org/akaza/openclinica/control/submit/DownloadAttachedFileServlet.java
+++ b/web/src/main/java/org/akaza/openclinica/control/submit/DownloadAttachedFileServlet.java
@@ -68,21 +68,27 @@ public class DownloadAttachedFileServlet extends SecureController {
     public void processRequest() throws Exception {
         FormProcessor fp = new FormProcessor(request);
         String filePathName = "";
         String fileName = fp.getString("fileName");
         File f = new File(fileName);
-        
-        if(fileName != null && fileName.indexOf("..") > -1) {
-        	throw new RuntimeException("Traversal attempt - absolute path not allowed " + fileName);
-        }
-        
+              
         if (fileName != null && fileName.length() > 0) {
-            int parentStudyId = currentStudy.getParentStudyId();
+            int parentStudyId = currentStudy.getParentStudyId();           
             String testPath = Utils.getAttachedFileRootPath();
             String tail = File.separator + f.getName();
             String testName = testPath + currentStudy.getOid() + tail;
-            File temp = new File(testName);
+            
+            String filePath = testPath + currentStudy.getOid() +File.separator;            
+            File temp = new File(filePath,f.getName());            
+            String canonicalPath= temp.getCanonicalPath();
+            
+            if (canonicalPath.startsWith(filePath)) {
+            	;
+            }else {
+            	throw new RuntimeException("Traversal attempt - file path not allowed " + fileName);
+            }
+            
             if (temp.exists()) {
                 filePathName = testName;
                 logger.info(currentStudy.getName() + " existing filePathName=" + filePathName);
             } else {
                 if (currentStudy.isSite(parentStudyId)) {
diff --git a/web/src/main/java/org/akaza/openclinica/controller/BatchCRFMigrationController.java b/web/src/main/java/org/akaza/openclinica/controller/BatchCRFMigrationController.java
index 50b1d264c..eaf120942 100644
--- a/web/src/main/java/org/akaza/openclinica/controller/BatchCRFMigrationController.java
+++ b/web/src/main/java/org/akaza/openclinica/controller/BatchCRFMigrationController.java
@@ -125,17 +125,24 @@ public class BatchCRFMigrationController implements Runnable {
     @RequestMapping(value = "/forms/migrate/{filename}/downloadLogFile")
     public void getLogFile(@PathVariable("filename") String fileName, HttpServletResponse response) throws Exception {
         InputStream inputStream = null;
         try {
         	//Validate/Sanitize user input filename using a standard library, prevent from path traversal 
-            String logFileName = getFilePath() + File.separator + FilenameUtils.getName(fileName);
-            File fileToDownload = new File(logFileName);
-            inputStream = new FileInputStream(fileToDownload);
-            response.setContentType("application/force-download");
-            response.setHeader("Content-Disposition", "attachment; filename=" + fileName);
-            IOUtils.copy(inputStream, response.getOutputStream());
-            response.flushBuffer();
+            String logFilePath = getFilePath() + File.separator;
+            File fileToDownload = new File(logFilePath, fileName);                    
+            String canonicalPath= fileToDownload.getCanonicalPath();
+            
+            if (canonicalPath.startsWith(logFilePath)) {
+            	inputStream = new FileInputStream(fileToDownload);
+                response.setContentType("application/force-download");
+                response.setHeader("Content-Disposition", "attachment; filename=" + fileName);
+                IOUtils.copy(inputStream, response.getOutputStream());
+                response.flushBuffer();
+            }else {
+            	throw new RuntimeException("Traversal attempt - file path not allowed " + fileName);
+            }
+            
         } catch (Exception e) {
             logger.debug("Request could not be completed at this moment. Please try again.");
             logger.debug(e.getStackTrace().toString());
             throw e;
         } finally {
diff --git a/web/src/main/java/org/akaza/openclinica/controller/openrosa/OpenRosaSubmissionController.java b/web/src/main/java/org/akaza/openclinica/controller/openrosa/OpenRosaSubmissionController.java
index b2d2d9236..d68eb7ed3 100644
--- a/web/src/main/java/org/akaza/openclinica/controller/openrosa/OpenRosaSubmissionController.java
+++ b/web/src/main/java/org/akaza/openclinica/controller/openrosa/OpenRosaSubmissionController.java
@@ -25,11 +25,10 @@ import org.akaza.openclinica.i18n.core.LocaleResolver;
 import org.akaza.openclinica.service.pmanage.ParticipantPortalRegistrar;
 import org.akaza.openclinica.web.pform.PFormCache;
 import org.apache.commons.fileupload.FileItem;
 import org.apache.commons.fileupload.disk.DiskFileItemFactory;
 import org.apache.commons.fileupload.servlet.ServletFileUpload;
-import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.exception.ExceptionUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -192,20 +191,29 @@ public class OpenRosaSubmissionController {
                 accessPermission = true;
         }
         return accessPermission;
     }
 
-    public static String getAttachedFilePath(String inputStudyOid) {
-    	// Using a standard library to validate/Sanitize user inputs which will be used in path expression to prevent from path traversal
-    	String studyOid =  FilenameUtils.getName(inputStudyOid);
+    public static String getAttachedFilePath(String studyOid) throws Exception {    	
         String attachedFilePath = CoreResources.getField("attached_file_location");
         if (attachedFilePath == null || attachedFilePath.length() <= 0) {
-            attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator + studyOid + File.separator;
-        } else {
-            attachedFilePath += studyOid + File.separator;
+            attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator;
+        }
+        File tempFile =  new File(attachedFilePath,studyOid);
+        String canonicalPath= tempFile.getCanonicalPath();
+        
+        if (canonicalPath.startsWith(attachedFilePath)) {
+        	if (attachedFilePath == null || attachedFilePath.length() <= 0) {
+                attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator + studyOid + File.separator;
+            } else {
+                attachedFilePath += studyOid + File.separator;
+            }
+            return attachedFilePath;
+        }else {
+        	throw new RuntimeException("Traversal attempt - file path not allowed " + studyOid);
         }
-        return attachedFilePath;
+        
     }
 
     private File processUploadedFile(FileItem item, String dirToSaveUploadedFileIn) {
         dirToSaveUploadedFileIn = dirToSaveUploadedFileIn == null ? System.getProperty("java.io.tmpdir") : dirToSaveUploadedFileIn;
         String fileName = item.getName();

Vulnerable code:        
        	if (oldExcelFile.exists() && oldExcelFile.length() > 0) {
                 if (!excelFile.exists() || excelFile.length() <= 0) {
                     // if the old name exists and the new name does not...
                     excelFile = oldExcelFile;
                     excelFileName = oldExcelFileName;
                 }
             }                  
        
        if(fileName != null && fileName.indexOf("..") > -1) {
        	throw new RuntimeException("Traversal attempt - absolute path not allowed " + fileName);
        }
        
            int parentStudyId = currentStudy.getParentStudyId();
            File temp = new File(testName);
            String logFileName = getFilePath() + File.separator + FilenameUtils.getName(fileName);
            File fileToDownload = new File(logFileName);
            inputStream = new FileInputStream(fileToDownload);
            response.setContentType("application/force-download");
            response.setHeader("Content-Disposition", "attachment; filename=" + fileName);
            IOUtils.copy(inputStream, response.getOutputStream());
            response.flushBuffer();
import org.apache.commons.io.FilenameUtils;
    public static String getAttachedFilePath(String inputStudyOid) {
    	// Using a standard library to validate/Sanitize user inputs which will be used in path expression to prevent from path traversal
    	String studyOid =  FilenameUtils.getName(inputStudyOid);
            attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator + studyOid + File.separator;
        } else {
            attachedFilePath += studyOid + File.separator;
        return attachedFilePath;
Fixed code:             
            String canonicalPath1= excelFile.getCanonicalPath();
            String canonicalPath2= oldExcelFile.getCanonicalPath();
            if (canonicalPath1.startsWith(dir) && canonicalPath2.startsWith(dir)) {
            	if (oldExcelFile.exists() && oldExcelFile.length() > 0) {
                    if (!excelFile.exists() || excelFile.length() <= 0) {
                        // if the old name exists and the new name does not...
                        excelFile = oldExcelFile;
                        excelFileName = oldExcelFileName;
                    }
                }     
            }else {
            	 addPageMessage(respage.getString("the_excel_is_not_available_on_server_contact"));
                 forwardPage(Page.CRF_LIST_SERVLET);
            }
        	             
              
            int parentStudyId = currentStudy.getParentStudyId();           
            
            String filePath = testPath + currentStudy.getOid() +File.separator;            
            File temp = new File(filePath,f.getName());            
            String canonicalPath= temp.getCanonicalPath();
            
            if (canonicalPath.startsWith(filePath)) {
            	;
            }else {
            	throw new RuntimeException("Traversal attempt - file path not allowed " + fileName);
            }
            
            String logFilePath = getFilePath() + File.separator;
            File fileToDownload = new File(logFilePath, fileName);                    
            String canonicalPath= fileToDownload.getCanonicalPath();
            
            if (canonicalPath.startsWith(logFilePath)) {
            	inputStream = new FileInputStream(fileToDownload);
                response.setContentType("application/force-download");
                response.setHeader("Content-Disposition", "attachment; filename=" + fileName);
                IOUtils.copy(inputStream, response.getOutputStream());
                response.flushBuffer();
            }else {
            	throw new RuntimeException("Traversal attempt - file path not allowed " + fileName);
            }
            
    public static String getAttachedFilePath(String studyOid) throws Exception {    	
            attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator;
        }
        File tempFile =  new File(attachedFilePath,studyOid);
        String canonicalPath= tempFile.getCanonicalPath();
        
        if (canonicalPath.startsWith(attachedFilePath)) {
        	if (attachedFilePath == null || attachedFilePath.length() <= 0) {
                attachedFilePath = CoreResources.getField("filePath") + "attached_files" + File.separator + studyOid + File.separator;
            } else {
                attachedFilePath += studyOid + File.separator;
            }
            return attachedFilePath;
        }else {
        	throw new RuntimeException("Traversal attempt - file path not allowed " + studyOid);