Skip to content
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

Possible SQL injection vulnerability #116

Closed
seratch opened this issue Apr 18, 2013 · 7 comments
Closed

Possible SQL injection vulnerability #116

seratch opened this issue Apr 18, 2013 · 7 comments
Assignees
Milestone

Comments

@seratch
Copy link
Member

seratch commented Apr 18, 2013

SQLSyntax.apply(String) is unsafe. This API should not be public.

val input = "'Chris' or id is not null"
val name = SQLSyntax(input)
val foundUser = sql"select ${u.result.*} from ${User.as(u)} where ${u.name} = ${name}"
  .map(User(u.resultName)).single.apply()

The above code will run the following SQL.

select u.id as i_on_u, u.name as n_on_u, u.company_id as ci_on_u from users u 
  where u.name = 'Chris' or id is not null

The following case is safe. Users should use only sql"" or sqls"".

val input = "'Chris' or id is not null"
val name = sqls"${input}"
val foundUser = sql"select ${u.result.*} from ${User.as(u)} where ${u.name} = ${name}"
  .map(User(u.resultName)).single.apply()
@roeoender
Copy link

Hi, I would like to be able to specify table schema name dynamically,something like this psuedocode:
val dynamicSchemaName : SQLSyntax = SanitizedSQLSchemaName(SomeConfigurationFileLoader.loadStringParameter("dynamic.schema.name"))
val amount = sql"""select count(*) from ${dynamicSchemaName}.my_table_name""".map(_.long(1)).single.apply
Can you explain if this is or will be possible? Right now I have to fallback to SQL() and string concatenation and my own sql sanitization which is ugly.

@seratch
Copy link
Member Author

seratch commented Oct 27, 2013

val amount = sql"""select count(*) from ${dynamicSchemaName}.my_table_name""".map(_.long(1)).single.apply
Can you explain if this is or will be possible?

As mentioned above, it's impossible. All parameters will be converted to bind variables in SQLInterpolation.

Right now I have to fallback to SQL() and string concatenation and my own sql sanitization which is ugly.

It's the only way to meet your needs for now.

@denisftw
Copy link

denisftw commented Apr 9, 2014

Although I agree that the apply() method may not be the best place to put this functionality, I think it should remain somewhere in the library. One simple solution could be to create a method SQLSyntax.createUnsafe(String).

Here are my arguments:

  • a good design principle is "Generalize the 80% cases; get out of the way for the rest". Take a look at Neal Ford's presentation here: http://vimeo.com/44235657, particularly lesson 9.
  • by not allowing to generate an SQLSyntax object from an arbitrary string, developers resort to using class interpolation, which is arguably more error-prone and more dangerous.
  • creating SQL parts out of strings per se is not a vulnerability, but embedding user input directly into queries certainly is. The problem is that on the library level you never know where a developer got this particular string from. For instance, it may have been generated by something like List(1, 2, 3).mkString(",") and then used in SQL IN-operator, which is totally safe.

@seratch
Copy link
Member Author

seratch commented Apr 9, 2014

Though my greatest fear was that unsafe API becomes a dangerous bypass for library users,

by not allowing to generate an SQLSyntax object from an arbitrary string, developers resort to using class interpolation, which is arguably more error-prone and more dangerous.

Indeed you have a point there. So I've changed my mind. Now I think adding unsafe API is reasonable.

@vvviiimmm
Copy link

Maybe I'm missing something but following code doesn't work for me

val tableName = "table2"
val sqlsTableName = sqls"${tableName}"`
val interpolatedSql = sql"select * from ${sqlsTableName}"
println(interpolatedSql.statement) // prints `select * from ?`
interpolatedSql.map(r => println(r)).single().apply() // Error: near "?": syntax error

Version 2.3.5. Any help would be greatly appreciated, spent a lot of time trying to get any interpolation working.

@seratch
Copy link
Member Author

seratch commented Feb 25, 2016

val sqlsTableName = SQLSyntax.createUnsafely(tableName) should work for you. As its name tells you, undoubtedly this API is unsafe.

@vvviiimmm
Copy link

@seratch Thanks a lot for your fast reply. Apparently, this is actually what I should do, because I can't know the details about a database I'll work with. Mapping to classes doesn't make sense in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants