From 733cfbf7ccde74dfde708b884f6684b41b455661 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Wed, 7 Jun 2023 14:26:23 -0700 Subject: [PATCH 1/6] mysql_clear_password --- sqlx-mysql/src/connection/auth.rs | 6 +++++ sqlx-mysql/src/protocol/auth.rs | 3 +++ .../src/protocol/connect/auth_switch.rs | 24 ++++++++++++------- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/sqlx-mysql/src/connection/auth.rs b/sqlx-mysql/src/connection/auth.rs index c533c90812..1ef32c1d03 100644 --- a/sqlx-mysql/src/connection/auth.rs +++ b/sqlx-mysql/src/connection/auth.rs @@ -27,6 +27,12 @@ impl AuthPlugin { // https://mariadb.com/kb/en/sha256_password-plugin/ AuthPlugin::Sha256Password => encrypt_rsa(stream, 0x01, password, nonce).await, + + AuthPlugin::MySqlClearPassword => { + let mut pw_bytes = password.as_bytes().to_owned(); + pw_bytes.push(0); // null terminate + Ok(pw_bytes) + }, } } diff --git a/sqlx-mysql/src/protocol/auth.rs b/sqlx-mysql/src/protocol/auth.rs index 261a8817ab..091831e506 100644 --- a/sqlx-mysql/src/protocol/auth.rs +++ b/sqlx-mysql/src/protocol/auth.rs @@ -7,6 +7,7 @@ pub enum AuthPlugin { MySqlNativePassword, CachingSha2Password, Sha256Password, + MySqlClearPassword, } impl AuthPlugin { @@ -15,6 +16,7 @@ impl AuthPlugin { AuthPlugin::MySqlNativePassword => "mysql_native_password", AuthPlugin::CachingSha2Password => "caching_sha2_password", AuthPlugin::Sha256Password => "sha256_password", + AuthPlugin::MySqlClearPassword => "mysql_clear_password", } } } @@ -27,6 +29,7 @@ impl FromStr for AuthPlugin { "mysql_native_password" => Ok(AuthPlugin::MySqlNativePassword), "caching_sha2_password" => Ok(AuthPlugin::CachingSha2Password), "sha256_password" => Ok(AuthPlugin::Sha256Password), + "mysql_clear_password" => Ok(AuthPlugin::MySqlClearPassword), _ => Err(err_protocol!("unknown authentication plugin: {}", s)), } diff --git a/sqlx-mysql/src/protocol/connect/auth_switch.rs b/sqlx-mysql/src/protocol/connect/auth_switch.rs index 0c25a42500..2940f7d329 100644 --- a/sqlx-mysql/src/protocol/connect/auth_switch.rs +++ b/sqlx-mysql/src/protocol/connect/auth_switch.rs @@ -27,14 +27,22 @@ impl Decode<'_> for AuthSwitchRequest { let plugin = buf.get_str_nul()?.parse()?; // See: https://github.com/mysql/mysql-server/blob/ea7d2e2d16ac03afdd9cb72a972a95981107bf51/sql/auth/sha2_password.cc#L942 - if buf.len() != 21 { - return Err(err_protocol!( - "expected 21 bytes but found {} bytes", - buf.len() - )); - } - let data = buf.get_bytes(20); - buf.advance(1); // NUL-terminator + let data = if buf.len() != 21 { + if matches!(plugin, AuthPlugin::MySqlClearPassword) { + // Contrary to the MySQL protocol, AWS Aurora with IAM sends + // no data. + Bytes::new() + } else { + return Err(err_protocol!( + "expected 21 bytes but found {} bytes", + buf.len() + )); + } + } else { + let data = buf.get_bytes(20); + buf.advance(1); // NUL-terminator + data + }; Ok(Self { plugin, data }) } From 87a91f6e295aa3b19bc3447472f63135bc4ba162 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Wed, 7 Jun 2023 14:29:20 -0700 Subject: [PATCH 2/6] comment --- sqlx-mysql/src/protocol/connect/auth_switch.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlx-mysql/src/protocol/connect/auth_switch.rs b/sqlx-mysql/src/protocol/connect/auth_switch.rs index 2940f7d329..9f3da0a95d 100644 --- a/sqlx-mysql/src/protocol/connect/auth_switch.rs +++ b/sqlx-mysql/src/protocol/connect/auth_switch.rs @@ -30,7 +30,8 @@ impl Decode<'_> for AuthSwitchRequest { let data = if buf.len() != 21 { if matches!(plugin, AuthPlugin::MySqlClearPassword) { // Contrary to the MySQL protocol, AWS Aurora with IAM sends - // no data. + // no data. That is fine because the MySQL protocol says to + // ignore any data sent. Bytes::new() } else { return Err(err_protocol!( From 43d2d1eb72f780d880a41eb2132829628ede2636 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Wed, 7 Jun 2023 16:02:01 -0700 Subject: [PATCH 3/6] refactor no-data edge case and add unit tests --- .../src/protocol/connect/auth_switch.rs | 56 +++++++++++++------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/sqlx-mysql/src/protocol/connect/auth_switch.rs b/sqlx-mysql/src/protocol/connect/auth_switch.rs index 9f3da0a95d..558fda7760 100644 --- a/sqlx-mysql/src/protocol/connect/auth_switch.rs +++ b/sqlx-mysql/src/protocol/connect/auth_switch.rs @@ -26,24 +26,26 @@ impl Decode<'_> for AuthSwitchRequest { let plugin = buf.get_str_nul()?.parse()?; + if matches!(plugin, AuthPlugin::MySqlClearPassword) && buf.is_empty() { + // Contrary to the MySQL protocol, AWS Aurora with IAM sends + // no data. That is fine because the mysql_clear_password says to + // ignore any data sent. + // See: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_connection_phase_authentication_methods_clear_text_password.html + return Ok(Self { + plugin, + data: Bytes::new(), + }); + } + // See: https://github.com/mysql/mysql-server/blob/ea7d2e2d16ac03afdd9cb72a972a95981107bf51/sql/auth/sha2_password.cc#L942 - let data = if buf.len() != 21 { - if matches!(plugin, AuthPlugin::MySqlClearPassword) { - // Contrary to the MySQL protocol, AWS Aurora with IAM sends - // no data. That is fine because the MySQL protocol says to - // ignore any data sent. - Bytes::new() - } else { - return Err(err_protocol!( - "expected 21 bytes but found {} bytes", - buf.len() - )); - } - } else { - let data = buf.get_bytes(20); - buf.advance(1); // NUL-terminator - data - }; + if buf.len() != 21 { + return Err(err_protocol!( + "expected 21 bytes but found {} bytes", + buf.len() + )); + } + let data = buf.get_bytes(20); + buf.advance(1); // NUL-terminator Ok(Self { plugin, data }) } @@ -57,3 +59,23 @@ impl Encode<'_, Capabilities> for AuthSwitchResponse { buf.extend_from_slice(&self.0); } } + +#[test] +fn test_decode_auth_switch_packet_data() { + const AUTH_SWITCH_NO_DATA: &[u8] = b"\xfecaching_sha2_password\x00abcdefghijabcdefghij\x00"; + + let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), ()).unwrap(); + + assert!(matches!(p.plugin, AuthPlugin::CachingSha2Password)); + assert_eq!(p.data, &b"abcdefghijabcdefghij"[..]); +} + +#[test] +fn test_decode_auth_switch_packet_no_data() { + const AUTH_SWITCH_NO_DATA: &[u8] = b"\xfemysql_clear_password\x00"; + + let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), ()).unwrap(); + + assert!(matches!(p.plugin, AuthPlugin::MySqlClearPassword)); + assert_eq!(p.data, Bytes::new()); +} From a10ba312e05193df542eeaa8eb2cd6ba62333fed Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Wed, 7 Jun 2023 16:22:17 -0700 Subject: [PATCH 4/6] add a connection option to explicitly enable mysql_clear_password --- sqlx-mysql/src/connection/establish.rs | 2 +- sqlx-mysql/src/options/mod.rs | 13 ++++++++++++ .../src/protocol/connect/auth_switch.rs | 21 +++++++++++++++---- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/sqlx-mysql/src/connection/establish.rs b/sqlx-mysql/src/connection/establish.rs index 75838c57f5..00f8c1980a 100644 --- a/sqlx-mysql/src/connection/establish.rs +++ b/sqlx-mysql/src/connection/establish.rs @@ -134,7 +134,7 @@ impl<'a> DoHandshake<'a> { } 0xfe => { - let switch: AuthSwitchRequest = packet.decode()?; + let switch: AuthSwitchRequest = packet.decode_with(self.options.enable_cleartext_plugin)?; plugin = Some(switch.plugin); let nonce = switch.data.chain(Bytes::new()); diff --git a/sqlx-mysql/src/options/mod.rs b/sqlx-mysql/src/options/mod.rs index dc06380b07..8b75d92e2a 100644 --- a/sqlx-mysql/src/options/mod.rs +++ b/sqlx-mysql/src/options/mod.rs @@ -68,6 +68,7 @@ pub struct MySqlConnectOptions { pub(crate) collation: Option, pub(crate) log_settings: LogSettings, pub(crate) pipes_as_concat: bool, + pub(crate) enable_cleartext_plugin: bool, } impl Default for MySqlConnectOptions { @@ -95,6 +96,7 @@ impl MySqlConnectOptions { statement_cache_capacity: 100, log_settings: Default::default(), pipes_as_concat: true, + enable_cleartext_plugin: false, } } @@ -258,4 +260,15 @@ impl MySqlConnectOptions { self.pipes_as_concat = flag_val; self } + + /// Enables mysql_clear_password plugin support. + /// + /// Security Note: + /// Sending passwords as cleartext may be a security problem in some + /// configurations. Please consider using TLS or encrypted tunnels for + /// server connection. + pub fn enable_cleartext_plugin(mut self, flag_val: bool) -> Self { + self.enable_cleartext_plugin = flag_val; + self + } } diff --git a/sqlx-mysql/src/protocol/connect/auth_switch.rs b/sqlx-mysql/src/protocol/connect/auth_switch.rs index 558fda7760..28b8119c57 100644 --- a/sqlx-mysql/src/protocol/connect/auth_switch.rs +++ b/sqlx-mysql/src/protocol/connect/auth_switch.rs @@ -14,8 +14,8 @@ pub struct AuthSwitchRequest { pub data: Bytes, } -impl Decode<'_> for AuthSwitchRequest { - fn decode_with(mut buf: Bytes, _: ()) -> Result { +impl Decode<'_, bool> for AuthSwitchRequest { + fn decode_with(mut buf: Bytes, enable_cleartext_plugin: bool) -> Result { let header = buf.get_u8(); if header != 0xfe { return Err(err_protocol!( @@ -26,6 +26,10 @@ impl Decode<'_> for AuthSwitchRequest { let plugin = buf.get_str_nul()?.parse()?; + if matches!(plugin, AuthPlugin::MySqlClearPassword) && !enable_cleartext_plugin { + return Err(err_protocol!("mysql_cleartext_plugin disabled")); + } + if matches!(plugin, AuthPlugin::MySqlClearPassword) && buf.is_empty() { // Contrary to the MySQL protocol, AWS Aurora with IAM sends // no data. That is fine because the mysql_clear_password says to @@ -64,17 +68,26 @@ impl Encode<'_, Capabilities> for AuthSwitchResponse { fn test_decode_auth_switch_packet_data() { const AUTH_SWITCH_NO_DATA: &[u8] = b"\xfecaching_sha2_password\x00abcdefghijabcdefghij\x00"; - let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), ()).unwrap(); + let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), true).unwrap(); assert!(matches!(p.plugin, AuthPlugin::CachingSha2Password)); assert_eq!(p.data, &b"abcdefghijabcdefghij"[..]); } +#[test] +fn test_decode_auth_switch_cleartext_disabled() { + const AUTH_SWITCH_CLEARTEXT: &[u8] = b"\xfemysql_clear_password\x00abcdefghijabcdefghij\x00"; + + let e = AuthSwitchRequest::decode_with(AUTH_SWITCH_CLEARTEXT.into(), false).unwrap_err(); + + assert_eq!(e.to_string(), "encountered unexpected or invalid data: mysql_cleartext_plugin disabled"); +} + #[test] fn test_decode_auth_switch_packet_no_data() { const AUTH_SWITCH_NO_DATA: &[u8] = b"\xfemysql_clear_password\x00"; - let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), ()).unwrap(); + let p = AuthSwitchRequest::decode_with(AUTH_SWITCH_NO_DATA.into(), true).unwrap(); assert!(matches!(p.plugin, AuthPlugin::MySqlClearPassword)); assert_eq!(p.data, Bytes::new()); From abe4472043cd1075588ce27ba3127a8cc9971fc8 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Mon, 12 Jun 2023 13:08:39 -0700 Subject: [PATCH 5/6] cargo fmt --- sqlx-mysql/src/connection/auth.rs | 2 +- sqlx-mysql/src/connection/establish.rs | 3 ++- sqlx-mysql/src/protocol/connect/auth_switch.rs | 5 ++++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/sqlx-mysql/src/connection/auth.rs b/sqlx-mysql/src/connection/auth.rs index 1ef32c1d03..dce04cf843 100644 --- a/sqlx-mysql/src/connection/auth.rs +++ b/sqlx-mysql/src/connection/auth.rs @@ -32,7 +32,7 @@ impl AuthPlugin { let mut pw_bytes = password.as_bytes().to_owned(); pw_bytes.push(0); // null terminate Ok(pw_bytes) - }, + } } } diff --git a/sqlx-mysql/src/connection/establish.rs b/sqlx-mysql/src/connection/establish.rs index 00f8c1980a..2b183ba356 100644 --- a/sqlx-mysql/src/connection/establish.rs +++ b/sqlx-mysql/src/connection/establish.rs @@ -134,7 +134,8 @@ impl<'a> DoHandshake<'a> { } 0xfe => { - let switch: AuthSwitchRequest = packet.decode_with(self.options.enable_cleartext_plugin)?; + let switch: AuthSwitchRequest = + packet.decode_with(self.options.enable_cleartext_plugin)?; plugin = Some(switch.plugin); let nonce = switch.data.chain(Bytes::new()); diff --git a/sqlx-mysql/src/protocol/connect/auth_switch.rs b/sqlx-mysql/src/protocol/connect/auth_switch.rs index 28b8119c57..58b7fbb2ef 100644 --- a/sqlx-mysql/src/protocol/connect/auth_switch.rs +++ b/sqlx-mysql/src/protocol/connect/auth_switch.rs @@ -80,7 +80,10 @@ fn test_decode_auth_switch_cleartext_disabled() { let e = AuthSwitchRequest::decode_with(AUTH_SWITCH_CLEARTEXT.into(), false).unwrap_err(); - assert_eq!(e.to_string(), "encountered unexpected or invalid data: mysql_cleartext_plugin disabled"); + assert_eq!( + e.to_string(), + "encountered unexpected or invalid data: mysql_cleartext_plugin disabled" + ); } #[test] From 6afa87ccef89b3e611ae8f080817aaa8ea48c7b6 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Mon, 17 Jul 2023 14:38:54 -0400 Subject: [PATCH 6/6] scary comment & log warning --- sqlx-mysql/src/connection/establish.rs | 11 ++++++++++- sqlx-mysql/src/options/mod.rs | 8 ++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/sqlx-mysql/src/connection/establish.rs b/sqlx-mysql/src/connection/establish.rs index 2b183ba356..d7ffc048f7 100644 --- a/sqlx-mysql/src/connection/establish.rs +++ b/sqlx-mysql/src/connection/establish.rs @@ -11,7 +11,7 @@ use crate::protocol::connect::{ AuthSwitchRequest, AuthSwitchResponse, Handshake, HandshakeResponse, }; use crate::protocol::Capabilities; -use crate::{MySqlConnectOptions, MySqlConnection}; +use crate::{MySqlConnectOptions, MySqlConnection, MySqlSslMode}; impl MySqlConnection { pub(crate) async fn establish(options: &MySqlConnectOptions) -> Result { @@ -49,6 +49,15 @@ impl<'a> DoHandshake<'a> { .transpose()? .unwrap_or_else(|| charset.default_collation()); + if options.enable_cleartext_plugin + && matches!( + options.ssl_mode, + MySqlSslMode::Disabled | MySqlSslMode::Preferred + ) + { + log::warn!("Security warning: sending cleartext passwords without requiring SSL"); + } + Ok(Self { options, charset, diff --git a/sqlx-mysql/src/options/mod.rs b/sqlx-mysql/src/options/mod.rs index 8b75d92e2a..0585f7cfe7 100644 --- a/sqlx-mysql/src/options/mod.rs +++ b/sqlx-mysql/src/options/mod.rs @@ -265,8 +265,12 @@ impl MySqlConnectOptions { /// /// Security Note: /// Sending passwords as cleartext may be a security problem in some - /// configurations. Please consider using TLS or encrypted tunnels for - /// server connection. + /// configurations. Without additional defensive configuration like + /// ssl-mode=VERIFY_IDENTITY, an attacker can compromise a router + /// and trick the application into divulging its credentials. + /// + /// It is strongly recommended to set `.ssl_mode` to `Required`, + /// `VerifyCa`, or `VerifyIdentity` when enabling cleartext plugin. pub fn enable_cleartext_plugin(mut self, flag_val: bool) -> Self { self.enable_cleartext_plugin = flag_val; self