Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: audit protocol handling #3441

Merged
merged 40 commits into from
Aug 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
90dd39a
fix(ci): enable unit-tests for all relevant packages
abonander Aug 15, 2024
9bef397
fix: tests in `sqlx-postgres`
abonander Aug 15, 2024
5c595cb
fix(postgres): prevent integer overflow when binding arguments
abonander Aug 15, 2024
2ed868d
fix: audit `sqlx_postgres::types::rust_decimal` for overflowing casts
abonander Aug 15, 2024
e1f04cb
fix: audit `sqlx_postgres::types::bit_vec` for overflowing casts
abonander Aug 15, 2024
74e720e
fix: audit `sqlx_postgres::types::time` for overflowing casts
abonander Aug 15, 2024
13561cd
fix: audit `sqlx_postgres::types::chrono` for overflowing casts
abonander Aug 15, 2024
6599f1c
fix: audit `sqlx_postgres::types::cube` for overflowing casts
abonander Aug 16, 2024
63349de
chore: run `cargo fmt`
abonander Aug 16, 2024
dd92def
fix: audit `PgValueRef::get()` and usage sites for bad casts
abonander Aug 16, 2024
31fc4ed
fix: audit `sqlx_postgres::types::bit_vec` for casts involving sign loss
abonander Aug 16, 2024
de957f9
fix: audit `sqlx_postgres::types::rust_decimal` for casts involving s…
abonander Aug 16, 2024
072139a
fix: audit `PgNumeric` and usages for casts involving sign loss
abonander Aug 16, 2024
627c289
fix: audit `sqlx_postgres::type::int` for bad casts
abonander Aug 16, 2024
8a17e46
fix: audit `sqlx_postgres::types::hstore` for bad casts
abonander Aug 16, 2024
2cb6217
fix: audit `sqlx_postgres::types::array` for bad casts
abonander Aug 16, 2024
ec5326e
refactor: rename `sqlx_core::io::{Encode, Decode}` for clarity
abonander Aug 17, 2024
c2f7339
refactor(postgres): make better use of traits to improve protocol han…
abonander Aug 17, 2024
b783dbd
fix(mysql): fallout from ec5326e5
abonander Aug 17, 2024
15fc55c
chore(mysql): deny bad-cast lints
abonander Aug 18, 2024
a9510c8
fix(mysql): audit for bad casts
abonander Aug 20, 2024
0d9f2c8
chore: configure clippy cast lints at workspace level
abonander Aug 20, 2024
8d63ec7
fix(sqlite): forward optional features correctly
abonander Aug 20, 2024
d121704
fix: use same fix for the same cast in `Migrate::apply()` everywhere
abonander Aug 20, 2024
445b789
fix(sqlite): audit for bad casts
abonander Aug 20, 2024
2e9ba07
fix(postgres): dead code `StatementId::UNNAMED`
abonander Aug 21, 2024
d3e75e7
fix(postgres): decode `PgDatabaseError` for `ErrorResponse`
abonander Aug 21, 2024
8db2055
chore(postgres): include nullables query in error
abonander Aug 21, 2024
e731cfd
fix(postgres): syntax error in nullables query
abonander Aug 21, 2024
37f53cc
fix(postgres): syntax error in EXPLAIN query
abonander Aug 21, 2024
59f5cd0
fix(mysql): correct `Capabilities` assertions in unit tests
abonander Aug 21, 2024
65feda5
fix(mysql): add `sqlx` as a dev-dependency for doctests
abonander Aug 22, 2024
98fe7ca
fix(mysql): fix doctests
abonander Aug 22, 2024
0c1ff60
fix(sqlite): fix unit and doctests
abonander Aug 23, 2024
5889f76
fix(postgres): fix missing inversion on `PgNumeric::is_valid_digit()`
abonander Aug 23, 2024
6aaec27
fix(mysql): don't use an arbitrary `cfg` for one test
abonander Aug 24, 2024
985392e
fix(postgres): use checked decrement on `pending_ready_for_query_count`
abonander Aug 24, 2024
f9e5176
chore(postgres): create regression test for RUSTSEC-2024-0363
abonander Aug 23, 2024
127d617
chore(sqlite): create regression test for RUSTSEC-2024-0363
abonander Aug 24, 2024
7e48dbe
chore(mysql): create regression test for RUSTSEC-2024-0363
abonander Aug 24, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 42 additions & 10 deletions .github/workflows/sqlx.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,55 @@ jobs:
- run: cargo build --all-features

