Skip to end of metadata
Go to start of metadata

SQL injection vulnerabilities arise in applications where elements of a SQL query originate from an untrusted source. Without precautions, the untrusted data may maliciously alter the query, resulting in information leaks or data modification. The primary means of preventing SQL injection are sanitization and validation, which are typically implemented as parameterized queries and stored procedures.

Suppose a system authenticates users by issuing the following query to a SQL database. If the query returns any results, authentication succeeds; otherwise, authentication fails.

SELECT * FROM db_user WHERE username='<USERNAME>' AND 
                            password='<PASSWORD>'

Suppose an attacker can substitute arbitrary strings for <USERNAME> and <PASSWORD>. In that case, the authentication mechanism can be bypassed by supplying the following <USERNAME> with an arbitrary password:

validuser' OR '1'='1

The authentication routine dynamically constructs the following query:

SELECT * FROM db_user WHERE username='validuser' OR '1'='1' AND password='<PASSWORD>'

If validuser is a valid user name, this SELECT statement yields the validuser record in the table. The password is never checked because username='validuser' is true; consequently, the items after the OR are not tested. As long as the components after the OR generate a syntactically correct SQL expression, the attacker is granted the access of validuser.

Similarly, an attacker could supply the following string for <PASSWORD> with an arbitrary username:

' OR '1'='1

producing the following query:

SELECT * FROM db_user WHERE username='<USERNAME>' AND password='' OR '1'='1'

'1'='1' always evaluates to true, causing the query to yield every row in the database. In this scenario, the attacker would be authenticated without needing a valid username or password.

Noncompliant Code Example

This noncompliant code example shows JDBC code to authenticate a user to a system. The password is passed as a char array, the database connection is created, and then the passwords are hashed.

Unfortunately, this code example permits a SQL injection attack by incorporating the unsanitized input argument username into the SQL command, allowing an attacker to inject validuser' OR '1'='1. The password argument cannot be used to attack this program because it is passed to the hashPassword() function, which also sanitizes the input.

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

class Login {
  public Connection getConnection() throws SQLException {
    DriverManager.registerDriver(new
            com.microsoft.sqlserver.jdbc.SQLServerDriver());
    String dbConnection = 
      PropertyManager.getProperty("db.connection");
    // Can hold some value like
    // "jdbc:microsoft:sqlserver://<HOST>:1433,<UID>,<PWD>"
    return DriverManager.getConnection(dbConnection);
  }

  String hashPassword(char[] password) {
    // Create hash of password
  }

  public void doPrivilegedAction(String username, char[] password)
                                 throws SQLException {
    Connection connection = getConnection();
    if (connection == null) {
      // Handle error
    }
    try {
      String pwd = hashPassword(password);

      String sqlString = "SELECT * FROM db_user WHERE username = '" 
                         + username +
                         "' AND password = '" + pwd + "'";
      Statement stmt = connection.createStatement();
      ResultSet rs = stmt.executeQuery(sqlString);

      if (!rs.next()) {
        throw new SecurityException(
          "User name or password incorrect"
        );
      }

      // Authenticated; proceed
    } finally {
      try {
        connection.close();
      } catch (SQLException x) {
        // Forward to handler
      }
    }
  }
}

Noncompliant Code Example (PreparedStatement)

The JDBC library provides an API for building SQL commands that sanitize untrusted data. The java.sql.PreparedStatement class properly escapes input strings, preventing SQL injection when used correctly. This code example modifies the doPrivilegedAction() method to use a PreparedStatement instead of java.sql.Statement. However, the prepared statement still permits a SQL injection attack by incorporating the unsanitized input argument username into the prepared statement.

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

class Login {
  public Connection getConnection() throws SQLException {
    DriverManager.registerDriver(new
            com.microsoft.sqlserver.jdbc.SQLServerDriver());
    String dbConnection = 
      PropertyManager.getProperty("db.connection");
    // Can hold some value like
    // "jdbc:microsoft:sqlserver://<HOST>:1433,<UID>,<PWD>"
    return DriverManager.getConnection(dbConnection);
  }

  String hashPassword(char[] password) {
    // Create hash of password
  }

