Skip to content

Commit

Permalink
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Browse files Browse the repository at this point in the history 
I have refactored SecuredUpload a bit by clearly separating what is used for
web shell in uploaded files and reverse shell in query strings.

The idea is also to keep as much as possible securing code in SecuredUpload.
Even if now more than upload is concerned.

Conflicts handled by hand in ControlFilter.java
  • Loading branch information
@JacquesLeRoux
JacquesLeRoux committed Nov 18, 2024
1 parent 98abd37 commit b15ffa0
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 12 deletions.
13 changes: 7 additions & 6 deletions framework/security/config/security.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,13 @@ csvformat=CSVFormat.DEFAULT
#-- Else "template.utility.Execute" is a good replacement but not as much catching, who knows...
#--
#-- If you are sure you are safe for a token you can remove it, etc.
deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,alert(,\
%eval,@eval,eval(,runtime,import,passthru,shell_exec,assert,str_rot13,system,decode,include,page ,\
chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,iframe,object,embed,onload,build,\
python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,assign,webappPath,\
ifconfig,route,crontab,netstat,uname ,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost,thread,require,gzdeflate,\
execute,println,calc,touch,curl,base64,tcp,4444
#-- If you add a token beware that it does not content ",". It's the separator.
deniedWebShellTokens=$SHA$OFBiz$c_93W08vqLMlJHjOZ7_A6Wcaenw,$SHA$OFBiz$SigPYIfwat32A80hsAOakW0uH5A,$SHA$OFBiz$--GB0cWVhqHm-dWklW-zlGAIMkU,$SHA$OFBiz$4LL0rcLbpJHftX4g1WeF8ThuKyQ,$SHA$OFBiz$pUBkkg8Z-CiOTIYhIR1kU3DgXqY,$SHA$OFBiz$kpcFR3kDCOtNybDHn8ZPLuCVrOk,$SHA$OFBiz$zadWo3Yv2v9ArAgtj5Hdy1yjjAA,$SHA$OFBiz$gcjailglxcjBO361A7K66-4daLs,$SHA$OFBiz$5z4tXuvujvU8WlSrn3i11oUNFZo,$SHA$OFBiz$uYjP2BSE6bJ8V2QuXPWgiiwcss0,$SHA$OFBiz$fjfa3KJJBB3t7rGS5wh6outrKoY,$SHA$OFBiz$z-t-R4DxwjsPhagQBrQRCBdf3BY,$SHA$OFBiz$HrYX7eewflZIr8E888G8zmFF6Lc,$SHA$OFBiz$GHmJSyaU9BWm0-KzNnXohAxl9bI,$SHA$OFBiz$TesqkPeZ7yS47BmFslQXXr3ovGY,$SHA$OFBiz$Dz_7Eyen6amw-apK9qhPzaYmnls,$SHA$OFBiz$2pmthesuDLlR3O9n4KpTEJj9VYo,$SHA$OFBiz$k5XSAgbxb4haAtuLqnqv7br4CEs,$SHA$OFBiz$kJiheLYqfu5_fuqziZm7gbWjY4c,$SHA$OFBiz$XWtRiDGRe0TMUI7UXz0CR9Sv9tA,$SHA$OFBiz$Dp_e80SMctTATMBvAbV-AdWBWVI,$SHA$OFBiz$Neh3j_APB0yPRT_PWU3QGjECQ9E,$SHA$OFBiz$ahxsKzOxXVpO32ro0o2ZFHkfu7E,$SHA$OFBiz$L_fDY_1ZhtK1uspifvpGdE6Rcyo,$SHA$OFBiz$gPCPdqtOO6ITDDnsh0nqErM8HFk,$SHA$OFBiz$VJ7iu2ahIsnNwKzi_GJEZgKDRy0,$SHA$OFBiz$kGE-kBrJAeBJm1Xn5Qm05cuCd30,$SHA$OFBiz$xsRaikCSp41F_X764pdk_wxaYZU,$SHA$OFBiz$1unkgiOnE5O2rqUCqY_duW_xnPI,$SHA$OFBiz$nrSYHp0SxTKMp_H4sOnvrR-dZq4,$SHA$OFBiz$hmYgSZ2Wgoc8vdLM3fy10E_uZUY,$SHA$OFBiz$LuxZ11yFny6g6vjWu1jVzk7TjR8,$SHA$OFBiz$5WqGB3Lfzy9KnTXG5h60XPBlkow,$SHA$OFBiz$x4SRgUnl6S9W9X6Q21Jy5wd1QTs,$SHA$OFBiz$mas19QA53Qh6FLwf0kj0_WiFZCo,$SHA$OFBiz$0dw2-AyjRHvCHvTj-73th1DytFg,$SHA$OFBiz$KxVpL5-wZfCrzAYkDhFc-yRtTaE,$SHA$OFBiz$wwiVmh0L-Y927zwa_hrj8oGl6ew,$SHA$OFBiz$EFqJVHT9f8Ob9ZpsIJjyeuZiPDw,$SHA$OFBiz$7zGkQ-BF-RBMYtpsO7FmXBFwF6s,$SHA$OFBiz$artoVG1KXEUvIG2kbIylpmq04lw,$SHA$OFBiz$NxLLfYdQOGEPM-GXwNhPh0ur6TY,$SHA$OFBiz$gwG3lLYkvl-yVSFns_DBKqo0cNI,$SHA$OFBiz$vj02gw1FkFqA5zIyiRvYZUEcqjs,$SHA$OFBiz$zOt9gRMeVsrzI5_N8WOFwjFTyig,$SHA$OFBiz$H8u_HRRJAbSnXyt2k0t2O5d2_QY,$SHA$OFBiz$chUX79h8xiGK5mB9N75K_Bo5Avw,$SHA$OFBiz$f-Jw8wMd26vWCjbE_CvhqDusmbQ,$SHA$OFBiz$qkXsNKMv64iD7QDzQ8lv7vpKMkE,$SHA$OFBiz$O2SZT6X70zMZMFCpWlphdpQivI8,$SHA$OFBiz$64VvCKQJc7SHqT6cYwPWgdkY1zY,$SHA$OFBiz$q17B18_EXBVQvHOWvWmYholUDOI,$SHA$OFBiz$zUA4UVOfa1cphh1C1MVhOhvUB1s,$SHA$OFBiz$2reEp0H4pMMDFMnTr9gwogVzxC4,$SHA$OFBiz$f9PsGTM20kkmPX4JVd-8I2ebB6E,$SHA$OFBiz$YVum5f2Na6fpCaSfsv5TZs-kCWY,$SHA$OFBiz$a16iXjb5XbKAyCPMlS-lx12YcC4,$SHA$OFBiz$ZJnaBKIkmyjVBg1SZvNJekSTLZ8,$SHA$OFBiz$nvqFxLzsTuHCcy-t-jiUBCaqd_0,$SHA$OFBiz$ljBFfxfB8TaRILm4SfpEEb-p_is,$SHA$OFBiz$DrxjyuaalCfTjlQfEvceCKyYMIs,$SHA$OFBiz$aFruQvW2pPETw3db85EB4pvIrYo,$SHA$OFBiz$BiVTL50RhjLiK_taJFg1oSV0lLg,$SHA$OFBiz$lwWfHNn-Nt-BAWVq3W7OWVyntDI,$SHA$OFBiz$e_z0JOo9WbzRrlMnCyqB_Gxi90A,$SHA$OFBiz$93Xauqpr5d423utWhgSCnAts9w4,$SHA$OFBiz$fEPBXV3eHtsXV4bocCER9gepfyM,$SHA$OFBiz$6OH59-ADFsWMWXOUcvfzICyuAoY,$SHA$OFBiz$NTdNhs3eO_vc4AdnzocgaHJ24QQ,$SHA$OFBiz$Jej7rHT0AI9DvkUKYKE6UyGUTgE,$SHA$OFBiz$qY1GFzypqRXGEovS25CbQlRgqSs,$SHA$OFBiz$wsLFFzEGOAnotzpJAjJ_FdzodQc,$SHA$OFBiz$IdFg_DkduxtBLPuZJo0kQpSdoQo,$SHA$OFBiz$KZv9an-8Esyi2x8ocE8QArWy0eE,$SHA$OFBiz$8-W6U3hfge1JZr51F7ubQtxJ3Vg,$SHA$OFBiz$r1XqXme0FIWkQtktuOvC_tkANiM,$SHA$OFBiz$6DUJFGYw6nhmTnGN2nXkobgRNuo,$SHA$OFBiz$05Y0bAV2BKC3o926t5uXNoEJ8uM,$SHA$OFBiz$eUMU3LUH6G0z9VbgfXx5qNtw2Eg,$SHA$OFBiz$36OtnebAU_6HUHH7p2g4esqLQZ0,$SHA$OFBiz$KzwrrDc0ND6DCTJ8GdZf3LcPov4,$SHA$OFBiz$HkheNKmlDx-YpuBRXdif2ytrHDc

#-- SHA-1 versions of tokens containing (as String) at least one deniedWebShellTokens
#-- This is notably used to allow special values in query parameters.
#-- If you add a token beware that it does not content ",". It's the separator.
allowedTokens=$SHA$OFBiz$EP-l2t4A_60cRYYnEqEaSiDjfrs,$SHA$OFBiz$JG1RWjLnFzQOpNRUqllybbbfyOE

allowStringConcatenationInUploadedFiles=false

Expand Down
50 changes: 44 additions & 6 deletions framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.nio.ByteBuffer;
import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -69,6 +70,7 @@
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.ofbiz.base.crypto.HashCrypt;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.FileUtil;
import org.apache.ofbiz.base.util.StringUtil;
Expand Down Expand Up @@ -129,23 +131,34 @@ public static boolean isValidEncodedText(String content, List<String> allowed) t
}
}

