Checking if file is within a directory for security

Question

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 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");

Answer 1

It's a bit hard without the full context of the method (or even the entire class), but it looks like this will have the functionality you want (assuming basedir and classDir are the two "safe" directories that you're checking for). I have a couple of tips for improvements, though they're not major.

Your set up for tested and parentFile seems a bit more convoluted than it needs to be (again, this could be because of the method's surrounding context, though). There's no need to even hold a reference to tested, it seems like. You can do something like the following:

File parentFile = null;
try {
    parentFile = new File(file).getCanonicalFile();
} catch(IOException ioe) {
    throw new SecurityException("The base directory failed to resolve.")
}

while(parentFile != null) {
    //The rest of your code

Notice that I also didn't break the throw new SecurityException line into multiple. This is based on your subsequent lines, which show that you're willing to have content out past its character limit. No need to perform unnecessary line breaks. It just makes the code muddier. I also reworded it, since "resolution failed to resolve" is a bit redundant.

Also, if you're checking against multiple files, I would use a List of them. This will make it easier for you to tack on functionality later, if you need to. e.g.,

List<File> safeDirectories = Arrays.asList(baseDirectory, classDirectory);
// ... Then, elsewhere ...
if(safeDirectories.contains(parentFile)) //...

Finally (minor thing), rename basedir as either baseDir or baseDirectory. This will let it follow the camelCase format and be more descriptive.

Answer 2

Nit-Picks

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(.....);
}

Answer 3

To be confident about your method, add some unit tests, for example:

// given:
// File basedir = new File("c:/work/tmp/base");
// File classDir = new File("c:/work/tmp/class");

@Test(expected = SecurityException.class)
public void testAboveParentFails() {
    checkRead("c:/work/tmp");
}

@Test(expected = SecurityException.class)
public void testUnrelatedFails() {
    checkRead("c:/blah");
}

@Test(expected = SecurityException.class)
public void testNearbyFails() {
    checkRead("c:/work/tmp/base2");
}

@Test
public void testValid() {
    checkRead("c:/work/tmp/base");
    checkRead("c:/work/tmp/class");
    checkRead("c:/work/tmp/class/hello");
    checkRead("c:/work/tmp/class/hello/asd/asdads");
}

Try to cover all corner cases you can think of.

---

I don't see the point of this check:

try {
    tested = new File(file).getCanonicalFile();
} catch (IOException e1) {
    throw new SecurityException(
            "The basedir resolution failed to resolve.");
}

Given the unit tests above (none of those paths exist in my system btw), the tests still pass even if I remove this code block. I think you can delete it.

---

I think you can simplify the main part of your method like this:

List<File> secureDirs = Arrays.asList(basedir, classDir);
File file = new File(path);
do {
    if (secureDirs.contains(file)) {
        return;
    }
    file = file.getParentFile();
} while (file != null);

The thing is, when you create a file with new File it will never be null. Btw path cannot be null either, your original implementation would fail with that too.