  public void doPrivilegedAction(
    String username, char[] password
  ) throws SQLException {
    Connection connection = getConnection();
    if (connection == null) {
      // Handle error
    }
    try {
      String pwd = hashPassword(password);
      String sqlString = "select * from db_user where username=" + 
        username + " and password =" + pwd;      
      PreparedStatement stmt = connection.prepareStatement(sqlString);

      ResultSet rs = stmt.executeQuery();
      if (!rs.next()) {
        throw new SecurityException("User name or password incorrect");
      }

      // Authenticated; proceed
    } finally {
      try {
        connection.close();
      } catch (SQLException x) {
        // Forward to handler
      }
    }
  }
}

Compliant Solution (PreparedStatement)

This compliant solution uses a parametric query with a ? character as a placeholder for the argument. This code also validates the length of the username argument, preventing an attacker from submitting an arbitrarily long user name.

  public void doPrivilegedAction(
    String username, char[] password
  ) throws SQLException {
    Connection connection = getConnection();
    if (connection == null) {
      // Handle error
    }
    try {
      String pwd = hashPassword(password);

      // Validate username length
      if (username.length() > 8) {
        // Handle error
      }

      String sqlString = 
        "select * from db_user where username=? and password=?";
      PreparedStatement stmt = connection.prepareStatement(sqlString);
      stmt.setString(1, username);
      stmt.setString(2, pwd);
      ResultSet rs = stmt.executeQuery();
      if (!rs.next()) {
        throw new SecurityException("User name or password incorrect");
      }

      // Authenticated; proceed
    } finally {
      try {
        connection.close();
      } catch (SQLException x) {
        // Forward to handler
      }
    }
  }

Use the set*() methods of the PreparedStatement class to enforce strong type checking. This technique mitigates the SQL injection vulnerability because the input is properly escaped by automatic entrapment within double quotes. Note that prepared statements must be used even with queries that insert data into the database.

Risk Assessment

Failure to sanitize user input before processing or storing it can result in injection attacks.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

IDS00-J

High

Probable

Medium

P12

L1

Automated Detection

ToolVersionCheckerDescription
The Checker Framework

2.1.3

Tainting CheckerTrust and security errors (see Chapter 8)
CodeSonar
5.1p0
FB.SECURITY.SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING
FB.SECURITY.SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE
A prepared statement is generated from a nonconstant String
Nonconstant string passed to execute method on an SQL statement
Coverity7.5

SQLI
FB.SQL_PREPARED_STATEMENT_GENERATED_

FB.SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE

Implemented
Findbugs1.0SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTEImplemented
Fortify1.0

HTTP_Response_Splitting
SQL_Injection__Persistence
SQL_Injection

Implemented
Klocwork

SV.DATA.BOUND
SV.DATA.DB
SV.HTTP_SPLIT
SV.PATH
SV.PATH.INJ
SV.SQL

Implemented
Parasoft Jtest
10.3
BD-SECURITY-TDSQLImplemented
SonarQube
6.7

S2077

S3649

Executing SQL queries is security-sensitive

SQL queries should not be vulnerable to injection attacks

Related Vulnerabilities

CVE-2008-2370 describes a vulnerability in Apache Tomcat 4.1.0 through 4.1.37, 5.5.0 through 5.5.26, and 6.0.0 through 6.0.16. When a RequestDispatcher is used, Tomcat performs path normalization before removing the query string from the URI, which allows remote attackers to conduct directory traversal attacks and read arbitrary files via a .. (dot dot) in a request parameter.

Related Guidelines

Android Implementation Details

This rule uses Microsoft SQL Server as an example to show a database connection. However, on Android, DatabaseHelper from SQLite is used for a database connection. Because Android apps may receive untrusted data via network connections, the rule is applicable.

Bibliography




