New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
trejkaz opened this Issue Oct 22, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@trejkaz
Contributor

trejkaz commented Oct 22, 2015

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

This comment has been minimized.

Member

kcooney commented Oct 22, 2015

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?

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Oct 22, 2015

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

This comment has been minimized.

Contributor

UrsMetz commented Oct 23, 2015

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

@kcooney

This comment has been minimized.

Member

kcooney commented Oct 24, 2015

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

This comment has been minimized.

Member

kcooney commented Oct 24, 2015

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

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Oct 25, 2015

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. :/

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Oct 25, 2015

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

This comment has been minimized.

Member

kcooney commented Oct 26, 2015

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?

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Oct 26, 2015

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.

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Oct 26, 2015

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

This comment has been minimized.

Member

kcooney commented Nov 4, 2015

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?

@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Nov 4, 2015

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. :/

trejkaz added a commit to trejkaz/junit that referenced this issue Dec 7, 2015

Catch SecurityException in addition to IOException.
Fixes junit-team#1213, not by adding the doPrivileged, but by coping with lack of access to the file by pretending it doesn't exist.
@trejkaz

This comment has been minimized.

Contributor

trejkaz commented Dec 7, 2015

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.

kcooney added a commit that referenced this issue Oct 14, 2016

Catch SecurityException in addition to IOException. (#1227)
Fixes #1213, not by adding the doPrivileged, but by coping with lack of access to the file by pretending it doesn't exist.

Harrisnick added a commit to Harrisnick/junit4 that referenced this issue May 4, 2017

Catch SecurityException in addition to IOException. (junit-team#1227)
Fixes junit-team#1213, not by adding the doPrivileged, but by coping with lack of access to the file by pretending it doesn't exist.

@kcooney kcooney modified the milestone: 4.13 Aug 6, 2017

@kcooney kcooney removed the 4.13 label Aug 6, 2017

sebasjm pushed a commit to sebasjm/junit4 that referenced this issue Mar 11, 2018

Catch SecurityException in addition to IOException. (junit-team#1227)
Fixes junit-team#1213, not by adding the doPrivileged, but by coping with lack of access to the file by pretending it doesn't exist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment