Groovy
  1. Groovy
  2. GROOVY-5116

Groovy enforces the use of the the dangerous permission java.util.PropertyPermission "*" "read,write" when using a SecurityManager

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.8.3
    • Fix Version/s: None
    • Component/s: groovy-runtime
    • Labels:

      Description

      In several occurrences in the code, the system properties are accessed in this manner:

      groovy.grape.Grape.java

      private static boolean enableGrapes = Boolean.valueOf(System.getProperties().getProperty("groovy.grape.enable", "true"));
      

      The use of System.getProperties() forces the use of this permission in the SecurityManager:

       java.util.PropertyPermission "*" "read,write"

      This is not really desired in security sensitive environments. It is not possible to use more fine-grained permission declaration like e.g.:

       java.util.PropertyPermission "groovy.*" "read,write"

      This problem could be easily avoided by accessing the properties in this manner:

      private static boolean enableGrapes = Boolean.valueOf(System.getProperty("groovy.grape.enable", "true"));
      

      Without the use of System.getProperties() it is not mandatory to set the dangerous write permission on all system properties and more fine-grained security permissions like in the example could be used.

        Activity

        Hide
        Paul King added a comment -

        I fixed the grape occurrences but there is one more in GroovyMain still outstanding that I am still investigating.

        Show
        Paul King added a comment - I fixed the grape occurrences but there is one more in GroovyMain still outstanding that I am still investigating.
        Hide
        Benjamin Wolff added a comment -

        Nice!

        Did you run a text search over the Groovy source to identify the occurences of 'System.getProperties()' and are the two you mentioned the only ones?

        Maybe it should also be checked, that there are only "groovy." prefixed custom system properties are used so that the permissions can be configured for Groovy as I mentioned in the issue description?! Maybe it would be a good to document which properties are used somewhere, if that's not already done .

        Thanks for investigating!

        Show
        Benjamin Wolff added a comment - Nice! Did you run a text search over the Groovy source to identify the occurences of 'System.getProperties()' and are the two you mentioned the only ones? Maybe it should also be checked, that there are only "groovy." prefixed custom system properties are used so that the permissions can be configured for Groovy as I mentioned in the issue description?! Maybe it would be a good to document which properties are used somewhere, if that's not already done . Thanks for investigating!
        Hide
        Benjamin Wolff added a comment -

        Hi, just wanted to ask, whether there are any news or plans regarding this issue?

        Thank you!

        Cheers,
        Ben

        Show
        Benjamin Wolff added a comment - Hi, just wanted to ask, whether there are any news or plans regarding this issue? Thank you! Cheers, Ben
        Hide
        Paul King added a comment -

        Hi Ben, in what context are you setting:

        java.util.PropertyPermission "*" "read,write"

        Thanks, Paul.

        Show
        Paul King added a comment - Hi Ben, in what context are you setting: java.util.PropertyPermission "*" "read,write" Thanks, Paul.
        Hide
        Benjamin Wolff added a comment -

        I hope I understood your question correct .

        For example, when activating the SecurityManager in tomcat, by default this very mighty setting to read and modify all system properties is not activated, for good reasons. The point here is, that using Groovy in environments (e.g. with Grails ont tomcat) where tighter security restrictions are demanded, forces you to set this very permissive setting in, e.g., the catalina.policy file of the tomcat for the web-application.

        Cheers,
        Ben

        Show
        Benjamin Wolff added a comment - I hope I understood your question correct . For example, when activating the SecurityManager in tomcat, by default this very mighty setting to read and modify all system properties is not activated, for good reasons. The point here is, that using Groovy in environments (e.g. with Grails ont tomcat) where tighter security restrictions are demanded, forces you to set this very permissive setting in, e.g., the catalina.policy file of the tomcat for the web-application. Cheers, Ben
        Hide
        Jochen Theodorou added a comment -

        and if you do not allow Groovy to write system properties, Groovy will cause an security exception? If yes it would help us telling where, because afaik Groovy does not write system properties anywhere.

        Show
        Jochen Theodorou added a comment - and if you do not allow Groovy to write system properties, Groovy will cause an security exception? If yes it would help us telling where, because afaik Groovy does not write system properties anywhere.
        Hide
        Benjamin Wolff added a comment -

        Yes, Groovy will cause an security exception during the bootstrap process of the web-application, which in turn fails to start up.

        As I wrote in the issue description, there is an important difference between using the two calls

        System.getProperties().getProperty(...)

        and

        System.getProperty(...)

        In the first call, you retrieve a map of ALL specified system properties. This map allows you to freely access and remove the properties. Therefore, the mere call of this method requires the "*" "read,write" permission on all properties, even if you don't write (or read) any property from the returned map!

        The second call, where you explicitly get a specific property, can be easily and securely configured in the permissions, as I suggested in the issue description.

        A subtle, but very fundamental difference .

        Cheers,
        Ben

        Show
        Benjamin Wolff added a comment - Yes, Groovy will cause an security exception during the bootstrap process of the web-application, which in turn fails to start up. As I wrote in the issue description, there is an important difference between using the two calls System .getProperties().getProperty(...) and System .getProperty(...) In the first call, you retrieve a map of ALL specified system properties. This map allows you to freely access and remove the properties. Therefore, the mere call of this method requires the "*" "read,write" permission on all properties, even if you don't write (or read) any property from the returned map! The second call, where you explicitly get a specific property, can be easily and securely configured in the permissions, as I suggested in the issue description. A subtle, but very fundamental difference . Cheers, Ben
        Hide
        Guillaume Delcroix added a comment -

        Ben, perhaps you could suggest a patch (or a github pull request) with the changes you think would solve those security permissions problems?

        Show
        Guillaume Delcroix added a comment - Ben, perhaps you could suggest a patch (or a github pull request) with the changes you think would solve those security permissions problems?
        Hide
        Benjamin Wolff added a comment -

        Hi Guillaume,

        sure, I'd love to, never have issued a patch or pull request yet, but there has to be a first time . So I think I'll have to clone the Groovy repo, make the change and send a pull request? Ok, I'll sort that out when I have the time.

        For the procedure, I think it is save to just do a fulltext search on the codebase for System.getProperties(). to identify the critical spots, and simply remove the getProperties(). part.

        Show
        Benjamin Wolff added a comment - Hi Guillaume, sure, I'd love to, never have issued a patch or pull request yet, but there has to be a first time . So I think I'll have to clone the Groovy repo, make the change and send a pull request? Ok, I'll sort that out when I have the time. For the procedure, I think it is save to just do a fulltext search on the codebase for System.getProperties(). to identify the critical spots, and simply remove the getProperties(). part.
        Hide
        Jochen Theodorou added a comment - - edited

        Benjamin, the only place I found, that does use System.getProperties() is in GroovyMain. Do you use GroovyMain from tomcat?

        Show
        Jochen Theodorou added a comment - - edited Benjamin, the only place I found, that does use System.getProperties() is in GroovyMain. Do you use GroovyMain from tomcat?
        Hide
        Benjamin Wolff added a comment -

        Sorry, I can't tell. I'm using a Grails application and tracked down the SecurityException one by one and also came over this issue. I'm not using Groovy, besides coding in it, in any special way, so I don't know if Grails uses the GroovyMain internally. However, the issue originated from the Grape occurrence, so it may be that the GroovyMain is not used anyway...

        Show
        Benjamin Wolff added a comment - Sorry, I can't tell. I'm using a Grails application and tracked down the SecurityException one by one and also came over this issue. I'm not using Groovy, besides coding in it, in any special way, so I don't know if Grails uses the GroovyMain internally. However, the issue originated from the Grape occurrence, so it may be that the GroovyMain is not used anyway...
        Hide
        Guillaume Delcroix added a comment -

        There are also some calls to System.properties or System.properties['someKey'] as well, which are in the end call System.getProperties()

        Show
        Guillaume Delcroix added a comment - There are also some calls to System.properties or System.properties ['someKey'] as well, which are in the end call System.getProperties()
        Hide
        Guillaume Delcroix added a comment -

        To be precise, related to the GroovyConsole in GTKDefaults.groovy, WindowsDefaults.groovy.
        And in relation to groovysh, in CommandSupport.groovy and Groovysh.groovy.

        Show
        Guillaume Delcroix added a comment - To be precise, related to the GroovyConsole in GTKDefaults.groovy, WindowsDefaults.groovy. And in relation to groovysh, in CommandSupport.groovy and Groovysh.groovy.
        Hide
        Jochen Theodorou added a comment -

        Benjamin, could you give us the trace of such an SecurityException? So that I can see where the offending call is actually done

        Show
        Jochen Theodorou added a comment - Benjamin, could you give us the trace of such an SecurityException? So that I can see where the offending call is actually done
        Hide
        Benjamin Wolff added a comment -

        I did some testing and currently, I could not reproduce the described behaviour any longer.

        However, the issue with the getProperties() usage still remains.

        Just for the archives: I'm using Grails 2.0.4, which in turn bundles Groovy 1.8.6. The environment is an Apache Tomcat 7 with activated security manager. The catalina.policy file is the default one with the following additional permissions, which I identified to be required as a baseline for this environment:

        // Additional permissions to get Grails applications running
        grant {
        	// Necessary permissions already present in CERN default cfg
        	permission java.net.SocketPermission "*", "connect, resolve";
        	permission java.lang.RuntimePermission "createClassLoader";
        	permission java.lang.RuntimePermission "getClassLoader";
        	permission java.lang.RuntimePermission "accessDeclaredMembers";
        	permission java.lang.RuntimePermission "getProtectionDomain";
        	permission java.lang.RuntimePermission "accessClassInPackage.*";
        	permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
        	permission java.util.PropertyPermission "*", "read";
        	permission java.io.FilePermission "${catalina.home}${file.separator}logs${file.separator}-", "read, write, delete";
        	permission java.io.FilePermission "${catalina.home}${file.separator}temp${file.separator}-", "read, write, delete";
        	permission java.io.FilePermission "${catalina.home}${file.separator}temp", "read";
        
        	// Additional file permission for listing the content of the logs folder 
        	permission java.io.FilePermission "${catalina.home}${file.separator}logs", "read";
        	
        	// Necessary additional permissions for Grails/Spring/Hibernate
        	permission java.util.PropertyPermission "net.sf.ehcache.*", "read,write";
        	permission java.util.PropertyPermission "grails.*", "read,write";
        	permission java.util.PropertyPermission "user.language", "read,write";
        	permission java.lang.RuntimePermission "setContextClassLoader";
        	permission java.lang.RuntimePermission "modifyThread";
        	
        	// Necessary Groovy permissions
        	permission groovy.security.GroovyCodeSourcePermission "/groovy/shell";
        	permission groovy.security.GroovyCodeSourcePermission "/groovy/script";
        };
        
        Show
        Benjamin Wolff added a comment - I did some testing and currently, I could not reproduce the described behaviour any longer. However, the issue with the getProperties() usage still remains. Just for the archives: I'm using Grails 2.0.4, which in turn bundles Groovy 1.8.6. The environment is an Apache Tomcat 7 with activated security manager. The catalina.policy file is the default one with the following additional permissions, which I identified to be required as a baseline for this environment: // Additional permissions to get Grails applications running grant { // Necessary permissions already present in CERN default cfg permission java.net.SocketPermission "*" , "connect, resolve" ; permission java.lang.RuntimePermission "createClassLoader" ; permission java.lang.RuntimePermission "getClassLoader" ; permission java.lang.RuntimePermission "accessDeclaredMembers" ; permission java.lang.RuntimePermission "getProtectionDomain" ; permission java.lang.RuntimePermission "accessClassInPackage.*" ; permission java.lang.reflect.ReflectPermission "suppressAccessChecks" ; permission java.util.PropertyPermission "*" , "read" ; permission java.io.FilePermission "${catalina.home}${file.separator}logs${file.separator}-" , "read, write, delete" ; permission java.io.FilePermission "${catalina.home}${file.separator}temp${file.separator}-" , "read, write, delete" ; permission java.io.FilePermission "${catalina.home}${file.separator}temp" , "read" ; // Additional file permission for listing the content of the logs folder permission java.io.FilePermission "${catalina.home}${file.separator}logs" , "read" ; // Necessary additional permissions for Grails/Spring/Hibernate permission java.util.PropertyPermission "net.sf.ehcache.*" , "read,write" ; permission java.util.PropertyPermission "grails.*" , "read,write" ; permission java.util.PropertyPermission "user.language" , "read,write" ; permission java.lang.RuntimePermission "setContextClassLoader" ; permission java.lang.RuntimePermission "modifyThread" ; // Necessary Groovy permissions permission groovy.security.GroovyCodeSourcePermission "/groovy/shell" ; permission groovy.security.GroovyCodeSourcePermission "/groovy/script" ; };
        Hide
        Paul King added a comment -

        Thanks for your investigations Benjamin. Some very useful information. By way of summary for this issue as far as I see it, given that the Grape usage has been fixed and that this now seems fixed in a grails context, it just remains for us to decide what if anything we need to do for the GroovyMain case.

        Show
        Paul King added a comment - Thanks for your investigations Benjamin. Some very useful information. By way of summary for this issue as far as I see it, given that the Grape usage has been fixed and that this now seems fixed in a grails context, it just remains for us to decide what if anything we need to do for the GroovyMain case.

          People

          • Assignee:
            Unassigned
            Reporter:
            Benjamin Wolff
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development