Skip to content

Commit

Permalink
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Browse files Browse the repository at this point in the history 
We don't need hashed deniedWebShellTokens, only hashed allowedTokens, get back
to simple text.

Simplifies SecuredUpload:
 No need for Base64 handling, using deniedWebShellTokens is enough.
 Improves performance, remove duplicated calls inside loops or stream().allMatch
 Improves comments.
  • Loading branch information
@JacquesLeRoux
JacquesLeRoux committed Nov 27, 2024
1 parent 1168cbc commit 34584ea
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 39 deletions.
9 changes: 7 additions & 2 deletions framework/security/config/security.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,13 +274,18 @@ csvformat=CSVFormat.DEFAULT
#--
#-- If you are sure you are safe for a token you can remove it, etc.
#-- 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$OFBiz$6OFCtVDL3TrEPYs2DHksSgG2oxQ,$SHA$OFBiz$Yt2T-HPPUDcXpFsMlC3qGF79h54,$SHA$OFBiz$TEvm-0ZVHEB9cWGEmFLOUJbe6-4,$SHA$OFBiz$eFy-0O3wFwfEQPqThwfM9aPD6ew,$SHA$OFBiz$PQnk_0Ari5nyLg6DGnlsplwIgNA,$SHA$OFBiz$vK9n82xoc_amxbiZR6lVv3DQkEM,$SHA$OFBiz$UuSswMgxi4t2BtLZwFyhPXWX4bc,$SHA$OFBiz$5A9Qy0wwcLrWB7wn9DyhNACyslw,$SHA$OFBiz$rqrNJU3j6m_oQFYQDuIZA19jVAM
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,base32,xxd,bash


#-- 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$488OJhFI6NUQlvuqRVFHq6_KN8w
allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48

allowStringConcatenationInUploadedFiles=false

Expand Down
52 changes: 16 additions & 36 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 @@ -39,7 +39,6 @@
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.Base64;
import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -101,7 +100,7 @@

public class SecuredUpload {

// To check if a webshell is not uploaded
// To check if a webshell is not uploaded or a reverse shell put in the query string

// This can be helpful:
// https://en.wikipedia.org/wiki/File_format
Expand All @@ -122,52 +121,32 @@ public class SecuredUpload {
private static final String FILENAMEVALIDCHARACTERS =
UtilProperties.getPropertyValue("security", "fileNameValidCharacters", "[a-zA-Z0-9-_ ]");

/**
* For the content analyze if it's a base64 string with potential code injection
* @param content
* @param allowed
* @return
* @throws IOException
*/
public static boolean isNotValidEncodedText(String content, List<String> allowed) throws IOException {
try {
return isValidText(new String(Base64.getDecoder().decode(content), StandardCharsets.UTF_8), allowed);
} catch (IllegalArgumentException e) {
// the encoded text isn't a Base64, allow it because there is no security risk
return false;
}
}

// 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
// A file containing a reverse shell, base64 encoded or not, will be rejected.
// check there is no web shell in the uploaded file
// A file containing a reverse shell will be rejected.
public static boolean isValidText(String content, List<String> allowed) throws IOException {
return isValidText(content, allowed, false);
}

public static boolean isValidText(String content, List<String> allowed, boolean testEncodeContent) throws IOException {
public static boolean isValidText(String content, List<String> allowed, boolean isQuery) throws IOException {
if (content == null) {
return false;
}
if (!testEncodeContent) {
if (!isQuery) {
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 && isNotValidEncodedText(content, allowed)) {
return false;
} else if (testEncodeContent) {
// e.g. split parameters of an at all non encoded HTTP query string
} else {
// Check the query string is safe, notably no reverse shell
List<String> queryParameters = StringUtil.split(content, "&");
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token, allowed));
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token.toLowerCase(), 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));
// Check there is no web shell in an uploaded file
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content.toLowerCase(), token.toLowerCase(), allowed));
}

public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
Expand Down Expand Up @@ -872,21 +851,22 @@ private static boolean isValidTextFile(String fileName, Boolean encodedContent)
return isValidText(content, allowed);
}

// This is used for checking there is no web shell
// Check 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);
boolean isOK = !content.contains(string) || allowed.contains(string);
if (!isOK) {
Debug.logInfo("The uploaded file contains the string '" + string + "'. It can't be uploaded for security reason", MODULE);
}
return isOK;
}

// This is used for checking there is no reverse shell
// Check there is no reverse shell in query string
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)))) {
if (!parameter.contains(string)
|| allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.toLowerCase().getBytes(StandardCharsets.UTF_8)))) {
continue;
} else {
isOK = false;
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -176,7 +176,7 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrlInString(queryString)
|| !SecuredUpload.isValidText(queryString, SecuredUpload.getallowedTokens(), true)
|| !SecuredUpload.isValidText(queryString.toLowerCase(), 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");
Expand Down

0 comments on commit 34584ea

Please sign in to comment.