views:

3710

answers:

7

I'm trying to put some anti sql injection in place in java and am finding it very difficult to work with the the "replaceAll" string function. Ultimately I need a function that will convert any existing \ to \\, any " to \", any ' to \', and any \n to \\n so that when the string is evaluated by MySQL SQL injections will be blocked.

I've jacked up some code I was working with and all the \\\\\\\\\\\ in the function are making my eyes go nuts. If anyone happens to have an example of this I would greatly appreciate it.

+15  A: 

The only way to prevent SQL injection is with parameterized SQL. It simply isn't possible to build a filter that's smarter than the people who hack SQL for a living.

So use parameters for all input, updates, and where clauses. Dynamic SQL is simply an open door for hackers, and that includes dynamic SQL in stored procedures. Parameterize, parameterize, parameterize.

Cylon Cat
And even parameterized SQL isn't a 100% guarantee. But it's a very good start.
duffymo
@duffymo, I agree that nothing is ever 100% safe. Do you have an example of SQL injection that will work even with parameterized SQL?
Cylon Cat
@Cylon Cat: Sure, when a chunk of SQL (like @WhereClause or @tableName) is passed as the parameter, concatenated into the SQL, and executed dynamically. SQL injection occurs when you let users write your code. It doesn't matter whether you capture their code as a parameter or not.
Steve Kass
@Steve, thanks. I don't think I've seen a database that would let you pass keywords or whole SQL phrases as a parameter; that sounds dangerous. Where clauses can be parameterized just like input or update values, though. SQL Server is really good about that; Oracle is iffy with it on update statements, but fine on queries. As for dynamic table names.... no thanks.
Cylon Cat
BTW, I don't know why this isn't mentioned more, but working with PreparedStatements is also *much* easier and *much* more readable. That alone probably makes them the default for every programmer who knows about them.
Edan Maor
@Edan, it really does depend on the environment. In .NET, I know of nothing like PreparedStatements, but the combination of LINQ and an ORM provides even more safety, productivity, and maintainability. Because LINQ is integrated into C# and VB, everything about your query and results are strongly named, strongly typed, and compiler-checked. All SQL statements are generated automatically, as needed, fully parameterized. So "PreparedStatement" would feel like a step backwards. But it all depends on the environment and tools that you're working with; use whatever works best.
Cylon Cat
@Cylon: That's the argument of dynamically-generated SQL (typically by an ORM) and manually-constructed procedures. The idea of a "prepared statement" is fully present in .NET, though; the `DbCommand` (and associated `DbParameter`) abstract class provide full support for parameterizing your SQL, depending on the provider.
Adam Robinson
I've been using a homebrew mysql/java solution where a statement such as "select * from sometable where somefield = '[somefield]'" then when I execute the query My code first escapes characters in each of the argument values... [somefield] and then does a replace of somefield with the escape string. So yeah I'm just trying to find a good hook in so under the hood it uses PreparedStatements but everything else stays the same... Definitely didn't make my day to think of all the impending work of this change... So it's on tomorrow's plate
Isisagate
@Adam, I see the parallel; DBCommand is an aggregate of SQL command, parameters, connection reference, and methods to execute the command. However, "PreparedStatement" suggested the idea of preparing an execution plan; I don't know if any modern databases still require this as an explicit step in client code. I know in SQL Server, it's handled automatically on the server side.
Cylon Cat
Please note that PreparedStatements for some databases are VERY expensive to create, so if you need to do a lot of them, measure both types.
Thorbjørn Ravn Andersen
+7  A: 

Using a regular expression to remove text which could cause a SQL injection sounds like the SQL statement is being sent to the database via a Statement rather than a PreparedStatement.

One of the easiest ways to prevent an SQL injection in the first place is to use a PreparedStatement, which accepts data to substitute into a SQL statement using placeholders, which does not rely on string concatenations to create an SQL statement to send to the database.

For more information, Using Prepared Statements from The Java Tutorials would be a good place to start.

coobird
A: 

Why don't you use binds / query parameters instead?

Marius Burz
+14  A: 

PreparedStatements are the way to go, because they make SQL injection impossible. Here's a simple example taking the user's input as the parameters:

public insertUser(String name, String email) {
   Connection conn = setupTheDatabaseConnectionSomehow();
   PreparedStatement stmt = conn.prepareStatement("INSERT INTO person (name, email) values (?, ?)");
   stmt.setString(1, name);
   stmt.setString(2, email);
   stmt.executeUpdate();
}

No matter what characters are in name and email, those characters will be placed directly in the database. They won't affect the INSERT statement in any way.

