Unsafe use of user-supplied GString:s in Groovy/Grails

Question

The GString concept in Groovy is pretty powerful (see http://groovy.codehaus.org/Strings+and+GString).

GStrings let you do things like:

world = "World"
println "Hello ${world}"
# Output: Hello World
println "1+2 = ${1+2}"
# Output: 1+2 = 3
println "${System.exit(-1)}"
# Program terminated

I'm trying to figure out if using Groovy GString:s can introduce security problems in your code similar to SQL injection attacks.

In the example above the code was written by the author of the program, so the execution of the System.exit(-1) command cannot be seen as a security flaw as it was the stated intent of the author.

Let's say I'm writing a Grails web-app, where user input is taken from form fields (reading POST/GET params) and database tables (using GORM). Let's assume that an attacker controls both what's sent as POST/GET requests to the server and what's in the database.

The code in my app looks like this:

def str1 = params.someParameterControlledByTheAttacker
def str2 = SomeGORMPersistedObject.get(1).somePropertyFieldControlledByTheAttacker
render "Hello! Here is some text: ${str1} and ${str2}"

Is there any way an attacker can execute code in the above scenario? Why? Why not? My initial hypothesis is that GString usage is always safe. Please feel free to prove me wrong. Please be as concrete as possible.

Update #1: To keep the discussion focused, please disregard any HTML-XSS problems in the code since this question is about code-execution on the server-side, not on the client-side.

Update #2: Some people have pointed out that it is "generally a good idea to filter out unwanted strings". While filtering out "potentially bad characters" might certainly save you from some classes of security problems, it would be even better to write code that would be safe even without filtering. You can compare it with usage of PreparedStatements in the Java JDBC API - correct usage of PreparedStatements is guaranteed to save you from certain classes of injection attacks. Filtering your SQL input will probably give you the same result, but using PreparedStatements strictly dominates the filtering approach IMHO.

Answer 1

With the given code sample, there isn't a a security issue. str1 and str2 simply have their toString methods called and there is not a security hole with the default toString methods on GStrings.

Unless you, the programs author, had defined the getter for "somePropertyFieldControlledByTheAttacker" to actualy do an eval on the content of the value, it's safe.

The only way a security hole would happen is if the programs author adds one.

Answer 2

On second thought, It appears your use is fine since you simply eval the local variables that you define as a variable, but the string still has to be scrubbed of special character, but not things like "System.exit(-1)" because it will treat this as a string the whole way through unless ${} is around it in the string.

It would appear best practice would be to remove all special characters from strings, like "${}" because you don't have to use them in your code to be vulnerable to them.

But, if it never evals the string in a nested fashion you are fine, so I am not 100% sure.

Answer 3

I'm fairly certain that the objects passed in as params (in the Controller) are plain strings, and so they don't get evaluated at runtime (as GStrings do). Furthermore, I did a bit of experimentation, and it seems that to turn these into GStrings you need to do some pretty gnarly work. It's doable, but requires a lot of juggling.

In short, it's probably not a security hole.

It's still a really, really, really good idea to strip out special characters, at least in strings where they aren't needed... although determining this is 'left as an exercise to the reader'. But as long as you're conscious that all user-generated data (data pulled from the web or from the DB) is suspect, you'll be a lot safer.

Answer 4

you may have a cross-site scripting vulnerability in the above code- make sure you call .encodeAsHTML(), or a bad guy can mess with ya.

Answer 5

No, you won't have any new problems introduced by the GString mechanism, because the formation of GStrings is a "compile time" phenomenon. While their value may be determined (and changed) at run time their form is not.

Another way of looking at it: anything you can do with GStrings could be done with a closure and string concatenation, with exactly the same semantics; GStrings are just syntactic sugar. Unless you are worried about closures (or, heaven forbid, string concatenation) you shouldn't worry about GStrings.