23 Comments

  1. I recommend that schema validation as described in the old dedicated xml injection guideline should be mentioned/incorporated here. You cannot whitelist an XML document that you receive on a wire (which is the case usually).

    • In sql injection even if the username's length = 8, the error is being handled. The code should be changed to if ((username.length() > 8) {} instead of >= 8.
    • s/data/input? in

      different methods must be used to sanitize untrusted user data input.

    1. You cannot whitelist an XML document that you receive on a wire (which is the case usually).

      Sure you can. That is, you could implement the CS on an XML stream you get over the net. Or you could at least use whitelisting to validate the quantity.

      In sql injection even if the username's length = 8, the error is being handled. The code should be changed to if ((username.length() > 8) {} instead of >= 8.

      Fixed

      s/data/input? in
      different methods must be used to sanitize untrusted user data input.

      Also fixed

      1. You can try to use regexs for validation of XML on wire but it is quite common to have XMLs that are very large. Regex validation for XML docs is just far fetched (I don't mean that you can't do it). The first question anyone dealing with XML asks is - is there any way to validate the XML. If yes, the dirty work from the mentioned CS can be reduced.

        1. Just wanted to add that schema validation can be expensive performance wise so it can't be suggested as a CS (some vendors can't afford to provide it), but perhaps as an exception to the rule? Even secure XML data exchange over HTTPS?

          1. Sure it can be suggested; I've just done it (smile) It may not be a fast option, but it is an option.

  2. Hi there..

    I found that there's also CWE-89: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') and CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') that related to this guideline. So maybe we can add two of this CWE's item in the Related Guidelines section.

    Regards,

  3. This rule does not seem to cover JPA context.

    In our architecture, we use JPA and not JDBC.

    We have implemented a PMD rule for IDS00-J, which checks the query method.

    • CreateNamedQuery is encouraged.
    • Use of CreateNativeQuery and CreateQuery creates an alert.
    • The feedback that the alert gives to the developer is ""Please check that parameters are named or indexed to avoid JPQL injection".
    • The developer can then mark as a false positive if it is the case.
      Does this seem to be a correct interpretation of IDS00-J? Can you offer more advice in a JPA context please?
    1. Dhruv Mohindra says:

      The reason why the rule does not cover JPA is because that belongs to Java EE which is outside our scope.

      I don't know JPA very well but it appears that the checker is prompting the programmer to make a decision whilst this check for named and indexed parameters could also be automated. OTOH, the interpretation of the rule appears to be on the right lines.

      The thing about Java EE is that someone might be using JPA, or Hibernate, iBatis etc. for their persistence layer. Security requirements for all these frameworks are going to be different at some level and consulting an expert for the specific framework might be a better option.

  4. I'm disappointed by the "solutions" proposed for XML injection.  In general, generating XML via String concatenation is a bad idea.  The solution to XML injection is the same as the solution to SQL injection, use the tools which already handle this for you.  Like PreparedStatement for SQL, if you build your XML using DOM (or StAX, SAX, etc.) then any text values that you insert should be correctly escaped by the framework.

    Schema validation is a great way to check arbitrary XML received from an untrusted source, but if you are building the XML yourself, you don't need to validate after generation if you use one of the purpose built XML builder tools (DOM,StAX,etc.).

    1. Using one of the frameworks that provide checked construction of XML is certainly a good idea – the escaping is clearly desirable.  I agree that we should at least mention the type-safe framework approach. 

      That said, we were attempting to demonstrate a variety of solutions across a variety of different classes of sanitization.  Schema validation has the charm that it is different from the PreparedStatement approach.  As you point out, using DOM (or StAX or SAX) to build the XML is essentially similar to the SQL solution. We used the example you complain about exactly because it demonstrates a completely different approach.

       

      Of course, we cannot cover the complete space of acceptable solutions. The examples and compliant solutions are consequently illustrative rather than normative. The normative portion of this rule is:

      "Such data must be sanitized both because the subsystem may be unprepared to handle the malformed input and because unsanitized input may include an injection attack.

      In particular, programs must sanitize all string data that is passed to command interpreters or parsers so that the resulting string is innocuous in the context in which it is parsed or interpreted."

      All else is illustration and discussion.

       

      1. While i can see your point, the beginning of the book describes this book as a set of secure coding practices.  in my reading of the book, i am approaching the guide as a collection of best practices for java coding.  i would imagine most others reading this book will have the same perspective.  the average developer who determines that they need to make sure their xml handling is secure is going to come upon this website (or open up their copy of the book) and arrive at this chapter.  the will skip the the section on "xml injection" and conclude that the way to write secure xml is using string concatenation and regexes.  this is a terrible conclusion.  string concatenation should be a last resort and, if absolutely necessary, then sure, the techniques currently listed here (for xml handling) could be useful.  i doubt many developers will start reading from the beginning of the section, read through the section on preventing SQL injection, and conclude that they should use a DOMBuilder for secure xml creation.  especially if they do not already happen to be familiar with the xml tools provided by the jdk.

        as for Schema validation, it is certainly a different approach but, i would argue, largely mis-applied here.  you shouldn't often have to validate xml you generate yourself, if you do, you are most likely doing it wrong (the xml generation).  xml validation is for handling untrusted xml documents.  if i do the bulk of the generation myself (using the correct tools), i should have no cause to mistrust it (otherwise, nothing is safe).

        i agree that you cannot possibly cover the complete space of acceptable solutions (or even of all possible problems), but i would imagine that for the illustrations you do include, you should at least include a best practice solution.  additionally, sql injection and xml injection are problem spaces that a large number of developers probably face in day to day development, so not including best practices in those specific areas does a disservice to this invaluable guide.

        1. as for Schema validation, it is certainly a different approach but, i would argue, largely mis-applied here ...if i do the bulk of the generation myself (using the correct tools), i should have no cause to mistrust it (otherwise, nothing is safe).

          With all due respect, I have to disagree. "Nothing is safe" is generally a good attitude towards code, yours or others. If that's the message we're conveying in our example, then I daresay we're doing our job.

          Unless you've verified your code it has bugs in it.

           

          1. Well, the schema discussion is kind of secondary to my main point.  that said, if nothing is safe, why would i trust the schema validation code?  if i can't trust anything i can't really write any code at all.  My code has bugs in it.  The schema validation code has bugs in it.  all non-trivial code has bugs in it.  i didn't understand that elucidating that point was the point of the book.

            1. I don't mean to imply that its the point of the book, only that its an important perspective when attempting to write code securely. However, i've clearly ventured off-topic here. Best to save discussions of this sort for elsewhere.

               

        2. While i can see your point, the beginning of the book describes this book as a set of secure coding practices...

          While I see your point, it would seem to be an oversight in our introduction, rather than a problem with this example. Just to echo/amplify Dean's earlier point – in practice, the code examples are necessary to crystallize the meaning of the rule. Sometimes that requires venturing outside of best practices just to clarify a point.

          1. That's good to know.  I will adjust my perspective as i read the rest of the book.  i would recommend emphasizing this point strongly on this wiki, as others might mistake the "compliant code" examples as recommendations of "good code".

            1. Agreed. That said, please do continue pointing out compliant code which you find suboptimal.

              1. sure thing.  i've already posted about an incorrect solution here.

  5. The organization of the various sanitization rules continues to confound me.  This one emphasizes "command interpreters" in the description but contains no such examples.  Instead, these can be found in IDS07-J. Do not pass untrusted, unsanitized data to the Runtime.exec() method.

    Would it be better to split this up into three separate rules for SQL injection, XML Injection, and XML External Entity Attacks?

    1. This rule was originally split up as you are proposing. We had an 'sql injection' rule and an 'xml injection' rule. They got demoted to recommendations to save space in the book.

      1. We seem to have these guidelines:

        This is also an injection rule (log injection):

        I can also find a voided:

        I'm not sure I understand the "space" argument.  I guess perhaps it would be slightly longer with three Risk Assessments and so forth. 

        I guess I would vote for breaking these back up.  It is a pretty random collection as it stands.  What do you think?

        The next question would be why are some of these guidelines and some rules?

        Final question would be why doesn't IDS07-J fit the pattern, e.g., "Prevent command injection"

        1. IIRC the space argument is that we had a space constraint on the first Java book, we didn't want it to exceed 750 pages. At the time we had 5-6 rules that said "prevent XXX injection". IDS00-J was a generalization of all these rules, and addressed all these problems from the theoretical basis of data sanitization and external interpreters (eg SQL database, HTML web browser, etc). IDS07-J is still one of these rules; it addresses command injection (even if it does not contain that phrase.)

          So to save space, we eliminated the SQL and XML rules, folding their examples into IDS00-J. We also demoted the other rules to guidelines, as you cite. Not because they are any less valid that the IDS rules we preserved, but simply because they are less prevalent.

          As for what to do in the future, my suggestion is that we keep IDS00-J as a generalized version of all the 'prevent XXX injection' rules. We could resurrect the SQL and XML injection rules, leaving IDS00-J without code examples. IMO all the guidelines should be promoted to rules (as long as we don't care about the size of the rules).

          1. I definitely think we should forget about size for now and focus on getting things right.  There are almost enough injection rules that they could have their own section/chapter (INJ).  The generalized rules could go into the introduction were we already have a section on injection attacks.  Sound like a plan?