test:
name: Unit Test
name: Unit Tests
runs-on: ubuntu-22.04
strategy:
matrix:
runtime: [async-std, tokio]
tls: [native-tls, rustls-aws-lc-rs, rustls-ring, none]
steps:
- uses: actions/checkout@v4

- uses: Swatinem/rust-cache@v2
with:
key: ${{ runner.os }}-test

- run: >
- name: Install Rust
run: rustup update

- name: Test sqlx-core
run: >
cargo test
-p sqlx-core
--all-features

- name: Test sqlx-mysql
run: >
cargo test
-p sqlx-mysql
--all-features

- name: Test sqlx-postgres
run: >
cargo test
-p sqlx-postgres
--all-features

- name: Test sqlx-sqlite
run: >
cargo test
-p sqlx-sqlite
--all-features

- name: Test sqlx-macros-core
run: >
cargo test
-p sqlx-macros-core
--all-features

# Note: use `--lib` to not run integration tests that require a DB
- name: Test sqlx
run: >
cargo test
--manifest-path sqlx-core/Cargo.toml
--features json,_rt-${{ matrix.runtime }},_tls-${{ matrix.tls }}
-p sqlx
--lib
--all-features

sqlite:
name: SQLite
Expand Down Expand Up @@ -172,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
Expand All @@ -184,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
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,15 @@ criterion = { version = "0.5.1", features = ["async_tokio"] }
# Enable testing with SQLCipher if specifically requested.
libsqlite3-sys = { version = "0.30.1", features = ["bundled-sqlcipher"] }

# Common lint settings for the workspace
[workspace.lints.clippy]
# https://github.com/launchbadge/sqlx/issues/3440
cast_possible_truncation = 'deny'
cast_possible_wrap = 'deny'
cast_sign_loss = 'deny'
# See `clippy.toml`
disallowed_methods = 'deny'

#
# Any
#
Expand Down Expand Up @@ -264,6 +273,11 @@ name = "sqlite-migrate"
path = "tests/sqlite/migrate.rs"
required-features = ["sqlite", "macros", "migrate"]

[[test]]
name = "sqlite-rustsec"
path = "tests/sqlite/rustsec.rs"
required-features = ["sqlite"]

[[bench]]
name = "sqlite-describe"
path = "benches/sqlite/describe.rs"
Expand Down Expand Up @@ -314,6 +328,11 @@ name = "mysql-migrate"
path = "tests/mysql/migrate.rs"
required-features = ["mysql", "macros", "migrate"]

[[test]]
name = "mysql-rustsec"
path = "tests/mysql/rustsec.rs"
required-features = ["mysql"]

#
# PostgreSQL
#
Expand Down Expand Up @@ -363,3 +382,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"]
3 changes: 3 additions & 0 deletions sqlx-bench/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,6 @@ required-features = ["postgres"]
name = "sqlite_fetch_all"
harness = false
required-features = ["sqlite"]

[lints]
workspace = true
3 changes: 3 additions & 0 deletions sqlx-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,6 @@ completions = ["dep:clap_complete"]
[dev-dependencies]
assert_cmd = "2.0.11"
tempfile = "3.10.1"

[lints]
workspace = true
3 changes: 3 additions & 0 deletions sqlx-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,6 @@ hashbrown = "0.14.5"
[dev-dependencies]
sqlx = { workspace = true, features = ["postgres", "sqlite", "mysql", "migrate", "macros", "time", "uuid"] }
tokio = { version = "1", features = ["rt"] }

