SQL Prepared Statement Factory

Question

I have written a simple prepared statement factory in Java and would like some feedback on how "secure" it is. By "secure" I mean that I can pass any old garbage in as a string and it should not allow any form of SQL injection or modification of the query in an undesirable way.

Here is the code for the factory:

/**
 * A factory for creating SQL prepared statements.
 *
 * @author Jack Wilsdon
 */
public final class QueryFactory {
    private static final char[] ESCAPED_CHARACTERS = new char[] {0, '\n', '\r', '\\', '\'', '"', 0x1a};
    private static final String SUBSTITUTION_VALUE = "?";

    private final String query;
    private final String[] queryValues;
    private final char quote;

    /**
     * Constructs a new query factory with the specified query, values and quote.
     *
     * <p>
     *     This constructor is only for internal use when chaining calls.
     * </p>
     *
     * @param query the query for the factory
     * @param queryValues the query values for the factory
     * @param quote the character to wrap escaped strings in when building the query
     */
    private QueryFactory(final String query, final String[] queryValues, final char quote) {
        this.query = query;
        this.queryValues = queryValues;
        this.quote = quote;
    }

    /**
     * Constructs a new query factory with the specified query and quote.
     *
     * @param query the query for the factory
     * @param quote the character to wrap escaped strings in when building the query
     */
    public QueryFactory(final String query, final char quote) {
        this(query, new String[0], quote);
    }

    /**
     * Constructs a new query factory with the specified query.
     *
     * <p>
     *     The quote is set to a single quote by this constructor, which is the same as calling
     *     {@link #QueryFactory(String, char) QueryFactory('\'')}.
     * </p>
     *
     * @param query the query for the factory
     */
    public QueryFactory(final String query) {
        this(query, '\'');
    }

    /**
     * Returns a copy of the {@link #queryValues} array with the specified length.
     *
     * <p>
     *     If {@code newLength} is greater than the length of {@link #queryValues} then the remaining components are
     *     {@code null}.
     * </p>
     *
     * @param newLength the new length for the copied array
     * @return a copy of the {@link #queryValues} array
     */
    private String[] copyQueryValues(final int newLength) {
        // Ensure we don't try and copy more elements than we have in queryValues
        final int copyLength = Math.min(newLength, queryValues.length);
        final String[] queryValuesCopy = new String[newLength];

        System.arraycopy(queryValues, 0, queryValuesCopy, 0, copyLength);

        return queryValuesCopy;
    }

    /**
     * Escapes the specified value.
     *
     * <p>
     *     This method sanitizes the specified value so that it can be safely used in a <strong>quoted</strong> SQL
     *     string.
     * </p>
     *
     * <p>
     *     The {@link #ESCAPED_CHARACTERS} list is based off the list used by the MySQL
     *     <a href="http://dev.mysql.com/doc/refman/5.7/en/mysql-real-escape-string.html">mysql_real_escape_string</a>
     *     function.
     * </p>
     *
     * @param value the value to escape
     * @return an escaped copy of the specified value
     */
    private static String escape(final String value) {
        final StringBuilder escaped = new StringBuilder();
        for (final char character : value.toCharArray()) {
            for (final char escapeCharacter : ESCAPED_CHARACTERS) {
                if (character == escapeCharacter) {
                    escaped.append('\\');
                    break;
                }
            }

            escaped.append(character);
        }

        return escaped.toString();
    }

    /**
     * Sets the specified parameter index to the specified value.
     *
     * @param index the index of the parameter to set
     * @param value the value for the parameter
     * @param escapeValue whether or not to escape the specified value
     * @return a new {@link QueryFactory} instance with the specified parameter set
     */
    public QueryFactory set(final int index, final String value, final boolean escapeValue) {
        // Work out how long our new array needs to be if we're inserting past the end of queryValues
        final int newQueryValuesLength = Math.max(queryValues.length, index + 1);
        final String[] newQueryValues = copyQueryValues(newQueryValuesLength);

        if (escapeValue) {
            newQueryValues[index] = quote + escape(value) + quote;
        } else {
            newQueryValues[index] = value;
        }

        return new QueryFactory(query, newQueryValues, quote);
    }

