Checkmarx - v 9.3.0 HF11
I am passing env value as data directory path in docker file which used in dev/uat server
ENV DATA /app/data/
In local, using following Environment variable
DATA=C:\projects\app\data\
getDataDirectory("MyDirectoryName"); // MyDirectoryName is present in data folder
public String getDataDirectory(String dirName)
{
String path = System.getenv("DATA");
if (path != null) {
path = sanitizePathValue(path);
path = encodePath(path);
dirName = sanitizePathValue(dirName);
if (!path.endsWith(File.separator)) {
path = path + File.separator;
} else if (!path.contains("data")) {
throw new MyRuntimeException("Data Directory path is incorrect");
}
} else {
return null;
}
File file = new File(dirName); // NOSONAR
if (!file.isAbsolute()) {
File tmp = new File(SecurityUtil.decodePath(path)); // NOSONAR
if (!tmp.getAbsolutePath().endsWith(Character.toString(File.separatorChar))) {
dirName = tmp.getAbsolutePath() + File.separatorChar + dirName;
} else {
dirName = tmp.getAbsolutePath() + dirName;
}
}
return dirName;
}
public static String encodePath(String path) {
try {
return URLEncoder.encode(path, "UTF-8");
} catch (UnsupportedEncodingException e) {
logger.error("Exception while encoding path", e);
}
return "";
}
public static String validateAndNormalizePath(String path) {
path = path.replaceAll("/../", "/");
path = path.replaceAll("/%46%46/", "/");
path = SecurityUtil.cleanIt(path);
path = FilenameUtils.normalize(path); // normalize path
return path;
}
public static String sanitizePathValue(String filename){
filename = validateAndNormalizePath(filename);
String regEx = "..|\\|/";
// compile the regex to create pattern
// using compile() method
Pattern pattern = Pattern.compile(regEx);
// get a matcher object from pattern
Matcher matcher = pattern.matcher(filename);
// check whether Regex string is
// found in actualString or not
boolean matches = matcher.matches();
if(matches){
throw new MyAppRuntimeException("filename:'"+filename+"' is bad.");
}
return filename;
}
public static String validateAndNormalizePath(String path) {
path = path.replaceAll("/../", "/");
path = path.replaceAll("/%46%46/", "/");
path = SecurityUtil.cleanIt(path);
path = FilenameUtils.normalize(path); // normalize path
return path;
}
[Attempt] - Update code which I tried with the help of few members to prevent path traversal issue.
Tried to sanitize string and normalize string, but no luck and getting same issue.
How to resolve Stored Absolute Path Traversal issue ?
The software uses external input to construct a pathname that should be within a restricted directory, but it does not properly neutralize absolute path sequences such as "/abs/path" that can resolve to a location that is outside of that directory. CWE-36.
The most effective way to prevent file path traversal vulnerabilities is to avoid passing user-supplied input to filesystem APIs altogether.
A directory traversal vulnerability is the result of insufficient filtering/validation of browser input from users. Directory traversal vulnerabilities can be located in web server software/files or in application code that is executed on the server.
The impact of a Directory Traversal attack An attacker can leverage a directory traversal vulnerability in the system to step out of the root directory, allowing them to access other parts of the file system to view restricted files and gather more information required to further compromise the system.
Your first attempt is not going to work because escaping alone isn't going to prevent a path traversal. Replacing single quotes with double quotes won't do it either given you need to make sure someone setting a property/env variable with ../../etc/resolv.conf
doesn't succeed in tricking your code into overwriting/reading a sensitive file. I believe Checkmarx won't look for StringUtils
as part of recognizing it as sanitized, so the simple working example below is similar without using StringUtils
.
Your second attempt won't work because it is a validator that uses control flow to prevent a bad input when it throws an exception. Checkmarx analyzes data flows. When filename
is passed as a parameter to sanitizePathValue
and returned as-is at the end, the data flow analysis sees this as not making a change to the original value.
There also appears to be some customizations in your system that recognize System.getProperty
and System.getenv
as untrusted inputs. By default, these are not recognized in this way, so anyone trying to scan your code probably would not have gotten any results for Absolute Path Traversal. It is possible that the risk profile of your application requires that you call properties and environment variables as untrusted inputs, so you can't really just remove these and revert back to the OOTB settings.
As Roman had mentioned, the logic in the query does look for values that are prepended to this untrusted input to remove those data flows as results. The below code shows how this could be done using Roman's method to trick the scanner. (I highly suggest you do not choose the route to trick the scanner.....very bad idea.) There could be other string literal values that would work using this method, but it would require some actions that control how the runtime is executed (like using chroot) to make sure it actually fixed the issue.
If you scan the code below, you should see only one vulnerable data path. The last example is likely something along the lines of what you could use to remediate the issues. It really depends on what you're trying to do with the file being created.
(I tested this on 9.2; it should work for prior versions. If it doesn't work, post your version and I can look into that version's query.)
// Vulnerable
String fn1 = System.getProperty ("test");
File f1 = new File(fn1);
// Path prepend - still vulnerable, tricks the scanner, DO NOT USE
String fn2 = System.getProperty ("test");
File f2 = new File(Paths.get ("", fn2).toString () );
// Path prepend - still vulnerable, tricks the scanner, DO NOT USE
String fn3 = System.getProperty ("test");
File f3 = new File("" + fn3);
// Path prepend - still vulnerable, tricks the scanner, DO NOT USE
String fn4 = System.getProperty ("test");
File f4 = new File("", fn4);
// Sanitized by stripping path separator as defined in the JDK
// This would be the safest method
String fn5 = System.getProperty ("test");
File f5 = new File(fn5.replaceAll (File.separator, ""));
So, in summary (TL;DR), replace the file separator in the untrusted input value:
String fn5 = System.getProperty ("test");
File f5 = new File(fn5.replaceAll (File.separator, ""));
Edit Updating for other Checkmarx users that may come across this in search of an answer.
After my answer, OP updated the question to reveal that the issue being found was due to a mechanism written for the code to run in different environments. Pre-docker, this would have been the method to use. The vulnerability would have still been detected but most courses of action would have been to say "our deployment environment has security measures around it to prevent a bad actor from injecting an undesired path into the environment variable where we store our base path."
But now, with Docker, this is a thing of the past. Generally the point of Docker is to create applications that run the way same everywhere they are deployed. Using a base path in an environment likely means OP is executing the code outside of a container for development (based on the update showing a Windows path) and inside the container for deployment. Why not just run the code in the container for development as well as deployment as is intended by Docker?
Most of the answers tend to explain that OP should use a static path. This is because they are realizing that there is no way to avoid this issue because taking an untrusted input (from the environment) and prefixing it to a path is the exact problem of Absolute Path Traversal.
OP could follow the good advice of many posters here and put a static base path in the code then use Docker volumes or Docker bind mounts.
Is it difficult? Nope. If I were OP, I'd fix the base path prefix in code to a static value of /app/data
and do a simple volume binding during development. (When you think about it, if there is storage of data in the container during a deployment then the deployment environment must be doing this exact thing for /app/data
unless the data is not kept after the lifetime of the container.)
With the base path fixed at /app/data
, one option for OP to run their development build is:
docker run -it -v"C:\\projects\\app\\data":/app/data {container name goes here}
All data written by the application would appear in C:\projects\app\data
the same way it does when using the environment variables. The main difference is that there are no environment-variable-prefixed paths and thus no Absolute Path Traversal results from the static analysis scanner.
It depends on how Checkmarx comes to this point. Most likely because the value that is handed to File
is still tainted. So make sure both /../
and /%46%46/
are replaced by /
.
checkedInput = userInput.replaceAll("/../", "/");
Secondly, give File
a parent directory to start with and later compare the path of the file you want to process. Some common example code is below. If the file doesn't start with the full parent directory, then it means you have a path traversal.
File file = new File(BASE_DIRECTORY, userInput);
if (file.getCanonicalPath().startsWith(BASE_DIRECTORY)) {
// process file
}
Checkmarx can only check if variables contain a tainted value and in some cases if the logic is correct. Please also think about the running process and file system permissions. A lot of applications have the capability of overwriting their own executables.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With