[lints]
workspace = true
17 changes: 11 additions & 6 deletions sqlx-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,11 +312,16 @@ impl From<crate::migrate::MigrateError> for Error {
/// Format an error message as a `Protocol` error
#[macro_export]
macro_rules! err_protocol {
($expr:expr) => {
$crate::error::Error::Protocol($expr.into())
};

($fmt:expr, $($arg:tt)*) => {
$crate::error::Error::Protocol(format!($fmt, $($arg)*))
($($fmt_args:tt)*) => {
$crate::error::Error::Protocol(
format!(
"{} ({}:{})",
// Note: the format string needs to be unmodified (e.g. by `concat!()`)
// for implicit formatting arguments to work
format_args!($($fmt_args)*),
module_path!(),
line!(),
)
)
};
}
8 changes: 4 additions & 4 deletions sqlx-core/src/io/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,27 @@ use bytes::Bytes;

use crate::error::Error;

pub trait Decode<'de, Context = ()>
pub trait ProtocolDecode<'de, Context = ()>
where
Self: Sized,
{
fn decode(buf: Bytes) -> Result<Self, Error>
where
Self: Decode<'de, ()>,
Self: ProtocolDecode<'de, ()>,
{
Self::decode_with(buf, ())
}

fn decode_with(buf: Bytes, context: Context) -> Result<Self, Error>;
}

impl Decode<'_> for Bytes {
impl ProtocolDecode<'_> for Bytes {
fn decode_with(buf: Bytes, _: ()) -> Result<Self, Error> {
Ok(buf)
}
}

impl Decode<'_> for () {
impl ProtocolDecode<'_> for () {
fn decode_with(_: Bytes, _: ()) -> Result<(), Error> {
Ok(())
}
Expand Down
15 changes: 8 additions & 7 deletions sqlx-core/src/io/encode.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
pub trait Encode<'en, Context = ()> {
fn encode(&self, buf: &mut Vec<u8>)
pub trait ProtocolEncode<'en, Context = ()> {
fn encode(&self, buf: &mut Vec<u8>) -> Result<(), crate::Error>
where
Self: Encode<'en, ()>,
Self: ProtocolEncode<'en, ()>,
{
self.encode_with(buf, ());
self.encode_with(buf, ())
}

fn encode_with(&self, buf: &mut Vec<u8>, context: Context);
fn encode_with(&self, buf: &mut Vec<u8>, context: Context) -> Result<(), crate::Error>;
}

impl<'en, C> Encode<'en, C> for &'_ [u8] {
fn encode_with(&self, buf: &mut Vec<u8>, _: C) {
impl<'en, C> ProtocolEncode<'en, C> for &'_ [u8] {
fn encode_with(&self, buf: &mut Vec<u8>, _context: C) -> Result<(), crate::Error> {
buf.extend_from_slice(self);
Ok(())
}
}
4 changes: 2 additions & 2 deletions sqlx-core/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ mod read_buf;
pub use buf::BufExt;
pub use buf_mut::BufMutExt;
//pub use buf_stream::BufStream;
pub use decode::Decode;
pub use encode::Encode;
pub use decode::ProtocolDecode;
pub use encode::ProtocolEncode;
pub use read_buf::ReadBuf;

#[cfg(not(feature = "_rt-tokio"))]
Expand Down
2 changes: 0 additions & 2 deletions sqlx-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
#![recursion_limit = "512"]
#![warn(future_incompatible, rust_2018_idioms)]
#![allow(clippy::needless_doctest_main, clippy::type_complexity)]
// See `clippy.toml` at the workspace root
#![deny(clippy::disallowed_methods)]
// The only unsafe code in SQLx is that necessary to interact with native APIs like with SQLite,
// and that can live in its own separate driver crate.
#![forbid(unsafe_code)]
Expand Down
20 changes: 12 additions & 8 deletions sqlx-core/src/net/socket/buffered.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{cmp, io};

use crate::error::Error;

use crate::io::{AsyncRead, AsyncReadExt, Decode, Encode};
use crate::io::{AsyncRead, AsyncReadExt, ProtocolDecode, ProtocolEncode};

