Prompting user for connection parameters to SQL Server

By Posted on

Problem

I try to avoid keeping passwords etc. in memory or in plain text anywhere. But I am on a huge time crunch and this will only be used internally this week then probably won’t get touched again. I just want to make sure this is secure and if not, what the risk is.

I’m mostly concerned about the password sitting in memory. I’m not sanitizing data but for this particular case it isn’t needed.

My specific questions are:

  1. From a security perspective, would this code be acceptable in a SQL Importer?

  2. Besides data sanitation, is there a best practice somewhere that I’m missing in this?

Console.WriteLine("Enter the Server Name. Ex: SQLMASTER"); //Gets the server to connect to, an IP address is also acceptable.
ServerName = Console.ReadLine();
Console.WriteLine("Enter the Database. Ex: Accounts"); //The actual database to use.
DatabaseName = Console.ReadLine();
Console.WriteLine("Use Windows authentication? Y/N"); //you will usually use this. 
YesNo = Console.ReadLine();

if (YesNo.ToLower() != "y")
{
    Console.WriteLine("Enter your Username (Domain name may be required). Ex: MyName");
    UserName = Console.ReadLine();
    Console.WriteLine("Enter your Password. Ex: ********");
    Password = ReadPassword();

    //Builds the connection string with the data we collected above. We're going to send this to the SQL Connection. (Username and Password)
    ConnectionString = "Data Source = " + ServerName + "; Initial Catalog = " + DatabaseName + "; User ID = " + UserName + "; Password = " + Password;

    Password = ""; //We do this to clear the password from memory
    UserName = ""; //Clearing username from memory
}
else
{
    //Builds the connection string with the data we collected above. We're going to send this to the SQL Connection. (Windows Authentication)
    ConnectionString = "Data Source = " + ServerName + "; Initial Catalog = " + DatabaseName + "; Integrated Security=SSPI"; //User ID = " + UserName + "; Password = " + Password;
}

//Create a new connection with above connection string
SQLCon = new SqlConnection(ConnectionString);
ConnectionString = ""; //Again, doing this to purge the credentials from memory

try
{
   SQLCon.Open();
   Console.WriteLine("nConnected!n");
   Console.WriteLine("Working...n");
   .... etc
}
catch (Exception ex)
{
   MessageBox.Show("Failed! See console for details");
   Console.WriteLine(ex.Message);
}

Solution

It doesn’t matter what you do with this code, the ConnectionString can always be accessed in the SqlConnection.

As a test, here’s what you should do:

Launch your application outside of Visual Studio. Take a new instance of Visual Studio and go to ‘Debug’ -> ‘Attach to Process…’ and then locate the process for your programme.

Once you have done this, run your programme until the SQL point, then hit the ‘Pause debugging’ button in Visual Studio.

Once you have done this, go to the Diagnostic Tools and take a memory snapshot. While it’s paused you can view the memory dump of the snapshot.

You should see a record appear in the ‘Memory Usage’ tab of the diagnostic tools, click the ‘Objects (Diff)’ link.

Take a snapshot

A new tab will open with a lot of fun stuff, order everything by ‘Object Type’ and find SqlConnection.

Double click your SqlConnection line.

Find the root object

Click the first line (assuming you have one) in the new portion of the window, then click ‘Referenced Objects’ at the bottom section of the window.

It should show you a bunch of objects, find SqlConnectionString, and drill down into it.

You should see several string objects. Go ahead and hover each one, one will be your password.

Of course, the string object on the SqlConnection itself will also be a connection string and have your password in it.

Extract your connection string

Bottom line: the client computer is always compromised in your eyes. You can do no more to secure it at this point, with an instance of Visual Studio (which this can be done more effectively with a separate programme that hides on the client computer) I can view your connection string and extract it with little to no effort required. (This whole process takes ~10 seconds once you have used the ‘Break All’ on the programme.)

My advice to you: don’t take unnecessary security precautions to ensure your work is ‘safe in memory’ — there’s no such thing. All memory can be dumped, and even if it’s only in memory for ~0.00001ms, an attacker can still find it. Just worry about building your application, let the memory security be handled by the user. 🙂

