|
Comments and Discussions
|
 |
 |
Nice article. I like the fact that you mentioned MD5 and SHA-1 being hacked. Those should no longer be used in new applications and migrated for legacy applications (if possible).
|
|
|
|
 |
You should really read OWASP recommendations for password storage and identity management. Your implementation simply won't stand offline attack when password database is stolen, besides many other issues. You really should not reinvent the wheel, but rather use good patterns verified by experts
|
|
|
|
 |
If you had actually bothered to read, I did mention that
1. you should avoid writing your own password authentication in the first place
2. that spinning the password would be more secure, the entire point of doing that would be to make it more secure should the database be taken for offline attacks.
3. that this was only a STARTING place. Addressing OWASP suggestions means keeping up on them.
|
|
|
|
 |
Nice article. You might want to change the following :
Return "login failed" or "login successful"; we don't give want to show "user not found" ...
to
Return "login failed" or "login successful"; we don't want to return "user not found" ...
Cheers, and keep up the good work
|
|
|
|
|
 |
My pleasure.
Keep up the good work
|
|
|
|
 |
Good article.
Bcrypt would automatically slow it down. No need for a tarpit. Even it will make offline, password decrypting a menace, in the case ever the SQL database is leaked.
|
|
|
|
 |
In fact bcrypt is suitable only for worst case scenario these days, you should use adaptive functions like PBKDF or scrypt if possible
Banking establishments are more dangerous than standing armies. T.Jefferson
|
|
|
|
 |
In the article you suggest that " You store the salt separately from the password hash." But in your code, you choose to store the salt in the same table as the ID and encrypted password. So by the quoted message above, did you mean that the salt be stored in a different column? If that table's data is compromised, then even the salts are exposed too! Right?
If in real world the salts are not supposed to be stored in the same table, then can you please suggest some methods of safely storing the salts?
And if you really mean to suggest that salts can be stored in a separate column in the same table (or even a different table in the same database), then why are you averse to using the ID column, or email as a salt. A totally random salt, stored anywhere in the same database as the passwords, is as good as using ID/email as the salt.
Did I misunderstand something?
Gurjeet Singh
gurjeet.singh.im
|
|
|
|
 |
If the data in the database is compromised, you are kind of screwed anyways. So whether it is in the same table or not is irrelevant.
Why am I adverse to using the ID as the salt? There are a number of reasons, lack of randomness, and the likelihood of it being changed without understanding that it is more than just a meaningless ID.
In my opinion, ID columns are identifiers, nothing more, they are used to identify the row, for referential integrity. They are NOT there to provide DATA. When you rely on the ID column having certain data in it, you can break code and have no idea why.
Using the salt, in the table gives an attacker no idea of HOW you are using the salt, you could be appending the salt to the data, prepending the salt to the data, XOR encoding salt into the data then hashing it, using it to generate an HMAC hash, or any one of a dozen other things, having the salt doesn't mean that it is any easier to hack the salted hash, unless of course you are always using the same hash.
Another thing that COULD be done, is to generate a new salt and hash with every successful login.
You are missing lots, I suggest you go out and look at hashing algorithms.
|
|
|
|
 |
Some people say that you should put SALT into another table for added security, but if you use a hash of SHA 256 bits or above, no one will decrypt it. You'd need a super computer to do so.
|
|
|
|
 |
Salt is not the same as an encryption or decryption key. If you have a strong and reliable hashing algorithm, you cannot feasibly use the salt in any meaningful way to reverse the hashing process. The only reason the salt is there is to prevent the same plain text from being turned into identical hashed values.
For example, let's say four people in your database used the same password of "Password1" (yes, the worst password, but this is a simple example). All four records in the database would contain the same hash code value. Someone could then create a dictionary of common passwords and their resulting hash values, and do a reverse lookup on your database. By applying random salt values, all records would theoretically have unique hash values, even if the plain text passwords were the same. That's the only thing that salt does. Unless your hash algorithm has an exploitable weakness, you can't use the salt in a meaningful way to help return the plain text from the hashed value.
Having said that, completely random salt is better than sequential salt. Sequential or non-random salt values can introduce mathematical patterns that allow a hacker to get around it.
|
|
|
|
 |