// Tokio, async-std, and std all use this as the default capacity for their buffered I/O.
const DEFAULT_BUF_SIZE: usize = 8192;
Expand Down Expand Up @@ -59,32 +59,36 @@ impl<S: Socket> BufferedSocket<S> {

pub async fn read<'de, T>(&mut self, byte_len: usize) -> Result<T, Error>
where
T: Decode<'de, ()>,
T: ProtocolDecode<'de, ()>,
{
self.read_with(byte_len, ()).await
}

pub async fn read_with<'de, T, C>(&mut self, byte_len: usize, context: C) -> Result<T, Error>
where
T: Decode<'de, C>,
T: ProtocolDecode<'de, C>,
{
T::decode_with(self.read_buffered(byte_len).await?.freeze(), context)
}

pub fn write<'en, T>(&mut self, value: T)
#[inline(always)]
pub fn write<'en, T>(&mut self, value: T) -> Result<(), Error>
where
T: Encode<'en, ()>,
T: ProtocolEncode<'en, ()>,
{
self.write_with(value, ())
}

pub fn write_with<'en, T, C>(&mut self, value: T, context: C)
#[inline(always)]
pub fn write_with<'en, T, C>(&mut self, value: T, context: C) -> Result<(), Error>
where
T: Encode<'en, C>,
T: ProtocolEncode<'en, C>,
{
value.encode_with(self.write_buf.buf_mut(), context);
value.encode_with(self.write_buf.buf_mut(), context)?;
self.write_buf.bytes_written = self.write_buf.buf.len();
self.write_buf.sanity_check();

Ok(())
}

pub async fn flush(&mut self) -> io::Result<()> {
Expand Down
3 changes: 3 additions & 0 deletions sqlx-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,6 @@ sqlx-macros-core = { workspace = true }
proc-macro2 = { version = "1.0.36", default-features = false }
syn = { version = "2.0.52", default-features = false, features = ["parsing", "proc-macro"] }
quote = { version = "1.0.26", default-features = false }

[lints]
workspace = true
13 changes: 13 additions & 0 deletions sqlx-mysql/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ any = ["sqlx-core/any"]
offline = ["sqlx-core/offline", "serde/derive"]
migrate = ["sqlx-core/migrate"]

# Type Integration features
bigdecimal = ["dep:bigdecimal", "sqlx-core/bigdecimal"]
chrono = ["dep:chrono", "sqlx-core/chrono"]
rust_decimal = ["dep:rust_decimal", "rust_decimal/maths", "sqlx-core/rust_decimal"]
time = ["dep:time", "sqlx-core/time"]
uuid = ["dep:uuid", "sqlx-core/uuid"]

[dependencies]
sqlx-core = { workspace = true }

Expand Down Expand Up @@ -64,3 +71,9 @@ tracing = { version = "0.1.37", features = ["log"] }
whoami = { version = "1.2.1", default-features = false }

serde = { version = "1.0.144", optional = true }

[dev-dependencies]
sqlx = { workspace = true, features = ["mysql"] }

[lints]
workspace = true
2 changes: 2 additions & 0 deletions sqlx-mysql/src/any.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ impl<'a> TryFrom<&'a AnyConnectOptions> for MySqlConnectOptions {
fn map_result(result: MySqlQueryResult) -> AnyQueryResult {
AnyQueryResult {
rows_affected: result.rows_affected,
// Don't expect this to be a problem
#[allow(clippy::cast_possible_wrap)]
last_insert_id: Some(result.last_insert_id as i64),
}
}
4 changes: 2 additions & 2 deletions sqlx-mysql/src/connection/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl AuthPlugin {
0x04 => {
let payload = encrypt_rsa(stream, 0x02, password, nonce).await?;

stream.write_packet(&*payload);
stream.write_packet(&*payload)?;
stream.flush().await?;

Ok(false)
Expand Down Expand Up @@ -143,7 +143,7 @@ async fn encrypt_rsa<'s>(
}

// client sends a public key request
stream.write_packet(&[public_key_request_id][..]);
stream.write_packet(&[public_key_request_id][..])?;
stream.flush().await?;

// server sends a public key response
Expand Down
4 changes: 2 additions & 2 deletions sqlx-mysql/src/connection/establish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ impl<'a> DoHandshake<'a> {
database: options.database.as_deref(),
auth_plugin: plugin,
auth_response: auth_response.as_deref(),
});
})?;

stream.flush().await?;

Expand Down Expand Up @@ -160,7 +160,7 @@ impl<'a> DoHandshake<'a> {
)
.await?;

stream.write_packet(AuthSwitchResponse(response));
stream.write_packet(AuthSwitchResponse(response))?;
stream.flush().await?;
}

Expand Down
4 changes: 3 additions & 1 deletion sqlx-mysql/src/connection/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ impl MySqlConnection {
// otherwise, this first packet is the start of the result-set metadata,
*self.inner.stream.waiting.front_mut().unwrap() = Waiting::Row;

let num_columns = packet.get_uint_lenenc() as usize; // column count
let num_columns = packet.get_uint_lenenc(); // column count
let num_columns = usize::try_from(num_columns)
.map_err(|_| err_protocol!("column count overflows usize: {num_columns}"))?;

if needs_metadata {
column_names = Arc::new(recv_result_metadata(&mut self.inner.stream, num_columns, Arc::make_mut(&mut columns)).await?);
Expand Down
Loading