SQL injection in function `explainQuery`?

[hartwork]

Hi!

This was already part of a public discussion at the PostgreSQL Berlin meetup and I found no directions in this repository on where to send reports on potential vulnerabilities instead, so I hope it's okay to bring the following up here.

There is a place in this code base that (without any in-depth analysis) looks like it could be vulnerable to SQL injections:

agent/apps/dbagent/src/lib/targetdb/db.ts

Lines 390 to 391 in 30aecbc

	await client.query(`SET search_path TO ${schema}`);
	const explainQuery = `EXPLAIN ${query}`;

Can you confirm? Does it mean that a malicious or snitchy LLM could exploit explainQuery to run arbitrary queries on the connected database? Is there anything protecting against this that I might be missing?

Thanks and best, Sebastian

CC @divyenduz

[tsg]

Hi, thanks for raising this, it is indeed vulnerable. My intention when initially writing this was to ensure that it's only a single statement passed to EXPLAIN, so the result is an execution plan rather than actually executing the statement. Unfortunately EXPLAIN doesn't support parameters, so it has to be string manipulation. It is run in a read-only transaction, but because it currently doesn't protect against multiple statements, it's easy to escape.

The agent picks up the queries from pg_stat_statements or pg_stat_activity, where there are only normalized single statements. But if we assume the LLM is hijacked somehow, then it's possible that the explainQuery tool is called with a malicious string.

I think the best path forward is to sanitize the incoming query to be guaranteed a single statement, and potentially discard any queries that we're not sure about. As long as we can guarantee that the query contains a single statement, I think this should be safe because Postgres only plans the query, not actually execute it.

We still have to investigate a couple of things with this approach:

• stable functions might be executed by EXPLAIN, which could result in side-effects
• extensions can add planner hooks which could be problematic

[hartwork]

@tsg thanks for your response!

As long as we can guarantee that the query contains a single statement, I think this should be safe because Postgres only plans the query, not actually execute it.

Even beyond a single statement, I would not be surprised if things like denial of service are possible.

Even worse, EXPLAIN ANALYZE .. (and EXPLAIN (ANALYZE ..)?) could to be a concern, if nothing is blocking {query} from starting with flavurs of ANALYZE and the server is using PostgreSQL >=13. The official docs say:

Important

Keep in mind that the statement is actually executed when the ANALYZE option is used. Although EXPLAIN will discard any > output that a SELECT would return, other side effects of the statement will happen as usual.

I'm also wondering what SQL command would escape the BEGIN .. ROLLBACK wrap, either because the command is in general not rolled back by ROLLBACK — TRUNCATE or DROP maybe — or maybe even smuggling COMMIT via injection would be possible.

I'm not sure. Just thinking aloud.

[tsg]

Even worse, EXPLAIN ANALYZE .. (and EXPLAIN (ANALYZE ..)?) could to be a concern, if nothing is blocking {query} from starting with flavurs of ANALYZE and the server is using PostgreSQL >=13. The official docs say:

Yes, this does only EXPLAIN without ANALYZE, and we can check that it's an actual statement, not something that starts with ANALYZE or similar. This will take a bit of care, but I think it's doable. Especially since we can reject anything that's not normalized.

I'm also wondering what SQL command would escape the BEGIN .. ROLLBACK wrap, either because the command is in general not rolled back by ROLLBACK — TRUNCATE or DROP maybe — or maybe even smuggling COMMIT via injection would be possible.

I'd say the EXPLAIN wrap is more powerful than the transaction one, as long as we can guarantee single statement. We can keep the read-only transaction mostly as an extra layer, but if one can escape the EXPLAIN wrap, probably can avoid the transaction as well.

[divyenduz]

Thanks for raising this @hartwork. In addition to the improvements @tsg mentioned above, we implemented further improvements via #225

In summary,

1. We renamed the existing explainQuery to unsafeExplainQuery.
2. Created a new safeExplainQuery that takes a queryId as input and fetches the query directly from pg_stat_statements. Therefore, the model doesn't have an opportunity to be cynical.
3. Following abundance of caution, all the improvements mentioned above (single statement etc.) are also present in the safeExplainQuery.
4. The prompts already use safeExplainQuery, unsafeExplainQuery is not in our prompts anymore.

With this I am closing this issue but please feel free to reopen a new one with more feedback (or this one if you still feel it is not fully addressed).

Thank you again for taking the time to write this and helping us make Xata agent better. We really appreciate it 🙌🏼

[hartwork]

@divyenduz thanks for your work on this matter! I wonder what we know about the queries that make it into pg_stat_statements. I have one small follow-up bug report at #231.

[hartwork]

PS: I should note that issue #231 maybe well have security consequences because the code is used in attack prevention.