6

I need to sanitize some user entered data before building sql queries and updates to submit to my DB.

I know that it is preferable to use either prepared statements but this is not an option. Unfortunatly, I am stuck with escaping all user supplied Input.

It looks like the Postgres JDBC libs come with a tool to do String escaping. See org.postgresql.core.Utils.escapeLiteral(..) (attached below). I am hoping that since this comes with Postgres, that it is safe to use. After several hours of googling and looking at SQL cheatsheets I am unable to find an example that will break this.

Does the following look safe enough?

public class FruitDb {

    private Connection connection;

    public void findFruit ( String /* user enterable field */ fruitColor ) {

        String query = "SELECT * FROM fruit WHERE fruit_color = " + quote( fruitColor );

        Statement statement = connection.createStatement();
        statement.executeQuery( sql );
    }

    private String quote( String toQuote ) {
        return "'" + Utils.escapeLiteral( null, s, true ).toString() + "'";
    }

}

For those interested here is the implementation of Utils.escapeLiteral. Looks reasonably safe to me...

package org.postgresql.core;
class Utils { 

    ... 

    /**
     * Escape the given literal <tt>value</tt> and append it to the string builder
     * <tt>sbuf</tt>. If <tt>sbuf</tt> is <tt>null</tt>, a new StringBuilder will be
     * returned. The argument <tt>standardConformingStrings</tt> defines whether the
     * backend expects standard-conforming string literals or allows backslash
     * escape sequences.
     * 
     * @param sbuf the string builder to append to; or <tt>null</tt>
     * @param value the string value
     * @param standardConformingStrings if standard conforming strings should be used
     * @return the sbuf argument; or a new string builder for sbuf == null
     * @throws SQLException if the string contains a <tt>\0</tt> character
     */
    public static StringBuilder escapeLiteral(StringBuilder sbuf, String value, boolean standardConformingStrings)
        throws SQLException
    {
        if (sbuf == null)
        {
            sbuf = new StringBuilder(value.length() * 11 / 10); // Add 10% for escaping.
        }
        doAppendEscapedLiteral(sbuf, value, standardConformingStrings);
        return sbuf;
    }


    private static void doAppendEscapedLiteral(Appendable sbuf, String value, boolean standardConformingStrings)
        throws SQLException
    {
        try
        {
            if (standardConformingStrings)
            {
                // With standard_conforming_strings on, escape only single-quotes.
                for (int i = 0; i < value.length(); ++i)
                {
                    char ch = value.charAt(i);
                    if (ch == '\0')
                        throw new PSQLException(GT.tr("Zero bytes may not occur in string parameters."), PSQLState.INVALID_PARAMETER_VALUE);
                    if (ch == '\'')
                        sbuf.append('\'');
                    sbuf.append(ch);
                }
            }
            else
            {
                 // REMOVED.  I am using standard encoding. 
            }
        }
        catch (IOException e)
        {
            throw new PSQLException(GT.tr("No IOException expected from StringBuffer or StringBuilder"), PSQLState.UNEXPECTED_ERROR, e);
        }
    }
}

Similar Questions:

Community
  • 1
  • 1
sixtyfootersdude
  • 25,859
  • 43
  • 145
  • 213
  • Why aren't PreparedStatements an option. It's the only 100% safe way to prevent SQL injection. –  Apr 26 '17 at 22:46
  • @a_horse_with_no_name - Two reasons 1. I am trying to understand the problem and am unable to convince myself that this is an issue. 2. legacy code. Lots of it. – sixtyfootersdude Apr 26 '17 at 22:48
  • 1
    If you're implying you're refactoring to use `Utils.escapeLiteral`, why wouldn't you refactor to use prepared statements? Unless the existing code already uses `Utils.escapeLiteral`? – Alex Apr 26 '17 at 23:35
  • 1
    If all the legacy code follows a similar pattern... it would be pretty trivial to use a regex to convert your example into a prepared statement. I've done similar changes for modifying hundreds of similar pieces of code before... nothing prevents you from writing regex's that match and replace many lines all in one shot. If the code is not very consistent, it becomes a lot harder, obviously. – Alex Apr 26 '17 at 23:39
  • JFI: COMMENT ON TABLE .. IS 'comment' is not possible with JDBC PreparedStatement.. needs some form of escaping – alfonx Feb 04 '20 at 23:20

1 Answers1

1

Yes, it is safe to use escapeLiteral() and escapeIdentifier().

In fact, escapeLiteral() and escapeIdentifier() are complements for PQescapeLiteral() and PQescapeIdentifier() that are defined in libpq.

The main difference is JDBC's escapeLiteral() doesn't consider database connection to validate character encoding. However, Java's internal encoding is Unicode and converted to database character encoding. Therefore, it does not matter.

Another difference I noticed is how strings are escaped as SQL Literal. PQescapeLiteral adds quotes for string, but escapeLiteral() doesn't. e.g. abc'xyz became 'abc''xyz' with PQescapeLiteral(), but it became abc''xyz (Notice no quotes around the result)

escapeIdentifier() adds quotes just like PQescapeIdentifier(). e.g. abc"xyz became "abc""xyz"

CERT TOP 10 Secure Coding Practices #7 (Sanitize Output) suggests to sanitize all variables regardless of previous checks/validations. One can avoid using escapeLiteral() by using prepared query, but not escapeIdentifier() since prepared query cannot separate parameterized identifiers. i.e.

SELECT user_specified_col FROM tbl;

or

SELECT * FROM tbl ORDER BY user_specified_col;

Developers must validate "user_specified_col" strictly, but they must sanitize parameters in case for improper validation. i.e. Output sanitization must be done independently.

Yasuo Ohgaki
  • 429
  • 4
  • 4
  • Current escapeLiteral() behavior does not make sense at all. I guess it may be changed in the future. I suggest to make your own wrapper so that it works like PQescapeLiteral(). – Yasuo Ohgaki Sep 23 '20 at 08:11