There are different set methods for different data types -- which one you use depends on what your database fields are. For example, if you have an INTEGER column in the database, you should use a setInt method. The PreparedStatement documentation lists all the different methods available for setting and getting data.

Kaleb Brasee
via this method can you treat every parameter as a string and still be safe? I'm trying to figure out a way to update my existing architecture to be safe without having to rebuild the whole database layer...
Isisagate
All dynqmic SQL is just strings, so that isn't the question to ask. I'm not familiar with PrepareStatement, so the real question is does it generate a parameterized query that can then be executed with ExecuteUpdate. If yes, that's good. If no, then it's simply hiding the problem, and you may not have any secure option except redesigning the database layer. Dealing with SQL injection is one of those things you have to design in from the beginning; it's not something you can add easily later on.
Cylon Cat
If you're inserting into an INTEGER field, you'll want to use a 'setInt'. Likewise, other numerical database fields would use other setters. I posted a link to the PreparedStatement docs that list all the setter types.
Kaleb Brasee
Yes Cylon, PreparedStatements generate parameterized queries.
Kaleb Brasee
@Kaleb Brasee, thanks. That's good to know. The tools are different in every environment, but getting down to parameterized queries is the fundamental answer.
Cylon Cat
Could you please properly close JDBC resources on error?Like:try { ...} finally { try { pstmt.close(); } catch (java.sql.SQLException ignore) {}}I think examples should be correct as many people will literally copy them.If you do not like direct use of JDBC API you can use http://static.springsource.org/spring/docs/3.0.x/javadoc-api/org/springframework/jdbc/core/JdbcTemplate.html
Eduardo
I think most people would know enough to properly close connections, statements, and handle exceptions. Maybe not. If they don't... they'll learn when things blow up!
Kaleb Brasee
+3  A: 

If really you can't use Defense Option 1: Prepared Statements (Parameterized Queries) or Defense Option 2: Stored Procedures, don't build your own tool, use the OWASP Enterprise Security API. From the OWASP ESAPI hosted on Google Code:

Don’t write your own security controls! Reinventing the wheel when it comes to developing security controls for every web application or web service leads to wasted time and massive security holes. The OWASP Enterprise Security API (ESAPI) Toolkits help software developers guard against security‐related design and implementation flaws.

For more details, see Preventing SQL Injection in Java and SQL Injection Prevention Cheat Sheet.

Pay a special attention to Defense Option 3: Escaping All User Supplied Input that introduces the OWASP ESAPI project).

Pascal Thivent
The second worst way to deal with security questions is to be ignorant of the problems. The very worst way, IMO, is to buy expensive tools that allow you to think you can safely continue being ignorant of the problems.
Carl Smotricz
@Carl Smotricz To buy tools?!? Do you know what OWASP is? Did you at least check the links? If the only option is option 3, then it's a pretty good resource. Maybe just take some time before to make free claims next time.
Pascal Thivent
OWASP - very good source
Bozho
I stand by my comment but accept that (if you say so) it doesn't apply to OWASP. To me, it looked like they were supplying a lot of post-hoc band-aid fixes for problems that should have been solved at design and first implementation.
Carl Smotricz
+1  A: 

In case you are dealing with a legacy system, or you have too many places to switch to PreparedStatements in too little time - i.e. if there is an obstacle to using the best practice suggested by other answers, you can try AntiSQLFilter

Bozho
+1  A: 

(This is in answer to the OP's comment under the original question; I agree completely that PreparedStatement is the tool for this job, not regexes.)

When you say \n, do you mean the sequence \+n or an actual linefeed character? If it's the latter, the task is pretty straightforward:

s = s.replaceAll("['\"\\\\]", "\\\\$0");

To match one backslash in the input, you put four of them in the regex string. To put one backslash in the output, you put four of them in the replacement string. This is assuming you're creating the regexes and replacements in the form of Java String literals. If you create them any other way (e.g., by reading them from a file), you don't have to do all that double-escaping.

If you have a linefeed character in the input and you want to replace it with an escape sequence, you can make a second pass over the input with this:

s = s.replaceAll("\n", "\\\\n");

Or maybe you want two backslashes (I'm not too clear on that):

s = s.replaceAll("\n", "\\\\\\\\n");
Alan Moore
Thanks for the comment, I like the way you did all the characters in one, I was going about it the less regular expression way of a replace all for each... I'm not sure how to assign the answer on this question now. Ultimately PreparedStatements is the answer, but for my current objective your answer is the answer I need, would you be upset if I gave the answer to one of the earlier prepared statement's answers, or is there a way to share the answer between a couple?
Isisagate
Since this is just a temporary kludge, go ahead and accept one of the PreparedStatement answers.
Alan Moore