Mailing-List: contact users-help@groovy.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: users@groovy.incubator.apache.org
Message-ID: <55DEA594.20008@asert.com.au>
Date: Thu, 27 Aug 2015 15:52:20 +1000
From: Paul King <paulk@asert.com.au>
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64;
 rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: users@groovy.incubator.apache.org
Subject: Re: Security feedback request: Setting system properties via
 configuration
 settings
References: <55D2FA6F.9020001@asert.com.au>
 <5C676E6359909E478C7B811BDB48CA3540DEF1@CWWAPP478.windstream.com>
In-Reply-To: <5C676E6359909E478C7B811BDB48CA3540DEF1@CWWAPP478.windstream.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit


It isn't limited to scripts. Scripts are the main use case for @Grab
but it will work with normal classes too. I am not sure non-script usage
changes the risk profile. System.setProperty can be used in both scripts
and classes as well.

Where to perform the System.setProperty raises some interesting design
decisions. In the initial PR I placed that within Grap.grab. This has
the advantage that with a security manager in place you would have one
spot to toggle whether setting those properties would be allowed. The
disadvantage is that you couldn't easily discriminate between different
groups of script/class, e.g. system scripts vs user-defined ones.
Grape.grab is also called during compilation - which also sets the
system properties - as would be required to actually compile the script.
This does though need to be considered from a security point of view
and I wasn't really considering the full impact of that previously, so
I'm glad you raised the points.

So it's easy enough to move setting the system properties into the
class/script initializer. If done naively, there would be a change in
behavior to above - where currently the system properties wouldn't be
set if the global grape "kill-switch", 'groovy.grape.enable' was set
to false. So, we'd need to wrap the property setting inside an
appropriate check.

That just leaves the question of whether to run setting those
properties within the compiler or not. Obviously, such settings
might be needed to enable the compilation process to succeed.
If groovy was more like java with more obviously distinct compile
and run steps, it would probably make sense to tease apart those
concerns but since groovy scripts are often run in an all-in-one
compile and then run process, there would certainly be a case for
not forcing the user to enter such information twice. However,
given the security concerns in question here, I am inclined to
require that info twice for now and make the documentation clear
on that aspect. Later, if we can find a safe way, we could streamline
to remove the need for the duplicate info.

I'll create a new PR in the coming days.

Cheers, Paul.

On 18/08/2015 10:33 PM, Winnebeck, Jason wrote:
> Is this limited to scripts? Looking at the docs
> http://docs.groovy-lang.org/latest/html/documentation/grape.html it
> doesn't say it's strictly limited to the Script class with the main
> method running the application, but I've never tried to use grapes
> outside of this context. If @Grab works anywhere, it could be a risky
> move.
>
> I would say the set property should occur with the security
> privileges of annotated class, as if that class had
> System.setProperty in its static constructor, if there is a
> SecurityManager. I think this is important as one can run scripts as
> embedded Groovy script engines, and this is a concern. I'm thinking
> of GroovyShell's evaluate/run method. You wouldn't want
> SecurityManager to block setProperty in such a script but NOT to
> block it when used with @Grab. Also, even if there is not a
> SecurityManager, it might be reasonable for an GroovyShell/class
> loader option to block @Grab from calling setProperty, because in
> these cases it would not be a good idea to modify global state when
> not running a dedicated JVM for the script.
>
> That leads into a project I saw announced on this list, whose name I
> cannot remember now, that was basically a Groovy script daemon to
> eliminate the JVM startup time by shipping the scripts to the daemon
> to run. Calling setProperty for SSL or proxy purposes in that daemon
> would not work, much less not be safe. In this case that daemon
> should be able to suppress Grape's setProperty, even if the script
> might call other setProperty, and the daemon itself should control
> proxy/SSL settings.
>
> Jason
>
> -----Original Message----- From: Paul King
> [mailto:paulk@asert.com.au] Sent: Tuesday, August 18, 2015 5:27 AM
> To: users@groovy.incubator.apache.org;
> dev@groovy.incubator.apache.org; security@apache.org Subject:
> Security feedback request: Setting system properties via
> configuration settings
>
>
> Hi folks,
>
> We are planning to add the ability to set system properties via the
> @GrabConfig annotation[1]. This will allow scripts which use @Grab to
> access an Ivy/Maven repo via a proxy (e.g. using system property
> http.proxyHost) or specify a trust certificate store (using the
> javax.net.ssl.keystore system property) or set other needed system
> properties. This will use System.setProperty under the covers[2], so
> a well-defined security mechanism is in place.
>
> We don't see this proposed feature as creating any additional
> security risk since you could just as easily add such system
> properties when invoking the JVM at the command-line or have
> System.setProperty lines in your script - the only difference in the
> latter case is the timing since @Grab does it's magic during class
> initialization and adds the grabbed jars to the classpath if needed,
> so the properties must be set before the script is run.
>
> While we don't believe this introduces any new risks, we thought we'd
> ask for wider feedback and see if anyone else perceives any possible
> security risk that we might not be aware of and allow us to modify
> the proposed approach[2] if needed to mitigate any such risks.
>
> Cheers, Paul. [1] https://issues.apache.org/jira/browse/GROOVY-7548
> [2] https://github.com/apache/incubator-groovy/pull/83
>
> --- This email has been checked for viruses by Avast antivirus
> software. https://www.avast.com/antivirus
>
>
> ----------------------------------------------------------------------
>
>
This email message and any attachments are for the sole use of the intended recipient(s). Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message and any attachments.
>


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus