unquoted sql string is executed when setting savepoint

Of course, the alternative possibility is that I misread the definition of dbi_conn_queryf and that it quotes things.

Given that I don't currently have access to a running evergreen implementation, I have not been able to test this.

Thanks for the report. Unfortunately, I can confirm it.

Here's a srfsh script that exploits it:

open open-ils.pcrud
request open-ils.pcrud open-ils.pcrud.transaction.begin "AUTHTOKEN"
request open-ils.pcrud open-ils.pcrud.savepoint.set "AUTHTOKEN", "alpha\"; delete from foo; end transaction; begin transaction; savepoint \"bar;"
request open-ils.pcrud open-ils.pcrud.transaction.commit "AUTHTOKEN"
close open-ils.pcrud

Replace AUTHTOKEN, of course, with a token received from a login -- including that of an ordinary patron. And sure enough, when I tested this, foo was cleared.

Since open-ils.pcrud services are available via the HTTP translator, an exploit can be written in JavaScript and run remotely.

Wow. And yes, the savepoint code in general appears to have a number of similar errors. libdbi does not appear to give us access to libpq's PQescapeIdentifier, which I think we want pretty badly for this particular instance: http://www.postgresql.org/docs/9.1/static/libpq-exec.html#LIBPQ-PQESCAPEIDENTIFIER (long story short: dbi_conn_quote_string will take STRING and return 'ESCAPED_STRING' which doesn't fly for the savepoint identifier value, which expects no quotes or double-quotes).

As we probably don't want to try to write our own escaping logic, copying the guts of PostgreSQL's PQescapeInternal() src/interfaces/libpq/fe-exec.c might make the most short-term sense - although that pulls in more PostgreSQL internals via multibyte character handling, which we need to avoid mangling content. (It's unlikely that someone would specify a non-ASCII savepoint name in practice, but we're talking about exploits based on user input, so it's definitely a concern.) Perhaps we could just link directly against libpq and use PQescapeIdentifer accordingly.

Elsewhere, we appear to be doing some dbi_conn_quote_string() calls, although we skip those calls when input comes from the IDL (such as perm values, PK names, table names). I suppose this is defensible.

Also, in general database devs strongly suggest using prepared statements with placeholders to avoid exactly these sorts of problems. Perhaps a longer-term approach would be to drop libdbi in favour of using libpq & PQexecParams directly?

I'm mostly on the same page as Dan. As a possible step short of copying in part of fs-exec.c, I've posted a fix to the lp1098377_savepoint_name_sql_injection branch in the security repo that adds a routine to sanitize supplied savepoint names. This is perhaps justifiable only for the sake of getting an immediate fix out; fortunately, since savepoint names are both arbitrary and transitory, restricting callers to names that match /^[a-zA-Z0-9_]*$/ isn't an undue burden.

> As we probably don't want to try to write our own escaping logic,
> copying the guts of PostgreSQL's PQescapeInternal() src/interfaces/libpq
> /fe-exec.c might make the most short-term sense - although that pulls in

I'm about a thousand miles away from this code, but if the method in
question really just wants an identifier, could we simply feed it an
MD5 of whatever the caller provides?

An MD5 would have to be tweaked to make sure that the first character is a letter, but otherwise, that would serve to protect against injections. There would be a small tradeoff -- somebody trying to debug complicated code that uses multiple savepoints would find a Pg statement log slightly less useful.

On 11.01.2013 07:09, Galen Charlton wrote:
> An MD5 would have to be tweaked to make sure that the first character is
> a letter, but otherwise, that would serve to protect against injections.
> There would be a small tradeoff -- somebody trying to debug complicated
> code that uses multiple savepoints would find a Pg statement log
> slightly less useful.

One could simply prepend the md5 with "savepoint_" which would remove
the need for tweaking the md5.
I'm not familiar with how and where evergreen uses savepoints, but I
would think that forcing alphanumeric savepoints would probably not
cause any restrictions.

s/restrictions/unreasonable restrictions/

I think the only part of the system that uses savepoints at all right now is booking, and I assume it does it via cstore.

We could, as a result, probably just say "savepoints aren't usable via pcrud" for the time being as a quick fix, then figure out how to properly escape them for exposing via pcrud later.

I've tested Dan's follow-up patch that has the sanitization routine take the Pg identifier maximum length into account. The security branch lp1098377_savepoint_name_sanity_tested contains both patches with our signoffs.

I've pushed a follow-up commit to lp1098377_savepoint_name_sanity_tested with a check to ensure that the savepoint name is non-null. Otherwise, null names produce segfaults.

I've tested and signed off on Bill's patch. I've force-pushed it along with a follow-up to protect savepoint.release and savepoint.rollback against null names to lp1098377_savepoint_name_sanity_tested.

Pushed the whole shebang back with sign-off on Galen's final commit to

security/lp1098377_savepoint_name_sanity_tested_signoff

Merged all the way back to 2.1