On to the code review:

  • YesNo is a bad variable name; C# rules are camelCase for local members. It should also be named something related to what it does: windowsCredentials.
  • You say data sanitization isn’t important, but what happens if the user enters a semicolon then another ConnectionString property? You should always validate input.
  • You should be using the SQLCon, not creating it like you are. (using (var connection = new SqlConnection(ConnectionString))), same with SqlCommand and SqlDataReader if/when/where you use them.

Why?

The SqlConnection type is an IDisposable – that means, it’s capable of being automatically closed and deallocated.

In the case of SqlConnection, IIRC there are unmanaged objects that need disposed, cleared and cleaned up before the connection is done. There are resources it uses that are not automatically freed.

If you look at the API of SqlConnection, it has a Close() method which indicates that it will disconnect from the database and free it’s resources. This is important because if you forget the Close() call, then your database connection will hang open. This has performance implications for your programme, database and network.

So, we use using (...) construct for it which is a fancy way of writing:

var connection = new SqlConnection();
try
{
    /* Inner code here */
}
finally
{
    ((IDisposable)connection).Dispose();
}

The Dispose() method of SqlConnection then handles closing and flushing all the connection information.

On to your requests:

  1. From a security perspective, would this code be acceptable in a SQL Importer?

As far as security goes – maybe. I’m not a security expert, but I would be wary of having no input validation here. At the very least, do not allow input with a semicolon (;), as that can totally mess with your connection string.

  1. Besides data sanitation, is there a best practice somewhere that I’m missing in this?

I mentioned them above, but generally speaking you’re trying to solve a problem that we have accepted as a non-issue. Worry about the features of the application, then about the security. Your job as a programmer is not to secure the information in RAM, but to secure it:

  • On disk / file system
  • In the database
  • Over the network

Don’t worry so much about the password sitting in memory. Yes, it’s generally safer to keep it in memory as shortly as possible, but that goes along with Single-Responsibility Principle, the Principle of Least Astonishment, etc. Your password (and all variables) should be as short-lived as possible. (I.e. Don’t do like the old C code used to with 100 variables defined at the top of the file.)

I have turned this answer into a guide on my blog for further explanation.

Your code fails to escape values in the connection string. Use the SqlConnectionStringBuilder class instead. You set properties on it, and it will escape the values as necessary.

For example, if you are unclear how to set properties on an object:

var builder = new SqlConnectionStringBuilder();
builder.UserId = "my user id";
// and so on for the other properties

I hope that this is fairly self-explanatory, but if there is anything I can explain in more detail, please leave a comment.

Instead of integrating the username and password in the connectionstring you should use a SqlCredential object. The constructor of the SqlCredential class takes a username as a string and the password as a SecureString.

The resulting SqlCredential can the be passed with a connectionstring to the constructor of the SqlConnection.

Sample of reading the password from the console 

private static SecureString ReadPassword()
{
    var secured = new SecureString();
    ConsoleKeyInfo keyInfo = Console.ReadKey(true);

    while (keyInfo.Key != ConsoleKey.Enter)
    {
        secured.AppendChar(keyInfo.KeyChar);
        keyInfo = Console.ReadKey(true);
    } 

    secured.MakeReadOnly();
    return secured;
}

and used like so 

ConnectionString = "Data Source = " + ServerName + "; Initial Catalog = " + DatabaseName + ";";  
UserName = Console.ReadLine();  
SqlCredential credentials = new SqlCredential(UserName, ReadPassword());
using(SQLCon = new SqlConnection(ConnectionString, credentials))
{
    try
    {
       SQLCon.Open();
       Console.WriteLine("nConnected!n");
       Console.WriteLine("Working...n");
       .... etc
    }
    catch (Exception ex)
    {
       MessageBox.Show("Failed! See console for details");
       Console.WriteLine(ex.Message);
    }

}

You can for sure do this in a loop for the case that the user entered the wrong username or password, but you shoucl restrict the number of tries to a not too high number.

Part of ConnectionString repeats. Do the common part once.

You never test the credentials. You should report back the error message if the connection (Open) fails.

As stated in comments, sanitize data.

I bet the string is still in SQLCon anyway, otherwize .Open() would fail.

Leaving connections open is bad for scale.

Leave a Reply

Your email address will not be published. Required fields are marked *