Fixing common Java security code violations in Sonar

To get an idea, during Sonar analysis, your project is scanned by many tools to ensure that the source code conforms with the rules you’ve created in your quality profile. Whenever a rule is violated… well a violation is raised. With Sonar you can track these violations with violations drill down view or in the source code editor. There are hundreds of rules, categorized based on their importance. Ill try, in future posts, to cover as many as I can but for now let’s take a look at some common security rules / violations. There are two pairs of rules (all of them are ranked as critical in Sonar ) we are going to examine right now.

1. Array is Stored Directly ( PMD ) and Method returns internal array ( PMD )

These violations appear in the cases when an internal Array is stored or returned directly from a method. The following example illustrates a simple class that violates these rules.

public class CalendarYear {
 private String[] months;
 public String[] getMonths() {
    return months;    
 }
 public void setMonths(String[] months) {
    this.months = months;
 }
}

To eliminate them you have to clone the Array before storing / returning it as shown in the following class implementation, so noone can modify or get the original data of your class but only a copy of them.

public class CalendarYear {
 private String[] months;
 public String[] getMonths() {
    return months.clone();    
 }
 public void setMonths(String[] months) {
    this.months = months.clone();
 }
}

2. Nonconstant string passed to execute method on an SQL statement (findbugs) and A prepared statement is generated from a nonconstant String (findbugs)

Both rules are related to database access when using JDBC libraries. Generally there are two ways to execute an SQL Commants via JDBC connection : Statement and PreparedStatement. There is a lot of discussion about pros and cons but it’s out of the scope of this post. Let’s see how the first violation is raised based on the following source code snippet.

Statement stmt = conn.createStatement();
String sqlCommand = 'Select * FROM customers WHERE name = '' + custName + ''';
stmt.execute(sqlCommand);

You’ve already noticed that the sqlcommand parameter passed to execute method is dynamically created during run-time which is not acceptable by this rule. Similar situations causes the second violation.

String sqlCommand = 'insert into customers (id, name)  values (?, ?)';
Statement stmt = conn.prepareStatement(sqlCommand);

You can overcome this problems with three different ways. You can either use StringBuilder or String.format method to create the values of the string variables. If applicable you can define the SQL Commands as Constant in class declaration, but it’s only for the case where the SQL command is not required to be changed in runtime. Let’s re-write the first code snippet using StringBuilder

Statement stmt = conn.createStatement();
stmt.execute(new StringBuilder('Select FROM customers WHERE name = '').
                         append(custName).
                         append(''').toString());

and using String.format

Statement stmt = conn.createStatement();
String sqlCommand = String.format('Select * from customers where name = '%s'', custName);
stmt.execute(sqlCommand);

For the second example you can just declare the sqlCommand as following

private static final SQLCOMMAND = insert into customers (id, name)  values (?, ?)';

There are more security rules such as the blocker Hardcoded constant database password but I assume that nobody is still hardcodes passwords in source code files…

In following articles I’m going to show you how to adhere to performance and bad practice rules. Until then I’m waiting for your comments or suggestions.

Happy coding and don’t forget to share!

Reference: Fixing common Java security code violations in Sonar from our JCG partner Papapetrou P. Patroklos at the Only Software matters blog.