java.security.AccessControlException: access denied ("java.util.PropertyPermission" "user.home" "read") running under security manager, because JUnit forgot to use doPrivileged

[hakanai]

If you perform actions which cause permission checks with the security manager (and you're doing some action which is not exploitable by the caller), then you're supposed to use doPrivileged.

BaseTestRunner in JUnit has this:

    private static File getPreferencesFile() {
        String home = System.getProperty("user.home");  // <- not using doPrivileged
        return new File(home, "junit.properties");
    }

When running our test suite with the security manager enabled, we get a failure:

Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "user.home" "read")
    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:457)
    at java.security.AccessController.checkPermission(AccessController.java:884)
    at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
    at java.lang.SecurityManager.checkPropertyAccess(SecurityManager.java:1294)
    at java.lang.System.getProperty(System.java:717)
    at junit.runner.BaseTestRunner.getPreferencesFile(BaseTestRunner.java:225)
    at junit.runner.BaseTestRunner.readPreferences(BaseTestRunner.java:232)
    at junit.runner.BaseTestRunner.getPreferences(BaseTestRunner.java:51)
    at junit.runner.BaseTestRunner.getPreference(BaseTestRunner.java:246)
    at junit.runner.BaseTestRunner.getPreference(BaseTestRunner.java:250)
    at junit.runner.BaseTestRunner.<clinit>(BaseTestRunner.java:324)

This occurs irrespective of the fact that our security policy allows reading any system property, because something further down the stack, i.e. something inside IDEA's JUnit launcher (perhaps some dynamically-generated bytecode?) is "completely" untrustworthy and thus even the "grant to all code sources" section of the policy does not apply to it.

At the moment, our workaround for this is to give AllPermission to the IDEA installation, but this is not really satisfactory because every developer tends to install it in a different location depending on their own conventions, what platform they're on, whether they have admin access on the box, etc.

If JUnit would add a doPrivileged block here (and to any other place where it seems appropriate) then we wouldn't have to do this and our rule which says JUnit is completely trusted would be sufficient.

[kcooney]

The tests being run by JUnit could very well make calls to get system properties (or other operations that involve security checks), so I suspect that changing this one place won't solve your problems.

Why are you wanting all of your tests (and apparently IDEA) to run in a security manager?

[hakanai]

In the case of our own code and our own tests, we already use doPrivileged in any locations which I'm aware of. But even if we weren't, we give our own code permission to read any system property.

Actually, we give JUnit the permission to read any system property as well, but because it isn't using doPrivileged, callers further down the stack get checked as well, and apparently no matter what we put into the security policy, those ones can't be marked as trusted.

We use the security manager to stop code (both libraries and our own code (since this is a testing framework, it seems reasonable to get extra checking for free where possible!) doing various things we don't want them to do, but mostly to stop them writing data outside of where we tell them to or exiting the JVM (yeah, it has actually happened before...) The former could theoretically be done by writing a custom NIO2 filesystem which wraps the real filesystem, but it would be a lot of work to make it support absolutely everything the real one supports. The latter is impossible to do any other way, at least as far as I am aware.

[UrsMetz]

@trejkaz Maybe some of your requirements could be satisfied by System Rules?

[kcooney]

I am wondering why I do not see the untrustworthy code in your stack trace. Are you sure you are properly granting JUnit access? Perhaps when the tests run via JUnit they run using a different version of JUnit (one bundled with IDEA?)

If you create your down fork of JUnit and add the doPrivileged call, does that fix your problem?

[kcooney]

I also wonder why this call starts with junit.runner.BaseTestRunner and not some code under the org.junit tree.

[hakanai]

Yeah... I saw that too and wondered why it was using that, considering that all our tests are JUnit 4 style. That must be some IDEA quirk, or perhaps they were just too lazy to update to using the new runner. :/

[hakanai]

Actually, why is JUnit reading user.home anyway? I'd rather test framework libraries not do things which could result in different behaviour for different users and reading from the user's home directory doesn't seem like a particularly good way to ensure predictable results.

[kcooney]

I agree with you about having different behavior for different users. That code is extremely old.

Could you answer my other questions and Urs' question?

[hakanai]

I haven't tested forking yet. Our build system has recently made inserting arbitrary jars needlessly complex. :(

What I do know is that granting all permissions to the JUnit jar somehow worked around the issue, even though all sources already have permission to read all properties, so adding such a rule shouldn't have affected anything.

[hakanai]

If I create a fresh project, the results are different.

Caused by: java.security.AccessControlException: access denied ("java.io.FilePermission" "/Users/trejkaz/junit.properties" "read")
    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
    at java.security.AccessController.checkPermission(AccessController.java:884)
    at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
    at java.lang.SecurityManager.checkRead(SecurityManager.java:888)
    at java.io.FileInputStream.<init>(FileInputStream.java:127)
    at junit.runner.BaseTestRunner.readPreferences(BaseTestRunner.java:232)
    at junit.runner.BaseTestRunner.getPreferences(BaseTestRunner.java:51)
    at junit.runner.BaseTestRunner.getPreference(BaseTestRunner.java:246)
    at junit.runner.BaseTestRunner.getPreference(BaseTestRunner.java:250)
    at junit.runner.BaseTestRunner.<clinit>(BaseTestRunner.java:324)
    ... 8 more

Here I have used the same rule to permit the properties to be read.

grant {
      permission java.util.PropertyPermission "*", "read, write";
};

And then it denies reading from that file, which I think is correct. Although JUnit really should be catching the exception and proceeding as if the file doesn't exist, I guess that is a separate issue to this one.

The difference between this project and the one at work is that Gradle is in use at work and the IDEA plugin for Gradle is known to mess with how tests are run in some fashion.

[kcooney]

It looks like readPreferences() handles IOException correctly. Is the AccessControlException not wrapped by an IOException?

In any case, I am fine using doPrivileged() here if it is the only thing blocking you (do note that we don't have a release date for 4.13 yet). I would rather not add doPrivileged() calls everywhere. It would also be nice to have tests to make sure this code works if a security manager is installers. Do you think you could send a pull?

[hakanai]

The AccessControlException isn't wrapped, no. FileInputStream Javadoc says it can also throw SecurityException, which AccessControlException extends. I guess catching it and proceeding would be fine as well because in this case we probably don't really want it reading that file anyway? :)

I still intend to see how much further I can get with this one spot fixed, but our build is broken in so many other ways at the moment that all my time has gone into trying to figure out the other ones. One of the things that's broken is that updating libraries doesn't appear to update our IDEA project, which is exactly the sort of thing I'd like to be able to do to try out new libs. :/

[hakanai]

As yet I am still unable to test that PR on our full project, because when I try to include the local file, gradle refuses to add it to the IDEA project... So I don't know if there is any other shady stuff lurking, but maybe I should just search for all catches and look to see what they're doing.