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

Question

Groovy's GString concept is pretty effective (see http://groovy.codehaus.org/Strings+and+GString ).

GStrings allows you to 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 find out if Groovy uses GString: s, which can lead to security problems in your code, similar to SQL injection attacks.

In the above example, the code was written by the author of the program, so the execution of the System.exit (-1) command cannot be considered a security flaw, as this was the author's stated intention.

Let's say I'm writing a Grails web application where user input is taken from form fields (reading POST / GET parameters) and database tables (using GORM). Suppose that an attacker controls what is sent to the POST / GET server and what is in the database.

The code in my application is as follows:

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

Is there a way that an attacker can execute code in the above scenario? What for? Why not? My initial hypothesis is that using GString is always safe. Please feel free to prove that I am wrong. Be very specific.

Update # 1: To focus on the discussion, please ignore any HTML-XSS problems in the code, as this issue is about the execution of the code on the server side and not on the client side.

Update # 2: Some people have indicated that "it’s generally a good idea to filter out unwanted lines." Although filtering out "potentially bad characters" can certainly save you some security problems, it would be even better to write code that would be safe even without filtering. You can compare it using PreparedStatements in the Java JDBC API. Proper Use of PreparedStatements Guaranteed to save you from certain classes of injection attacks. Filtering your SQL input is likely to give you the same result, but using PreparedStatements strongly dominates the IMHO filtering method.

Answer 1

No, you will not have any new problems introduced by the GString mechanism, because the formation of GStrings is a "compile time" phenomenon. Although their meaning can be determined (and changed) at runtime, their shape is missing.

Another way to look at this: all that you can do with GStrings can be done with closing and concatenating strings, with exactly the same semantics; GStrings is just syntactic sugar. If you are not worried about closing (or, God forbid, string concatenation), you should not worry about GStrings.

Answer 2

There is no security issue in this code example. str1 and str2 simply call the toString methods, and there are no security holes in GStrings with standard toString methods.

If you, the author of the programs, did not define a getter for "somePropertyFieldControlledByTheAttacker" to actually make an eval on the contents of the value, it is safe.

The only thing that can happen in case of security is if the author of the programs adds it.

Answer 3

Secondly, it seems that your use is fine, because you just evaluate local variables that you define as a variable, but the string should still be cleared of the special character, but not such things as "System.exit (-1)) , because he will consider this as a whole string if there is no $ {} in the string.

It would seem to be best practice to remove all special characters from strings, such as "$ {}", because you do not need to use them in your code to be vulnerable to them.

But if it never evaluates the string in a nested manner, you're fine, so I'm not 100% sure.

Answer 4

I am sure that the objects passed as parameters (in the controller) are string strings, and therefore they do not get evaluated at runtime (as GStrings does). In addition, I experimented a bit, and it seems that to turn them into GStrings, you need to do a pretty tough job. This is doable, but requires a lot of juggling.

In short, this is probably not a security hole.

It is really, really, really a good idea to cross out special characters, at least in lines where they are not needed ... although the definition of this "remains as an exercise for the reader." But while you realize that all user data (data pulled from the Internet or from the database) is suspicious, you will be much safer.

Answer 5

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