It doesn't matter what you do with this code, the
ConnectionString can always be accessed in the
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.
A new tab will open with a lot of fun stuff, order everything by 'Object Type' and find
Double click your
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.
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:
- 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
SQLCon, not creating it like you are. (
using (var connection = new SqlConnection(ConnectionString))), same with
SqlDataReader if/when/where you use them.
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();
/* Inner code here */
Dispose() method of
SqlConnection then handles closing and flushing all the connection information.
On to your requests:
- 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.
- 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.