Hello Alaric Dailey, first thanks much for your nice article(s).
I have already read a lot of extensive article about connections and database layers at code project, but as net programming beginner with limited english vocabulary it is often not easy to understand, since most articles are written in C# and I am only able to start with Visual Basic 2008 Express Windows Applications, but that is OK, knowing one day C# is the goal for me too and there are translation websites.
What I wanted to say is that with these simple condensed scenarios are perfect to learn for myself and I think also for others starting with Databases.
I have decided to implement this concept in my project in terms of security and independence from the database provider. I think to extract the table and column names as name-templates into a class is also welcome, am I right?
I would like another example of this article as Windows Application with integrated error handling, transactions and using the class DataSourceInformation (ParameterMarkerPattern) out of the article "Using Information from the .NET DataProvider" in the DB class, for not only put the right quotePrefix/quoteSuffix. The password in the App.Config for the connectionString does not need to be
encrypted, it is safe in the App.Config?
I know, try to do by myself is the best, but I don't know how to implement reliable in your classes, maybe someone finds some time to show me.
Anyway I like much the way you write your articles, wish to see more of this, thanks a lot and please excuse my english.
|
|
|
|
 |
First, thanks for the compliments
Judging by what you are saying, you will likely find my article Don't hard code your DataProviders very useful as well. Now for some specific responses.
ditsche wrote: I think to extract the table and column names as name-templates into a class is also welcome, am I right?
Yes, you can implement your tables and columns as classes, I have my own ORM that I am rewriting, that does exactly that. Though it's not yet ready for public release.
ditsche wrote: The password in the App.Config for the connectionString does not need to be encrypted, it is safe in the App.Config?
Storing a password in plain text is NEVER a good idea, especially not in a windows application where users could get to the hard drive much easier than say a web site. That being said there are 2 acceptable solutions, the first and easiest is to use a trusted connection (also known as "Integrated Security"). Your only other choice is to encrypt your app.config file explained here
ditsche wrote: I would like another example of this article as Windows Application with integrated error handling, transactions and using the class DataSourceInformation (ParameterMarkerPattern)
I will work on something like that.
THIS IS NOT THREAD SAFE, AND DOESN'T SUPPORT TRANSACTIONS it is only suitable as a proof-of-concept. My enhanced classes will be available soon, so I do not recommend you use this, or at very least, you need to wrap some of the calls with "lock" statements, so the Initialize won't run multiple times.
All of that being said here is the modifed DB class that initializes its DataSourceInformation class
public static class DB
{
private static DbProviderFactory _factory = null;
private static string _connectionString = null;
private static DataSourceInformation _dataSourceInformation = null;
private static string _quotePrefix;
private static string _quoteSuffix;
private static bool _initialized;
public static DbProviderFactory Factory
{
get
{
if (_factory == null)
{
ConnectionStringSettings connectionSettings =
ConfigurationManager.ConnectionStrings["DSN"];
_factory = DbProviderFactories.GetFactory(
connectionSettings.ProviderName);
_connectionString = connectionSettings.ConnectionString;
}
return _factory;
}
}
public static string ConnectionString
{
get
{
return _connectionString;
}
}
public static string QuotePrefix
{
get
{
Initialize();
return _quotePrefix;
}
}
public static string QuoteSuffix
{
get
{
Initialize();
return _quoteSuffix;
}
}
public static string CreateProperParameterName(DbParameter parameter)
{
if (!_initialized)
{
Initialize();
}
string parameterName = (parameter.ParameterName ?? string.Empty).Trim();
int length = _dataSourceInformation.ParameterNameMaxLength;
if (length < 1)
{
return "?";
}
if (string.IsNullOrEmpty(parameterName))
{
parameterName = "a" + Guid.NewGuid().ToString().Replace("-", string.Empty);
if (length < parameterName.Length)
{
parameterName = parameterName.Substring(0, length);
}
parameter.ParameterName = parameterName;
}
return string.Format("{0}{1}", _dataSourceInformation.ParameterMarkerPattern[0], parameterName);
}
public static string QuoteIdentifier(string identifier)
{
Initialize();
var ss = identifier.Split('.');
for (int i = 0; i < ss.Length; i++)
{
ss[i] = string.Format("{0}{1}{2}", _quotePrefix, ss[i], _quoteSuffix);
}
return string.Join(".", ss);
}
private static void Initialize()
{
if (_initialized)
{
return;
}
_initialized = true;
using (var conn = GetConnection())
{
_dataSourceInformation =
new DataSourceInformation(
conn.GetSchema(DbMetaDataCollectionNames.DataSourceInformation));
var cb = Factory.CreateCommandBuilder();
if (!string.IsNullOrEmpty(cb.QuotePrefix))
{
_quoteSuffix = cb.QuoteSuffix;
_quotePrefix = cb.QuotePrefix;
}
else
{
using (var cmd = conn.CreateCommand())
{
cmd.CommandText = "SELECT '1' as [default]";
try
{
using (var dr = cmd.ExecuteReader())
{
while (dr.Read())
{
}
}
_quotePrefix = "[";
_quoteSuffix = "]";
}
catch
{
_quotePrefix = _quoteSuffix = "\"";
}
}
}
}
}
private static DbConnection GetConnection()
{
DbConnection conn = Factory.CreateConnection();
conn.ConnectionString = ConnectionString;
conn.Open();
return conn;
}
public static int ExecuteNonQuery(string sql,
IEnumerable<DbParameter> parameters)
{
using (var conn = GetConnection())
{
DbCommand cmd = null;
try
{
cmd = conn.CreateCommand();
cmd.CommandText = sql;
foreach (var parameter in parameters)
{
cmd.Parameters.Add(parameter);
}
return cmd.ExecuteNonQuery();
}
finally
{
if (cmd != null)
{
cmd.Parameters.Clear();
cmd.Dispose();
}
cmd = null;
}
}
}
public static DbDataReader ExecuteReader(string sql,
IEnumerable<DbParameter> parameters)
{
var conn = GetConnection();
DbCommand cmd = null;
try
{
cmd = conn.CreateCommand();
cmd.CommandText = sql;
foreach (var parameter in parameters)
{
cmd.Parameters.Add(parameter);
}
return cmd.ExecuteReader(CommandBehavior.CloseConnection);
}
finally
{
if (cmd != null)
{
cmd.Parameters.Clear();
cmd.Dispose();
}
cmd = null;
}
}
}
And your final statement creation block would like this
List<DbParameter> parameters = new List<DbParameter>();
var p = DB.Factory.CreateParameter();
p.ParameterName = "user";
//fill in data... AS SHOWN IN ARTICLE
parameters.Add(p);
p = DB.Factory.CreateParameter();
p.ParameterName = "salt";
//fill in data...
parameters.Add(p);
p = DB.Factory.CreateParameter();
p.ParameterName = "password";
//fill in data...
parameters.Add(p);
StringBuilder sb = new StringBuilder();
sb.AppendFormat("INSERT INTO {0}Users{1} ({0}user{1},{0}salt{1},{0}password{1}) VALUES (",
DB.QuotePrefix, DB.QuoteSuffix);
string comma = "";
foreach (var p1 in parameters)
{
sb.Append(comma);
comma = ",";
sb.Append(DB.CreateProperParameterName(p1));
}
sb.AppendLine(")");
Debug.WriteLine(sb.ToString());
|
|
|
|
 |
