جمعه، بهمن ۰۲، ۱۳۸۸

The r.a.Pe of JSF - Part III : The fear of SQL Injection

This is the third post of this series discussing one more interesting pattern in the mentioned project. If you are interested to find out how you should not write your program, read Part I and II either.
According to Wikipedia, "SQL injection is a code injection technique that exploits a security vulnerability occurring in the database layer of an application. The vulnerability is present when user input is either incorrectly filtered for string literalescape characters embedded in SQL statements or user input is not strongly typed and thereby unexpectedly executed".
If you are unfamiliar with this type of attack, read the whole article. It has clear examples of different types of SQL injection. The SQL injection is all about the user input! You can not attack yourself inside your code. Most libraries like JDBC or JPQL provide a mechanism for escaping user input.
For example in JPQL (or equivalently HQL) you must not insert user arguments directly inside the query. For example consider the following example which search students by name:
public Student searchStudent(String name) {
// Allows SQL injection attacks:
Query q = em.createQuery("from Student where name = '" + name + "'" );
// How to prevent injection
Query q = em.createQuery("from Student where name = :name" );
q.setParameter("name", name);
//The rest of code
...
}
The string after : in the query will be considered as an input parameter and you must provide its value after constructing the query using setParameter(name, value) method.
Now see what our asshole programmer friend has done here:

The loan request statuses are internal pieces of this application. When User opts for seeing all loan requests which are submitted to information office, this method get called. No way he can inject statuses inside the query.
IMO there are two reasons why he has wrote this method this way. First maybe he didn't know anything about SQL injection and just copy/pasted it from another method, where the use of parameters where correct. The second reason is that he knew about SQL injection and was very afraid of his code injecting malicious statements to his query! :D

Try to keep your code clean and readable. You can not attack your queries inside your program.

۲ نظر:

  1. I find consistency to be a legitimate reason to do this kind of thing.
    The penalty for this checks are not that high anyways.

    پاسخحذف
  2. If he was consistent in other areas of code, I would accept that he did it for sake of consistency! but believe me, he had no idea.
    My concern here is not the performance penalty. The code is unreadable. And look at the OR style he used. If he wanted a clean and consistent code, he must have used the Criteria API.

    پاسخحذف