patternjavaMinor
Checking if file is within a directory for security
Viewed 0 times
directoryfilecheckingwithinsecurityfor
Problem
I have a security manager I'm writing for a Java program, and within the checkRead method, I wish to only allow reads to files within a given directory.
The basis of it is that it will get a canonical file for the input (which is actually a string as per the method contract). It will then begin taking the file's parent repeatedly until it either matches one of the trusted directories, or becomes
Are there any glaring issues (security or style) that anyone might see here?
The basis of it is that it will get a canonical file for the input (which is actually a string as per the method contract). It will then begin taking the file's parent repeatedly until it either matches one of the trusted directories, or becomes
null (from trying to take the parent of the root directory). This check should be secure both on Unix/linux and Windows environments and styles of filenames. I don't need 100% certainty that a valid and allowed, but mangled or non-standard path would pass, but I certainly cannot rework the program's API to open files in a different, more secure manner, as my application's classloader needs to use this method to check its reads. Are there any glaring issues (security or style) that anyone might see here?
File tested;
try {
tested = new File(file).getCanonicalFile();
} catch (IOException e1) {
throw new SecurityException(
"The basedir resolution failed to resolve.");
}
File parentFile = tested;
while (parentFile != null) {
if (basedir.equals(parentFile) || classDir.equals(parentFile)) {
return;
}
parentFile = parentFile.getParentFile();
}
logger.warn("MosstestSecurityManager stopped an attempt to read a file from non-core code"
+ file);
throw new SecurityException(
"MosstestSecurityManager stopped an attempt to read a file from non-core code");Solution
Nit-Picks
This exception-handling code should include the IOException as part of the SecurityException re-throw:
General
I believe the rest of the code supplies the required functionality. There are odd cases where, for example, on Linux you can remount a sub-directory at a different mount point. These two distinct file-systems may mirror each other, and a file/directory in one mount-point is identical to the other mount point.
This may produce false-positives in your code (where your code will claim the directory is not allowed, but it actually is...). False-positives in 'edge cases' in a security situation are better than false-negatives.
The intricacies of the new-in-Java-7 NIO2 Path class/interface may help with this. You could translate your
Update .... more Paths:
Using the
This exception-handling code should include the IOException as part of the SecurityException re-throw:
try {
tested = new File(file).getCanonicalFile();
} catch (IOException e1) {
// set the cause of the SecurityException so that users can potentially fix things
throw new SecurityException(
"The basedir resolution failed to resolve.", e1);
}General
I believe the rest of the code supplies the required functionality. There are odd cases where, for example, on Linux you can remount a sub-directory at a different mount point. These two distinct file-systems may mirror each other, and a file/directory in one mount-point is identical to the other mount point.
This may produce false-positives in your code (where your code will claim the directory is not allowed, but it actually is...). False-positives in 'edge cases' in a security situation are better than false-negatives.
The intricacies of the new-in-Java-7 NIO2 Path class/interface may help with this. You could translate your
File instances in to Path instances, and then use the isSameFile(...) method... which may, or may not help (reading the documentation it is unclear, and I do not have direct experience with that).Update .... more Paths:
Using the
Path and other NIO2 features, your code could probably be reduced to:// basePath is already resolved as a Path.realPath from basedir
// classPath is already resolved as a Path.realPath from classDir
Path filePath = file.topath().realpath();
if (!filePath.startsWith(basePath) || !filePath.startsWith(classPath) {
// illegal file access....
throw new SecurityException(.....);
}Code Snippets
try {
tested = new File(file).getCanonicalFile();
} catch (IOException e1) {
// set the cause of the SecurityException so that users can potentially fix things
throw new SecurityException(
"The basedir resolution failed to resolve.", e1);
}// basePath is already resolved as a Path.realPath from basedir
// classPath is already resolved as a Path.realPath from classDir
Path filePath = file.topath().realpath();
if (!filePath.startsWith(basePath) || !filePath.startsWith(classPath) {
// illegal file access....
throw new SecurityException(.....);
}Context
StackExchange Code Review Q#47233, answer score: 6
Revisions (0)
No revisions yet.