From b1539b60a13d0fb52e9d452ad43f4a4ad50ab3ec Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Fri, 23 Aug 2024 01:25:25 -0700 Subject: [PATCH] chore(postgres): create regression test for RUSTSEC-2024-0363 --- .github/workflows/sqlx.yml | 4 +- Cargo.toml | 5 + tests/postgres/fixtures/rustsec/2024_0363.sql | 4 + tests/postgres/rustsec.rs | 146 ++++++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 tests/postgres/fixtures/rustsec/2024_0363.sql create mode 100644 tests/postgres/rustsec.rs diff --git a/.github/workflows/sqlx.yml b/.github/workflows/sqlx.yml index 599b90c33a..508f036eba 100644 --- a/.github/workflows/sqlx.yml +++ b/.github/workflows/sqlx.yml @@ -204,7 +204,7 @@ jobs: - run: > cargo test --no-default-features - --features any,postgres,macros,_unstable-all-types,runtime-${{ matrix.runtime }},tls-${{ matrix.tls }} + --features any,postgres,macros,migrate,_unstable-all-types,runtime-${{ matrix.runtime }},tls-${{ matrix.tls }} env: DATABASE_URL: postgres://postgres:password@localhost:5432/sqlx SQLX_OFFLINE_DIR: .sqlx @@ -216,7 +216,7 @@ jobs: run: > cargo test --no-default-features - --features any,postgres,macros,_unstable-all-types,runtime-${{ matrix.runtime }},tls-${{ matrix.tls }} + --features any,postgres,macros,migrate,_unstable-all-types,runtime-${{ matrix.runtime }},tls-${{ matrix.tls }} env: DATABASE_URL: postgres://postgres:password@localhost:5432/sqlx?sslmode=verify-ca&sslrootcert=.%2Ftests%2Fcerts%2Fca.crt SQLX_OFFLINE_DIR: .sqlx diff --git a/Cargo.toml b/Cargo.toml index fad4f9b1f9..3345aac5f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -372,3 +372,8 @@ required-features = ["postgres", "macros", "migrate"] name = "postgres-query-builder" path = "tests/postgres/query_builder.rs" required-features = ["postgres"] + +[[test]] +name = "postgres-rustsec" +path = "tests/postgres/rustsec.rs" +required-features = ["postgres", "macros", "migrate"] diff --git a/tests/postgres/fixtures/rustsec/2024_0363.sql b/tests/postgres/fixtures/rustsec/2024_0363.sql new file mode 100644 index 0000000000..c3bb5b0920 --- /dev/null +++ b/tests/postgres/fixtures/rustsec/2024_0363.sql @@ -0,0 +1,4 @@ +-- https://rustsec.org/advisories/RUSTSEC-2024-0363.html +-- https://github.com/launchbadge/sqlx/issues/3440 +CREATE TABLE injection_target(id BIGSERIAL PRIMARY KEY, message TEXT); +INSERT INTO injection_target(message) VALUES ('existing value'); diff --git a/tests/postgres/rustsec.rs b/tests/postgres/rustsec.rs new file mode 100644 index 0000000000..45fd76b9db --- /dev/null +++ b/tests/postgres/rustsec.rs @@ -0,0 +1,146 @@ +use sqlx::{Error, PgPool}; + +use std::{cmp, str}; + +// https://rustsec.org/advisories/RUSTSEC-2024-0363.html +#[sqlx::test(migrations = false, fixtures("./fixtures/rustsec/2024_0363.sql"))] +async fn rustsec_2024_0363(pool: PgPool) -> anyhow::Result<()> { + let overflow_len = 4 * 1024 * 1024 * 1024; // 4 GiB + + // These three strings concatenated together will be the first query the Postgres backend "sees" + // + // Rather contrived because this already represents an injection vulnerability, + // but it's easier to demonstrate the bug with a simple `Query` message + // than the `Prepare` -> `Bind` -> `Execute` flow. + let real_query_prefix = "INSERT INTO injection_target(message) VALUES ('"; + let fake_message = "fake_msg') RETURNING id;\0"; + let real_query_suffix = "') RETURNING id"; + + // Our payload is another simple `Query` message + let real_payload = + "Q\0\0\0\x4DUPDATE injection_target SET message = 'you''ve been pwned!' WHERE id = 1\0"; + + // This is the value we want the length prefix to overflow to (including the length of the prefix itself) + // This will leave the backend's buffer pointing at our real payload. + let fake_payload_len = real_query_prefix.len() + fake_message.len() + 4; + + // Pretty easy to see that this should overflow to `fake_payload_len` + let target_payload_len = overflow_len + fake_payload_len; + + // This is the length we expect `injected_value` to be + let expected_inject_len = target_payload_len + - 4 // Length prefix + - real_query_prefix.len() + - (real_query_suffix.len() + 1 /* NUL terminator */); + + let pad_to_len = expected_inject_len - 5; // Header for FLUSH message that eats `real_query_suffix` (see below) + + let expected_payload_len = 4 // length prefix + + real_query_prefix.len() + + expected_inject_len + + real_query_suffix.len() + + 1; // NUL terminator + + let expected_wrapped_len = expected_payload_len % overflow_len; + assert_eq!(expected_wrapped_len, fake_payload_len); + + // This will be the string we inject into the query. + let mut injected_value = String::with_capacity(expected_inject_len); + + injected_value.push_str(fake_message); + injected_value.push_str(real_payload); + + // The Postgres backend reads the `FLUSH` message but ignores its contents. + // This gives us a variable-length NOP that lets us pad to the length we want, + // as well as a way to eat `real_query_suffix` without breaking the connection. + let flush_fill = "\0".repeat(9996); + + let flush_fmt_code = 'H'; // note: 'F' is `FunctionCall`. + + 'outer: while injected_value.len() < pad_to_len { + let remaining_len = pad_to_len - injected_value.len(); + + // The max length of a FLUSH message is 10,000, including the length prefix. + let flush_len = cmp::min( + remaining_len - 1, // minus format code + 10000, + ); + + // We need `flush_len` to be valid UTF-8 when encoded in big-endian + // in order to push it to the string. + // + // Not every value is going to be valid though, so we search for one that is. + 'inner: for flush_len in (4..=flush_len).rev() { + let flush_len_be = (flush_len as i32).to_be_bytes(); + + let Ok(flush_len_str) = str::from_utf8(&flush_len_be) else { + continue 'inner; + }; + + let fill_len = flush_len - 4; + + injected_value.push(flush_fmt_code); + injected_value.push_str(flush_len_str); + injected_value.push_str(&flush_fill[..fill_len]); + + continue 'outer; + } + + panic!("unable to find a valid encoding/split for {flush_len}"); + } + + assert_eq!(injected_value.len(), pad_to_len); + + // The amount of data the last FLUSH message has to eat + let eat_len = real_query_suffix.len() + 1; // plus NUL terminator + + // Push the FLUSH message that will eat `real_query_suffix` + injected_value.push(flush_fmt_code); + injected_value.push_str(str::from_utf8(&(eat_len as i32).to_be_bytes()).unwrap()); + // The value will be in the buffer already. + + assert_eq!(expected_inject_len, injected_value.len()); + + let query = format!("{real_query_prefix}{injected_value}{real_query_suffix}"); + + // The length of the `Query` message we've created + let final_payload_len = 4 // length prefix + + query.len() + + 1; // NUL terminator + + assert_eq!(expected_payload_len, final_payload_len); + + let wrapped_len = final_payload_len % overflow_len; + + assert_eq!(wrapped_len, fake_payload_len); + + let res = sqlx::raw_sql(&query) + // Note: the connection may hang afterward + // because `pending_ready_for_query_count` will underflow. + .execute(&pool) + .await; + + if let Err(e) = res { + // Connection rejected the query; we're happy. + if matches!(e, Error::Protocol(_)) { + return Ok(()); + } + + panic!("unexpected error: {e:?}"); + } + + let messages: Vec = + sqlx::query_scalar("SELECT message FROM injection_target ORDER BY id") + .fetch_all(&pool) + .await?; + + // If the injection succeeds, `messages` will look like: + // ["you've been pwned!'.to_string(), "fake_msg".to_string()] + assert_eq!( + messages, + ["existing message".to_string(), "fake_msg".to_string()] + ); + + // Injection didn't affect our database; we're happy. + Ok(()) +}