    /**
     * Sets the specified parameter index to the specified value.
     *
     * <p>
     *     This method escapes the specified value and is the same as calling
     *     {@link #set(int, String, boolean) set(index, value, true)}.
     * </p>
     *
     * @param index the index of the parameter to set
     * @param value the value for the parameter
     * @return a new {@link QueryFactory} instance with the specified parameter set
     */
    public QueryFactory set(final int index, final String value) {
        return set(index, value, true);
    }

    /**
     * Sets the next available parameter to the specified value.
     * @param value the value for the parameter
     * @param escapeValue whether or not to escape the specified value
     * @return a new {@link QueryFactory} instance with the specified parameter set
     */
    public QueryFactory set(final String value, final boolean escapeValue) {
        return set(queryValues.length, value, escapeValue);
    }

    /**
     * Sets the next available parameter to the specified value.
     *
     * <p>
     *     This method escapes the specified value and is the same as calling
     *     {@link #set(String, boolean) set(value, true)}.
     * </p>
     *
     * @param value the value for the parameter
     * @return a new {@link QueryFactory} instance with the specified parameter set
     */
    public QueryFactory set(final String value) {
        return set(value, true);
    }

    /**
     * Converts the factory to a query string.
     * @return the built query
     */
    public String toString() {
        final StringBuilder builder = new StringBuilder(query);
        int nextIndex = -1;

        // Iterate all of our query values in order and find the next substitution value in the query string
        for (final String queryValue : queryValues) {
            nextIndex = builder.indexOf(SUBSTITUTION_VALUE, nextIndex + 1);

            // If there is another substitution value then we delete it and insert our query value instead
            if (nextIndex != -1) {
                builder.deleteCharAt(nextIndex);
                builder.insert(nextIndex, queryValue);
            } else {
                break;
            }
        }

        return builder.toString();
    }
}

And an example of how you would use it:

final String myQuery = new QueryFactory("SELECT name FROM users WHERE ? = ?")
    .set("id", false)
    .set("5")
    .toString();

And one or two thoughts I've had on the code:

1. Instead of having an escapeValue argument, should I rename set to rawSet? The main issue I see with my current method can be seen in my example above, .set("id", false) makes it look like you're setting the value of "id" to false when in fact you're setting the next value to "id" and disabling escaping for it.

2. Is my character escaping code enough to prevent injection? I found the list of characters in the mysql_real_escape_string documentation.

3. What do you think of my commenting? Is there enough and does it make sense?

Answer 1

As a user of your library, I would expect that an id parameter would accept an unquoted value, so, I pass false to the set method call.

final String myQuery = new QueryFactory("SELECT name FROM users WHERE ? = ?")
    .set("id", false)
    .set("5 or 1=1", false) //this is the point of reference
    .toString();

The resulting query is:

SELECT name FROM users WHERE id = 5 or 1=1

Alternatively, as a user of your library, in my code I could just inject code in the parameter that expects a field name:

final String myQuery = new QueryFactory("SELECT name FROM users WHERE ? = ?")
    .set("1=1 or id", false) //this is the point of reference
    .set("5")
    .toString();

The resulting query is:

SELECT name FROM users WHERE 1=1 or id = '5'

More specifically, the problems of the code are:

• The using application is defining if the value must be quoted or not. There should be something stricter.
• Keywords that may be injected in a parameter value are not checked. In my case, I'm giving a String as an unquoted value, and because it's unquoted, the OR operator will take effect. The = character is also a character that should not be given in an unquoted String. The space character is also a character that should not be allowed in an unquoted String.

Suggestions:

• There should be some overloaded set methods for accepting different types of values eg int, String etc, and then deciding internally for every case if quoting is necessary or not.
• There should be a special setField method, accepting a field name for criteria, that will always be appended unquoted, and has to be checked for keyword injections.

Moreover:

• The name of the class as QueryFactory is misleading. I would expect that your class would be a Singleton, it would implement the Factory pattern and it creates new Query objects.

Answer 2

Naming / description

You describe your code as an "SQL Prepared Statement Factory". That suggests that it helps one create prepared statements, yet that isn't what it actually does. It is more or less analogous to a prepared statement itself, albeit with none of the pre-compilation advantages that JDBC PreparedStatements typically provide.

Parameter types

Your query factory accepts only Strings as parameter values, whereas SQL recognizes a variety of data types, and most databases recognize an even wider range of types. It is not safe to assume that presenting a string representation of a value of some other type has the same meaning in your query as presenting a correctly-formatted literal of the correct type.

Furthermore, the syntax required for values of various types varies from database to database. This presents a serious problem for implementing a generic query factory that does not ultimately rely on an underlying JDBC driver. Your code seems to rely on the user to format non-string values appropriately for the target database.

Quoting

Your code seems to express the odd idea that quoting is necessary only for values that (may) need to be escaped. I guess the idea is to work around the lack of parameter typing by expecting the user to request escaping for parameters that should treated as SQL strings, and to not do so for parameters that should be treated as the verbatim text of SQL literal values. That's crazy.

For one thing, if you want to have special treatment for parameters that are to be taken as string value contents, then I would strongly recommend an altogether separate method for setting the values of those parameters. As it is, your API is unclear and a trap for users. SQL string values must always be presented quoted, but a user of your API who wants to insert a String parameter value that they are confident does not require escaping (maybe because it's a Java String literal) might think they can set it without requesting escaping, but they must provide their own quotes in that case. On the other hand, a user who wants to insert a non-String must be careful to specify non-escaping lest their value be unwantedly quoted in the generated query.

Security

For another thing, the possibility of feeding arbitrary unquoted text into query parameters severely undermines the main point of a class such as yours, which is indeed the point on which you explicitly sought feedback:

I [...] would like some feedback on how "secure" it is. By "secure" I mean that I can pass any old garbage in as a string and it should not allow any form of SQL injection or modification of the query in an undesirable way.

In fact, the user must exercise some care with your class to avoid the possibility of SQL injection. In the end, this comes down to another aspect of the problem of your class relying mostly on the user to format values correctly for the database. Relying on the user == accepting the consequences of user error. Including security consequences.

Literal appearance of the substitution character

Your class appears to provide no mechanism to express queries in which the substitution character appears as a literal. It cannot distinguish that case from the one where that character is a placeholder for a parameter.

Variation from standard prepared statement behavior

JDBC and native prepared statements provide for placeholders for SQL values, appearing wherever values can appear, not for arbitrary text at arbitrary positions, as does yours. This is why they don't have the problem with literal appearances of value placeholders that your code has, but it's bigger than that. In particular, standard prepared statements do not support placeholders for SQL keywords, for table names, or for column names (and this is what allows for them to be pre-compiled). Of course, you can support placeholders in those contexts if you wish, since you're not parsing, much less pre-compiling anything, but the resulting class will feel a bit alien to users familiar with JDBC.

Name of the set() methods

The name of the set() methods is a bit confusing, and the variation in its overloads is very confusing. Since the method name does not itself designate what is to be set, I would expect the parameters to pick up that responsibility. Some of the overrides do, sort of, by accepting a parameter index as their first argument, but some don't.

Implementation details

• The set(int, String, boolean) method potentially makes a copy of the parameter value array. Indeed, one of the relatively natural usage patterns for this class would end up creating a copy of the parameter array for every parameter. This is really inefficient. It would be better to create a right-size parameter array from the beginning. Moreover, then you would not have to worry that ...

• If the user does not provide values for one or more trailing parameter placeholders, then those placeholders will be output literally, instead of being replaced with NULL.

Overall

I think the problem you're trying to tackle is a lot more complicated than you've given it credit for. You clarified in comments that you wanted to "implement [something] similar to JDBC's PreparedStatement," and I guess you have done, but I'm afraid that the similarity is pretty superficial.