Skip to content

Commit

Permalink
Support mysql_clear_password auth switch. Fixes #268
Browse files Browse the repository at this point in the history 
For safety, sending the password in cleartext requires a secure
connection.
  • Loading branch information
@bgrainger
bgrainger committed Jun 1, 2017
1 parent fc5c777 commit 74cdcba
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 7 deletions.
7 changes: 5 additions & 2 deletions src/MySqlConnector/Serialization/ConnectionSettings.cs
View file
Expand Up @@ -59,9 +59,10 @@ public ConnectionSettings(MySqlConnectionStringBuilder csb)
UseCompression = csb.UseCompression;
}

public ConnectionSettings WithUseCompression(bool useCompression) => new ConnectionSettings(this, useCompression);
public ConnectionSettings WithSecureConnection(bool isSecureConnection) => new ConnectionSettings(this, isSecureConnection: isSecureConnection);
public ConnectionSettings WithUseCompression(bool useCompression) => new ConnectionSettings(this, useCompression: useCompression);

private ConnectionSettings(ConnectionSettings other, bool? useCompression)
private ConnectionSettings(ConnectionSettings other, bool? useCompression = null, bool? isSecureConnection = null)
{
// Base Options
ConnectionString = other.ConnectionString;
Expand All @@ -77,6 +78,7 @@ private ConnectionSettings(ConnectionSettings other, bool? useCompression)
SslMode = other.SslMode;
CertificateFile = other.CertificateFile;
CertificatePassword = other.CertificatePassword;
IsSecureConnection = isSecureConnection ?? other.IsSecureConnection;

// Connection Pooling Options
Pooling = other.Pooling;
Expand Down Expand Up @@ -115,6 +117,7 @@ private ConnectionSettings(ConnectionSettings other, bool? useCompression)
internal readonly MySqlSslMode SslMode;
internal readonly string CertificateFile;
internal readonly string CertificatePassword;
internal readonly bool IsSecureConnection;

// Connection Pooling Options
internal readonly bool Pooling;
Expand Down
24 changes: 19 additions & 5 deletions src/MySqlConnector/Serialization/MySqlSession.cs
View file
Expand Up @@ -231,6 +231,7 @@ public async Task ConnectAsync(ConnectionSettings cs, IOBehavior ioBehavior, Can
if (!serverSupportsSsl)
throw new MySqlException("Server does not support SSL");
await InitSslAsync(initialHandshake.ProtocolCapabilities, cs, ioBehavior, cancellationToken).ConfigureAwait(false);
cs = cs.WithSecureConnection(true);
}

var response = HandshakeResponse41Packet.Create(initialHandshake, cs);
Expand Down Expand Up @@ -286,12 +287,25 @@ private async Task SwitchAuthenticationAsync(PayloadData payload, ConnectionSett
{
// if the server didn't support the hashed password; rehash with the new challenge
var switchRequest = AuthenticationMethodSwitchRequestPayload.Create(payload);
if (switchRequest.Name != "mysql_native_password")
switch (switchRequest.Name)
{
case "mysql_native_password":
AuthPluginData = switchRequest.Data;
var hashedPassword = AuthenticationUtility.CreateAuthenticationResponse(AuthPluginData, 0, cs.Password);
payload = new PayloadData(new ArraySegment<byte>(hashedPassword));
await SendReplyAsync(payload, ioBehavior, cancellationToken).ConfigureAwait(false);
break;

case "mysql_clear_password":
if (!cs.IsSecureConnection)
throw new MySqlException("Authentication method '{0}' requires a secure connection.".FormatInvariant(switchRequest.Name));
payload = new PayloadData(new ArraySegment<byte>(Encoding.UTF8.GetBytes(cs.Password)));
await SendReplyAsync(payload, ioBehavior, cancellationToken).ConfigureAwait(false);
break;

default:
throw new NotSupportedException("Authentication method '{0}' is not supported.".FormatInvariant(switchRequest.Name));
AuthPluginData = switchRequest.Data;
var hashedPassword = AuthenticationUtility.CreateAuthenticationResponse(AuthPluginData, 0, cs.Password);
payload = new PayloadData(new ArraySegment<byte>(hashedPassword));
await SendReplyAsync(payload, ioBehavior, cancellationToken).ConfigureAwait(false);
}
}

public async Task<bool> TryPingAsync(IOBehavior ioBehavior, CancellationToken cancellationToken)
Expand Down

2 comments on commit 74cdcba

@caleblloyd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgrainger IsSecureConnection may be a bit misleading of a name for this.

No password, hashed or unhashed, should be sent over an untrusted, unencrypted connection. An eavesdropper that collects a hashed password could easily use a modified connector to gain access to the database if their IP was permitted.

Maybe AllowClearTextPassword or something along those lines would be better?

@bgrainger
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already improved this in 959ae7c (before I saw your comment).

The intent of the variable (which is now a field on MySqlSession) is to capture whether the current connection is actually secure. Only if it's true will the password be sent in clear text. If that's still unclear, I would like to improve it so it's not confusing.

Like I said on #268, we could require "opt in" by requiring a new AllowClearTextPassword=true connection string setting. I could be wrong, but I felt that a malicious proxy that MITMs the SSL connection in order to trick the client into negotiating mysql_clear_password is a low risk.

No password, hashed or unhashed, should be sent over an untrusted, unencrypted connection.

I'm not sure I completely understand what you're suggesting here, because the "default" MySQL connection sends a hashed password (secure mysql_native_password or insecure mysql_old_password (which we don't support)) over an unencrypted connection.

Please sign in to comment.