Skip to content

Commit

Permalink
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Browse files Browse the repository at this point in the history 
Add ROT13, and improves few short deniedWebShellTokens by surrounding them by
spaces. It's to avoid collisions while loading image files

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.

Adds 6 new deniedWebShellTokens in security.properties

This follows Leïla comment. It simplifies the SecuredUpload::isValidEncodedText
content and renames it isNotValidEncodedText

Also 3 new deniedWebShellTokens are added, some more to come

Conflicts handled by hand in ControlFilter.java and SecuredUpload.java
  • Loading branch information
@JacquesLeRoux
JacquesLeRoux committed Dec 3, 2024
1 parent e852957 commit 8385c8e
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 36 deletions.
13 changes: 11 additions & 2 deletions framework/security/config/security.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,21 @@ 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
#--
#-- If you cross issues while loading an image file because of a token found there, you may try to surround the string by spaces, as " tr " below.
#-- Actually most of the tokens should but it's now a bit late for me. I mean to test all of them...
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, tr , 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
49 changes: 22 additions & 27 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 @@ -40,7 +40,6 @@
import java.util.ArrayList;
import java.util.Base64;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -52,7 +51,7 @@
import javax.imageio.ImageReader;
import javax.imageio.stream.ImageInputStream;

import org.apache.batik.dom.svg.SAXSVGDocumentFactory;
import org.apache.batik.anim.dom.SAXSVGDocumentFactory;
import org.apache.batik.util.XMLResourceDescriptor;
import org.apache.commons.csv.CSVFormat;
import org.apache.commons.csv.CSVParser;
Expand Down Expand Up @@ -98,7 +97,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 @@ -120,46 +119,41 @@ public class SecuredUpload {
* @return
* @throws IOException
*/
public static boolean isValidEncodedText(String content, List<String> allowed) throws IOException {
public static boolean isNotValidEncodedText(String content, List<String> allowed) throws IOException {
try {
return !isValidText(Base64.getDecoder().decode(content).toString(), allowed, false)
|| !isValidText(Base64.getMimeDecoder().decode(content).toString(), allowed, false)
|| !isValidText(Base64.getUrlDecoder().decode(content).toString(), allowed, false);
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 true;
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
// 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 && !isValidEncodedText(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 @@ -451,7 +445,7 @@ private static boolean imageMadeSafe(String fileName) {
default:
throw new IOException("Format of the original image " + fileName + " is not supported for write operation !");
}
imageParser.writeImage(sanitizedImage, fos, new HashMap<>());
imageParser.writeImage(sanitizedImage, fos, null);
}
// Set state flag
safeState = true;
Expand Down Expand Up @@ -505,7 +499,7 @@ private static boolean isPdfFile(String fileName) {
return false;
}
// Load stream in PDF parser
PdfReader reader = new PdfReader(file.getAbsolutePath());
new PdfReader(file.getAbsolutePath());
return true;
} catch (Exception e) {
return false;
Expand Down Expand Up @@ -802,21 +796,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 @@ -145,7 +145,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrl(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
7 changes: 1 addition & 6 deletions framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,7 @@ public void renderLink(Appendable writer, Map<String, Object> context, MenuLink
targetParameters.append(parameter.getKey());
targetParameters.append("'");
targetParameters.append(",'value':'");
UtilCodec.SimpleEncoder simpleEncoder = (UtilCodec.SimpleEncoder) context.get("simpleEncoder");
if (simpleEncoder != null) {
targetParameters.append(simpleEncoder.encode(parameter.getValue()));
} else {
targetParameters.append(parameter.getValue());
}
targetParameters.append(parameter.getValue());
targetParameters.append("'}");
}
targetParameters.append("]");
Expand Down

0 comments on commit 8385c8e

Please sign in to comment.