From 961d76938650caa50ac422b7ebbcfbab8866d09d Mon Sep 17 00:00:00 2001 From: swatigogia2020 <73937345+swatigogia2020@users.noreply.github.com> Date: Wed, 29 Nov 2023 16:50:59 +0530 Subject: [PATCH] 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 Co-authored-by: SanoferSameera <79590907+SanoferSameera@users.noreply.github.com> --- README.md | 4 + .../pages/result/resultListView.jsp | 19 +++- .../health/lims/common/util/SafeRequest.java | 92 ++++++++++++------- .../login/action/LoginValidateAction.java | 2 +- .../action/ReferredOutUpdateAction.java | 2 + .../action/ResultsLogbookUpdateAction.java | 2 + .../lims/result/dao/ResultFileUploadDao.java | 2 +- .../daoimpl/ResultFileUploadDaoImpl.java | 16 +++- 8 files changed, 102 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 1639db3a0..413e95766 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,7 @@ +This is forked repo from Bahmni/OpenElis, to fix the security vulnerbilities found as part of security Audit for ABDM certification. + + + OpenElis ======== diff --git a/openelis/WebContent/pages/result/resultListView.jsp b/openelis/WebContent/pages/result/resultListView.jsp index adc711a7f..b33d6dcb1 100644 --- a/openelis/WebContent/pages/result/resultListView.jsp +++ b/openelis/WebContent/pages/result/resultListView.jsp @@ -1114,7 +1114,7 @@ function /*void*/ processTestReflexCD4Success(xhr) - + <% if(testResult.getUploadedFileName() != null){ %> <% String filePath = testResult.getUploadedFileName(); String fileNameWithUUID = filePath.substring(filePath.lastIndexOf("/") + 1); @@ -1215,3 +1215,20 @@ function /*void*/ processTestReflexCD4Success(xhr)

+ + \ No newline at end of file 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 index 63446e19f..cf4b44b87 100644 --- 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 ignoreEncodingForParams = Arrays.asList("sampleXML"); - public SafeRequest(HttpServletRequest request) { - super(request); + private static Pattern[] patterns = new Pattern[]{ + // Script fragments + Pattern.compile("", 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("", Pattern.CASE_INSENSITIVE), + Pattern.compile("", 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 getParameterMap() { - logger.debug("getParameterMap"); - Map newParameterMap = new HashMap<>(); - Map 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; } -} +} \ No newline at end of file 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 index 98ffa9c88..81651d8c9 100644 --- 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 index 07fb45394..104290660 100644 --- 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 index ae2666e8c..f745737f1 100644 --- 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 index 955215b33..33f3dcd86 100644 --- 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 index a49caa4ae..b6cb58a28 100644 --- 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();