Fixes done for security vulnerabilities (#50)

* [Rohit|Tazeen] Allows pdf, image, doc type file uploads. Restricts other file types and double extensions * [Rohit] Removes multiple error messages for invalid login, kept only one * [Rohit|Tazeen] Strips stored xss from imput fields * [Rohit|Tazeen] Adds server side validation to restricts file uploads * Update README.md --------- Co-authored-by: rohit-yawalkar <rohit.yawalkar@thoughtworks.com> Co-authored-by: SanoferSameera <79590907+SanoferSameera@users.noreply.github.com>
Bahmni · Nov 29, 2023 · 9515a14 · 9515a14
1 parent 4ba28d5
commit 9515a14
Show file tree

Hide file tree

Showing 7 changed files with 98 additions and 37 deletions.
diff --git a/openelis/WebContent/pages/result/resultListView.jsp b/openelis/WebContent/pages/result/resultListView.jsp
@@ -1114,7 +1114,7 @@ function /*void*/ processTestReflexCD4Success(xhr)
 			<html:hidden property="hideShowFlag"  styleId='<%="hideShow_" + index %>' value="hidden" />
 		</td>
         <td>
-            <input type="file" name='<%="testResult["+index+"].uploadedFile"%>' onchange='<%="markUpdated(" + index + ");"%>'>
+            <input type="file" id="<%= index %>" name='<%="testResult["+index+"].uploadedFile"%>' accept=".pdf,.doc,.docx,.jpg,.jpeg,.png">
             <% if(testResult.getUploadedFileName() != null){ %>
                     <%  String filePath = testResult.getUploadedFileName();
                         String fileNameWithUUID = filePath.substring(filePath.lastIndexOf("/") + 1);
@@ -1215,3 +1215,20 @@ function /*void*/ processTestReflexCD4Success(xhr)
 <logic:equal name="testCount"  value="0">
 <h2><bean:message key="result.noTestsFound"/></h2>
 </logic:equal>
+
+<script>
+( function($) {
+    $(document).ready(function() {
+        $('input[type="file"]').change(function(e){
+            var fileName = e.target.files[0].name;
+            var index = e.target.id;
+            if(fileName.split('.').includes('exe')){
+                e.target.value = '';
+                alert("file type not supported")
+            } else {
+                markUpdated(index);
+            }
+        });
+    });
+} ) ( jQuery );
+</script>
diff --git a/openelis/src/us/mn/state/health/lims/common/util/SafeRequest.java b/openelis/src/us/mn/state/health/lims/common/util/SafeRequest.java
@@ -1,57 +1,83 @@
 package us.mn.state.health.lims.common.util;
 
+
+import java.util.regex.Pattern;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
 
 public class SafeRequest extends HttpServletRequestWrapper {
+
     private final Logger logger = LogManager.getLogger(SafeRequest.class);
     private List<String> ignoreEncodingForParams = Arrays.asList("sampleXML");
 
-    public SafeRequest(HttpServletRequest request) {
-        super(request);
+    private static Pattern[] patterns = new Pattern[]{
+            // Script fragments
+            Pattern.compile("<script>(.*?)</script>", Pattern.CASE_INSENSITIVE),
+            // src='...'
+            Pattern.compile("src[\r\n]*=[\r\n]*\\\'(.*?)\\\'", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL),
+            Pattern.compile("src[\r\n]*=[\r\n]*\\\"(.*?)\\\"", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL),
+            // lonely script tags
+            Pattern.compile("</script>", Pattern.CASE_INSENSITIVE),
+            Pattern.compile("<script(.*?)>", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL),
+            // eval(...)
+            Pattern.compile("eval\\((.*?)\\)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL),
+            // expression(...)
+            Pattern.compile("expression\\((.*?)\\)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL),
+            // javascript:...
+            Pattern.compile("javascript:", Pattern.CASE_INSENSITIVE),
+            // vbscript:...
+            Pattern.compile("vbscript:", Pattern.CASE_INSENSITIVE),
+            // onload(...)=...
+            Pattern.compile("onload(.*?)=", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE | Pattern.DOTALL)
+    };
+
+    public SafeRequest(HttpServletRequest servletRequest) {
+        super(servletRequest);
     }
 
     @Override
-    public String getParameter(String name) {
-        HttpServletRequest request = (HttpServletRequest) this.getRequest();
-        String encodedValue = getEncodedParamValue(name, request.getParameter(name));
-        logger.debug(String.format("Intercepted action request: %s, name: %s, value= %s", request.getServletPath(), name, encodedValue));
-        return encodedValue;
-    }
+    public String[] getParameterValues(String parameter) {
+        String[] values = super.getParameterValues(parameter);
 
-    private String getEncodedParamValue(String name, String value) {
-        if (ignoreEncodingForParams.contains(name)) {
-            return value;
+        if (values == null) {
+            return null;
         }
-        return StringUtil.encode(value);
+
+        int count = values.length;
+        String[] encodedValues = new String[count];
+        for (int i = 0; i < count; i++) {
+            encodedValues[i] = stripXSS(values[i]);
+        }
+
+        return encodedValues;
     }
 
     @Override
-    public Map<String, String[]> getParameterMap() {
-        logger.debug("getParameterMap");
-        Map<String, String[]> newParameterMap = new HashMap<>();
-        Map<String, String[]> existingParameterMap = super.getParameterMap();
-        for (String key : existingParameterMap.keySet()) {
-            newParameterMap.put(key, getParameterValues(key));
-        }
-        return newParameterMap;
+    public String getParameter(String parameter) {
+        String value = super.getParameter(parameter);
+
+        return stripXSS(value);
     }
 
     @Override
-    public String[] getParameterValues(String name) {
-        logger.debug("getParameterValues");
-        String[] existingValues = super.getParameterValues(name);
-        String[] newValues = new String[existingValues.length];
-        for (int i = 0; i < existingValues.length; i++) {
-            newValues[i] = getEncodedParamValue(name, existingValues[i]);
-            logger.debug(String.format("Encoded param name: %s, value= %s", name, name, newValues[i]));
+    public String getHeader(String name) {
+        String value = super.getHeader(name);
+        return stripXSS(value);
+    }
+
+    private String stripXSS(String value) {
+        if (value != null) {
+
+            // Avoid null characters
+            value = value.replaceAll("\0", "");
+
+            // Remove all sections that match a pattern
+            for (Pattern scriptPattern : patterns){
+                value = scriptPattern.matcher(value).replaceAll("");
+            }
         }
-        return newValues;
+        return value;
     }
-}
+}
diff --git a/openelis/src/us/mn/state/health/lims/login/action/LoginValidateAction.java b/openelis/src/us/mn/state/health/lims/login/action/LoginValidateAction.java
@@ -149,7 +149,7 @@ protected ActionForward performAction(ActionMapping mapping,
 				}
 
 				errors = new ActionMessages();		
-				ActionError error = new ActionError("login.error.attempt.message",
+				ActionError error = new ActionError("login.error.message",
 													String.valueOf(loginUserFailAttemptCount),
 													String.valueOf(loginUserFailAttemptCountDefault), 
 													SystemConfiguration.getInstance().getLoginUserAccountUnlockMinute(), null);			

diff --git a/openelis/src/us/mn/state/health/lims/referral/action/ReferredOutUpdateAction.java b/openelis/src/us/mn/state/health/lims/referral/action/ReferredOutUpdateAction.java
@@ -421,6 +421,8 @@ private void saveUploadedFile(IReferralResultTest referralItem) {
             referralItem.setUploadedFileName(encodedFileName);
         } catch (IOException | URISyntaxException e) {
             referralItem.setUploadedFileName(null);
+        } catch (Exception e) {
+            e.printStackTrace();
         }
     }
 

diff --git a/openelis/src/us/mn/state/health/lims/result/action/ResultsLogbookUpdateAction.java b/openelis/src/us/mn/state/health/lims/result/action/ResultsLogbookUpdateAction.java
@@ -511,6 +511,8 @@ private void createFileForResult(TestResultItem testResultItem) {
         } catch (URISyntaxException e) {
             testResultItem.setUploadedFileName(null);
             e.printStackTrace();
+        } catch (Exception e) {
+            e.printStackTrace();
         }
     }
 

diff --git a/openelis/src/us/mn/state/health/lims/result/dao/ResultFileUploadDao.java b/openelis/src/us/mn/state/health/lims/result/dao/ResultFileUploadDao.java
@@ -6,5 +6,5 @@
 import java.net.URISyntaxException;
 
 public interface ResultFileUploadDao {
-    String upload(FormFile file) throws IOException, URISyntaxException;
+    String upload(FormFile file) throws IOException, URISyntaxException, Exception;
 }
diff --git a/openelis/src/us/mn/state/health/lims/result/daoimpl/ResultFileUploadDaoImpl.java b/openelis/src/us/mn/state/health/lims/result/daoimpl/ResultFileUploadDaoImpl.java
@@ -10,6 +10,8 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.UUID;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 public class ResultFileUploadDaoImpl implements ResultFileUploadDao {
     public static final String SEPARATOR = "_";
@@ -18,9 +20,12 @@ public class ResultFileUploadDaoImpl implements ResultFileUploadDao {
 
 
     @Override
-    public String upload(FormFile file) throws IOException, URISyntaxException {
+    public String upload(FormFile file) throws Exception {
         String encodedFileName = null;
         if(file != null && file.getFileName() != ""){
+            if(!validateFile(file.getFileName())) {
+                throw new Exception("file format not matching");
+            }
             String uuid = UUID.randomUUID().toString();
             String fileName = uuid + SEPARATOR + file.getFileName();
             encodedFileName = new URI(null, null, fileName, null).toString();
@@ -30,6 +35,15 @@ public String upload(FormFile file) throws IOException, URISyntaxException {
         return encodedFileName;
     }
 
+    private boolean validateFile(String fileName){
+        Pattern fileExtnPtrn = Pattern.compile("(^.((?!exe).)*\\.(jpg|JPG|jpeg|JPEG|doc|DOC|docx|DOCX|pdf|PDF|png|PNG)$)");
+        Matcher matcher = fileExtnPtrn.matcher(fileName);
+        if(matcher.matches()){
+            return true;
+        }
+        return false;
+    }
+
     private File getFile(String fileName) {
         String parentForUploadedFilesDirectory = new SiteInformationDAOImpl().getSiteInformationByName(PARENT_OF_UPLOADED_FILES_DIRECTORY).getValue();
         String uploadedFilesDirectory = new SiteInformationDAOImpl().getSiteInformationByName(UPLOADED_RESULTS_DIRECTORY).getValue();