- Executing a dynamically generated SQL statement
- Directly writing an Http Parameter to Servlet output
- Creating an SQL PreparedStatement from dynamic string
- Array is stored directly
Executing a Dynamically Generated SQL Statement
This is most common of all. One can find mention of this vulenrability at several places. As a matter of fact, many developers are also aware of this vulnerability, although this is a different thing they end up making mistakes once in a while. In several DAO classes, the instances such as following code were found which could lead to SQL injection attacks.
StringBuilder query = new StringBuilder();
query.append( "select * from user u where u.name in (" + namesString + ")" );
try {
Connection connection = getConnection();
Statement statement = connection.createStatement();
resultSet = statement.executeQuery(query.toString());
} |
Instead of above query, one could as well make use of prepared statement such as that demonstrated in the code below. It not only makes code less vulnerable to SQL injection attacks but also makes it more efficient.
StringBuilder query = new StringBuilder();
query.append( "select * from user u where u.name in (?)" );
try {
Connection connection = getConnection();
PreparedStatement statement = connection.prepareCall(query.toString());
statement.setString( 1, namesString );
resultSet = statement.execute();
} |
Directly writing an Http Parameter to Servlet Output
In Servlet classes, I found instances where the Http request parameter was written as it is, to the output stream, without any validation checks. Following code demonstrate the same:
public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
String content = request.getParameter("some_param");
//
// .... some code goes here
//
response.getWriter().print(content);
} |
Note that above code does not persist anything. Code like above may lead to what is called reflected (or non-persistent) cross site scripting (XSS) vulnerability. Reflected XSS occur when an attacker injects browser executable code within a single HTTP response. As it goes by definition (being non-persistent), the injected attack does not get stored within the application; it manifests only users who open a maliciously crafted link or third-party web page. The attack string is included as part of the crafted URI or HTTP parameters, improperly processed by the application, and returned to the victim. You could read greater details on following OWASP page on reflect XSS
Creating an SQL PreparedStatement from Dynamic Query String
What it essentially means is the fact that although PreparedStatement was used, but the query was generated as a string buffer and not in the way recommended for prepared statement (parametrized). If unchecked, tainted data from a user would create a String where SQL injection could make it behave in unexpected and undesirable manner. One should rather make the query statement parametrized and, use the PreparedStatement appropriately. Take a look at following code to identify the vulenarble code.
StringBuilder query = new StringBuilder();
query.append( "select * from user u where u.name in (" + namesString + ")" );
try {
Connection connection = getConnection();
PreparedStatement statement = connection.prepareStatement(query.toString());
resultSet = statement.executeQuery();
} |
Array is Stored Directly
Instances of this vulnerability, Array is stored directly, could help the attacker change the objects stored in array outside of program, and the program behave in inconsistent manner as the reference to the array passed to method is held by the caller/invoker. The solution is to make a copy within the object when it gets passed. In this manner, a subsequent modification of the collection won’t affect the array stored within the object. You could read the details on following* page. Following code represents the vulnerability:
// Note that values is a String array in the code below.
//
public void setValues(String[] somevalues) {
this.values = somevalues;
} |
reference from:http://vitalflux.com/java-4-security-vulnerabilities-related-coding-practices-avoid/
appendix:
There is a Sonar Violation:
Sonar Violation: Security - Array is stored directly
public void setMyArray(String[] myArray) { this.myArray = myArray; }Solution:
public void setMyArray(String[] newMyArray) { if(newMyArray == null) { this.myArray = new String[0]; } else { this.myArray = Arrays.copyOf(newMyArray, newMyArray.length); } }
It's complaining that the array you're storing is the same array that is held by the caller. That is, if the caller subsequently modifies this array, the array stored in the object (and hence the object itself) will change.
The solution is to make a copy within the object when it gets passed. This is called defensive copying. A subsequent modification of the collection won't affect the array stored within the object.
It's also good practice to normally do this when returning a collection (e.g. in a corresponding getMyArray() call). Otherwise the receiver could perform a modification and affect the stored instance.
Note that this obviously applies to all mutable collections (and in fact all mutable objects) - not just arrays. Note also that this has a performance impact which needs to be assessed alongside other concerns.
reference from:http://*.com/questions/11580948/sonar-violation-security-array-is-stored-directly