Thanks for the quick response and posting the modifed DB class, and giving people like me a chance to do things more flexible, am waiting anxiously for the things to come..
I do not want to steal much time of you for answering off-topic question, but would like to give a briefly description of my Project for understanding and ask something general:
I try to upgrade an old VBA/Access Auctionhouse-Application to Net. I have now 5 MDW-Backends (each ca 200-300 MB) with about 50 tables. To provide are customer management, auctions, articles, artists, invoices, contracts and much more (Reports, management of thousends of Images).
Should I think about a solution with a SQL server Backend? I'm unfortunately not very happy with the free version of SQL Server 2008 Express (each client has to install it to run in network, Management Studio to edit uncomfortable), but for developing tests it's OK and later export to better SQL-Servers or other DB's would be easier, also in view of an administrative user management can then directly resolved in the SQL server (Integrated Security), I think that had answered my own question?
Is it recommended to use a number of smaller applications or store everything in a big one?
I usually prefer unbound data in controls and editing one record fields in textboxes, but see here often a lot of data used to display and edited in controls like Grids or Lists. Does the newer Data Providers getting better in this case and does it not stress the network traffic more then necessary?
Am I able to use Blobs (Binary Large Objects) with parameter commands?
Does the Sharp Editor things better then the Visual Studio Editor or is it to prefer?
I know, this are a lot of question, but a keyword or simple yes/no would suffice me.
Many thanks in advance and greetings from Germany ...
|
|
|
|
 |
number of small applications vs one big one I think you are asking about a number of small databases vs 1 big one. The BEST answer is "it depends". Security, backups, replication needs, ability to restore bits without affecting others using the system, skills of existing DBAs or ability and cost of getting DBAs, whether or not there is "enterprise" support, and ease of deployment all weigh into the decision. Should you choose to go with one big database, using schemas (namepaces) is good way to keep the separation you would have from multiple databases, and keep from having issues like duplicate table names. Even multiple databases can be done and with MS SQL Server and MySQL can have all work done thru a single connection. "Real" schema support like I am talking about requires a really good RDBMS, like PostgreSQL, Microsoft SQL Server or Oracle. You can see a nice breakdown of comparisons of RDBMS on wikipediate here. Dealing with installs is one reason to use things like Access and MS SQL Express edition. However if you want to avoid much of the installs, you can use Microsoft SQL Server Compact Edition. Regardless if I am working with 1 database or 10, I don't like keeping databases locally, I prefer servers that everyone works from.
Blobs
In my opinion the only good way to handle them is with Parameters. Some providers allow you to do things to put the data to strings, but it doesn't appear to be standard.
SharpEditor vs Visual Studio I don't have any opinion on which is better, because I have never used SharpEditor.
Bound vs Unbound controls and network trafficBound controls don't require an open connection, for example, if I use the following code to open a datareader
cmd.ExecuteReader(CommandBehavior.CloseConnection);
then bind it to a DataGridView (windows control), GridView, or DataGrid (ASP.NET controls) it will bind without issue, and continue to work even though the connection to the database is gone.
Connected datatables and datasets are another story, those are what I suspect you are talking about, and those are going to have the same network traffic all the time regardless of provider.
|
|
|
|
|
 |