// Cover method of the same name below. Historically used with 84 references when below was created
// This is used for checking there is no web shell in an uploaded file
public static boolean isValidText(String content, List<String> allowed) throws IOException {
return isValidText(content, allowed, true);
return isValidText(content, allowed, false);
}

public static boolean isValidText(String content, List<String> allowed, boolean testEncodeContent) throws IOException {
if (content == null) {
return false;
}
String contentWithoutSpaces = content.replaceAll("\\s", "");
if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
&& !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
return false;
if (!testEncodeContent) {
String contentWithoutSpaces = content.replaceAll(" ", "");
if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
&& !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
return false;
}
}
// This is used for checking there is no reverse shell in a query string
if (testEncodeContent && !isValidEncodedText(content, allowed)) {
return false;
} else if (testEncodeContent) {
// e.g. split parameters of an at all non encoded HTTP query string
List<String> queryParameters = StringUtil.split(content, "&");
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token, allowed));
}

// This is used for checking there is no web shell in an uploaded file
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed));
}

Expand Down Expand Up @@ -789,6 +802,7 @@ private static boolean isValidTextFile(String fileName, Boolean encodedContent)
return isValidText(content, allowed);
}

// This is used for checking there is no web shell
private static boolean isValid(String content, String string, List<String> allowed) {
boolean isOK = !content.toLowerCase().contains(string) || allowed.contains(string);
if (!isOK) {
Expand All @@ -797,6 +811,24 @@ private static boolean isValid(String content, String string, List<String> allow
return isOK;
}

// This is used for checking there is no reverse shell
private static boolean isValid(List<String> queryParameters, String string, List<String> allowed) {
boolean isOK = true;
for (String parameter : queryParameters) {
if (!HashCrypt.cryptBytes("SHA", "OFBiz", parameter.getBytes(StandardCharsets.UTF_8)).contains(string)
|| allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.getBytes(StandardCharsets.UTF_8)))) {
continue;
} else {
isOK = false;
break;
}
}
if (!isOK) {
Debug.logInfo("The HTTP query string contains the string '" + string + "'. It can't be uploaded for security reason", MODULE);
}
return isOK;
}

