Skip to content

Commit

Permalink
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Browse files Browse the repository at this point in the history
Improves SecuredUpload.java in order to not accept Base64 encoded query strings
even if "base64" is already in deniedWebShellTokens, better guarantee.
Thanks to Nicolas.

Just an indentation in ControlFilter.

Add tcp and 4444 (for udp) in deniedWebShellTokens to stop reverse shells.
  • Loading branch information
JacquesLeRoux committed Oct 24, 2024
1 parent 62e7aa2 commit 05d0de9
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 6 deletions.
2 changes: 1 addition & 1 deletion framework/security/config/security.properties
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
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
execute,println,calc,touch,curl,base64,tcp,4444

allowStringConcatenationInUploadedFiles=false

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
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 @@ -119,14 +120,42 @@ 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 isValidEncodedText(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);
} catch (IllegalArgumentException e) {
// the encoded text isn't a Base64, allow it because there is no security risk
return true;
}
}

public static boolean isValidText(String content, List<String> allowed) throws IOException {
String contentWithoutSpaces = content.replace(" ", "");
return isValidText(content, allowed, true);
}

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;
}
return content != null ? DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed)) : false;
if (testEncodeContent && !isValidEncodedText(content, allowed)) {
return false;
}
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed));
}

public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public void webShellTokensTesting() {
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
execute,println,calc,touch,curl,base64,tcp
*/
try {
List<String> allowed = new ArrayList<>();
Expand Down Expand Up @@ -153,6 +153,8 @@ public void webShellTokensTesting() {
assertFalse(SecuredUpload.isValidText("touch", allowed));
assertFalse(SecuredUpload.isValidText("curl", allowed));
assertFalse(SecuredUpload.isValidText("base64", allowed));
assertFalse(SecuredUpload.isValidText("tcp", allowed));
assertFalse(SecuredUpload.isValidText("4444", allowed));
} catch (IOException e) {
fail(String.format("IOException occured : %s", e.getMessage()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
// wt=javabin allows Solr tests, see https://cwiki.apache.org/confluence/display/solr/javabin
if (UtilValidate.isUrl(queryString)
|| !SecuredUpload.isValidText(queryString, Collections.emptyList())
&& !(queryString.contains("JavaScriptEnabled=Y")
|| queryString.contains("wt=javabin"))) {
&& !(queryString.contains("JavaScriptEnabled=Y")
|| queryString.contains("wt=javabin"))) {
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 05d0de9

Please sign in to comment.