IMO it's much more better to use a bit different approach for implementing authentication with less coupling - create a base interface for password providers with methods for generating hashes of passwords, create an implementation of that interface and split other code in classes that will not violate Single Responsibility Principle.
And your database access code, IMO, is a bit too oldschool.
|
|
|
|
 |
I wasn't trying to show any particular design principle, just trying to make the code simple. All the code and information presented, is to keep the code simple. That is why I didn't get into length or complexity checking, design patterns, enabling and disabling the buttons, or anything else.
|
|
|
|
 |
I'm sorry, but decoupled is simple, your current code is more like a spaghetti code (all is mixed in one place). This code works when you need to proof some concepts. But, IMO, it's not suitable for article. Think about young developers that read your article and don't show them a bad example. It's very easy to move code to separate classes that have single responsibility. And the code will become much more readable and simple.
|
|
|
|
 |
I guess we will agree to disagree.
Yes it is tightly coupled.
No it is not spaghetti code.
The code is simple, however not useless.
All to often articles, blog entries, books and other examples get caught up in showing everything at once, and coders looking for a few lines of "how-to" code end up looking through pages and pages of someones idea of "proper" code. The situation gets compounded when you add cryptography in the mix. Once some poor programmer finds what they are looking for, they end up spending a good deal of time sifting through the articles and vocabulary they don't understand hoping to glean enough information to make the code work without sacrificing security. Then once they have the code, they pour over the code having to undo the authors ideas, so they can retro-fit it into some project that they had thrust upon them.
Consequently this article and the accompanying code could have been HUGE, with explanations of
- how hashes work
- polymorphism
- inheritance
- abstraction
- encapsulation
- random vs pseudo random numbers
- why use binary fields rather than strings
- Base64 encoding, and how to use it if you can't use binary fields
- the "factory" pattern I am taking advantage of in the DB class
- Connection Pooling
- genericizing DB code by using the base classes for the parameters
- genericizing SQL by wrapping the keywords based on the DB information
- abstracting classes to enable a simple class change to change the login logic
- separation of interface from logic
- minimum password length, and why its important
- password expiration, and why it is a good or bad idea
- preventing password reuse
- database normalization
- foreign keys
All of those are great and could have been added, and are covered in great detail in other places. However, not a single one of those explanations, or the required code to show it, is necessary.
The idea of this article and the code is to "simply simplify". Not a single line of code more than is necessary, not for abstraction, code reuse or anything else.
|
|
|
|
 |
I don't see this as spaghetti code. The definition of spaghetti code is the following:
"Spaghetti code is a pejorative term for source code which has a complex and tangled control structure, especially one using many GOTOs, exceptions, threads, or other "unstructured" branching constructs."
When I am looking for an answer to a particiluar question, I don't want to have to sift through somebody else's idea of what they think is "good coding practice". Many places have different ways of doing things. I just want to find what I need and plug it in to the structure I am currently working with.
|
|
|
|
 |
A nicely structured article with good descriptions of the algorithms in use. However, I would like to see a few more comments in or around the code as I, and probably others, have fairly limited knowledge of SQL/DataBase.
MVP 2010 - are they mad?
|
|
|
|
 |
Thanks for the feedback.
What would you like explained?
This article is supposed to be as simple as possible, I am disappointed that I left any confusion.
|
|
|
|
 |
I don't understand Richards complaint either. This article is not about databases or how to store passwords in them. Its more of a high level "best practices" type article for storing passwords in general. The fact that you included some DB code is icing on the cake. I gave you a 5.
|
|
|
|
 |
|
|
General News Suggestion Question Bug Answer Joke Rant Admin
Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.
|
Type | Article |
Licence | CPOL |
First Posted | 24 Jan 2010 |
Views | 48,502 |
Downloads | 867 |
Bookmarked | 75 times |
|
|