private static void deleteBadFile(String fileToCheck) {
Debug.logError("File : " + fileToCheck + ", can't be uploaded for security reason", MODULE);
File badFile = new File(fileToCheck);
Expand All @@ -815,6 +847,12 @@ private static List<String> getDeniedWebShellTokens() {
return UtilValidate.isNotEmpty(deniedTokens) ? StringUtil.split(deniedTokens, ",") : new ArrayList<>();
}

public static List<String> getallowedTokens() {
String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>();
}


private static boolean checkMaxLinesLength(String fileToCheck) {
try {
File file = new File(fileToCheck);
Expand Down
13 changes: 13 additions & 0 deletions framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLDecoder;
import java.util.HashSet;
import java.util.Set;

Expand All @@ -34,7 +35,9 @@
import javax.servlet.http.HttpServletResponse;

import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.security.SecurityUtil;

/*
Expand Down Expand Up @@ -138,6 +141,16 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
String requestUri = httpRequest.getRequestURI().substring(httpRequest.getContextPath().length());

// Reject wrong URLs
String queryString = httpRequest.getQueryString();
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrl(queryString)
|| !SecuredUpload.isValidText(queryString, SecuredUpload.getallowedTokens(), true)
&& isSolrTest()) {
Debug.logError("For security reason this URL is not accepted", module);
throw new RuntimeException("For security reason this URL is not accepted");
}
}
if (!requestUri.matches("/control/logout;jsessionid=[A-Z0-9]{32}\\.jvm1")) {
try {
String url = new URI(((HttpServletRequest) request).getRequestURL().toString())
Expand Down

0 comments on commit b15ffa0

Please sign in to comment.