Fority Scan reported "Path Manipulation" security issues in following snippet
String filePath = getFilePath(fileLocation, fileName);
final File file = new File(filePath);
LOGGER.info("Saving report at : " + filePath);
BufferedWriter fileWriter = new BufferedWriter(new FileWriter(file));
fileWriter.write(fileContent);
so i am checking for blacklisted characters in fileLocation and throwing exception, still the Fortify is throwing the exception.
try {
String filePath = getFilePath(fileLocation, fileName);
if (isSecurePath(filePath)) {
final File file = new File(filePath);
LOGGER.info("Saving report at : " + filePath);
BufferedWriter fileWriter = new BufferedWriter(new FileWriter(file));
fileWriter.write(fileContent);
} else {
throw new Exception("Security Issue. File Path has blacklisted characters");
}
} catch (final Exception e) {
LOGGER.error("Unable to prepare mail attachment : ", e);
message = "Mail cannot be send, Unable to prepare mail attachment";
}
private boolean isSecurePath(String filePath) {
String[] blackListChars = {".."};
return (StringUtils.indexOfAny(filePath, blackListChars)< 0);
}
should i ignore the scan report or what would be the correct fix for this?
Answer
Firstly SCA is a static analysis tool, so can't check your custom validation to determine whether it works correctly or not, as this is something a dynamic tool such as WebInspect is designed to do.
Secondly, blacklisting is a poor way of securing anything, whitelisting is the far more secure method and the fact you're mentioning blacklisting validation to stdout would entice an attacker. This is because you have to account for every single possible way of being attacked, including ways that may not have been discovered yet, so could easily become out of date before the software is even released.
Thirdly, this definitely wouldn't suffice against stopping path manipulation since you're only accounting for people looking for relative paths and more specifically relative paths above the current directory.
There's no way you have of detecting if somebody specifies a full path, or if somebody goes to a directory that's a symbolic link to a separate directory altogether, along with a couple of other possible alternative attacks.
Ideally you should follow the recommendations shown by SCA and have a very specific allowed paths & filenames. Where this isn't possible, use a whitelisting technique to specify the only characters that are allowed, and then validate to specify that it's not for example a SMB share or a full path specified. If it doesn't validate according to the specification of what users should be specifying, reject it.
Doing this will get rid of the issue itself, but SCA will likely still show the issue in the results (again due to the differences between static vs dynamic analysis). This can be worked around by auditing it as such or creating a custom cleanse rule for the function that validates the issue.