3
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ CodeReview is not the proper site for security of memory. \$\endgroup\$ – paparazzo Jan 3 '17 at 16:28
  • 5
    \$\begingroup\$ Less about "Security" and more about usability: if a username or password contains a semicolon then your connection string will be invalid. \$\endgroup\$ – Jack Wilsdon Jan 3 '17 at 16:28
  • 3
    \$\begingroup\$ @JackWilsdon Obligatory: xkcd.com/327 \$\endgroup\$ – πάντα ῥεῖ Jan 3 '17 at 16:29
  • 2
    \$\begingroup\$ @TylerC "Is this secure" is far too vague a question. Secure against what? An external hacker, a rogue developer, human error, the NSA? Once you know what you want to protect against, then you can start figuring out whether your system is secure or not. \$\endgroup\$ – Kaz Jan 3 '17 at 16:37
  • 3
    \$\begingroup\$ If someone has access to your machine then there is absolutely 0 security. No point in trying to re-assign the Password property -- it doesn't actually remove that string from memory. Storing it as a char array and overwriting each index might be better in that regard but you still have to turn it into a string anyway. Long story short: focus on actually important things, this is pointless. \$\endgroup\$ – Jeroen Vannevel Jan 3 '17 at 16:56
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Is this just a "fact of life" sort of thing then? Do commercial applications worry about this? If not I won't get too far into it. Also, THANK you for answering this question this way, its exactly what I'm looking for. \$\endgroup\$ – Tyler C Jan 3 '17 at 17:42
  • \$\begingroup\$ @TylerC Yes, it's a fact of life. Commercial applications usually don't bother to worry about it at this level, that's the responsibility of the user. Your job is to ensure that actual network traffic is encrypted/secured, data access is secured, etc. If the client machine is compromised, you are not at fault. \$\endgroup\$ – Der Kommissar Jan 3 '17 at 17:43
  • \$\begingroup\$ Just read through it again, can you explain why it would be better to be using the SQLCon instead of the way I'm doing it? \$\endgroup\$ – Tyler C Jan 3 '17 at 19:18
  • \$\begingroup\$ @TylerC Added an explanation. \$\endgroup\$ – Der Kommissar Jan 3 '17 at 19:23
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ That is a try block not a loop. User will have to shut down and restart. \$\endgroup\$ – paparazzo Jan 3 '17 at 16:54
  • \$\begingroup\$ I see what you're saying, very inconvenient (I actually got annoyed when using it because of this.) I will add it into a loop. Thanks! \$\endgroup\$ – Tyler C Jan 3 '17 at 16:56
  • \$\begingroup\$ 1. I will make that change 2. Test occurs, just not in a loop. 3. I will make that change as well, I know that can become a problem 4. Probably, It might be worth investigating but I see that its not a concern 5. I have a close at the end of the try block Thanks for the input! \$\endgroup\$ – Tyler C Jan 3 '17 at 17:29
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.