A storm of fail

It’s OK to not know something. It really is, if you’re willing to admit it, and have a reasonably accurate idea about your own level of competence. Unfortunately, thanks to the Dunning-Kruger effect, we’re all better than average drivers, parents, and… security experts. Through the excellent @InfoSecInsanity, I got pointed to a veritable (and unfortunately, involuntary) repository of ways you can screw up your web site’s security. I don’t want to pick too much on the author, as as far as I know, he’s well-intentioned, but he should really retract his post, as well as any other post he wrote about security, and take a few courses. He might also want to stop calling himself an”expert” about topics on which he has clearly no expertise.  Writing crappy code is one thing. Propagating dangerous code through blog posts, that hundreds of clueless people will copy into their own applications, is another.

So let’s see how many fails we can find in that post. If I missed any, let me know in comments…

The first paragraph should be enough to make you cry:

“Username and password will be stored in the cookie.”

No. You never store a password in athe cookie. Not even hashed, not even encrypted, but especially not in clear text. This is not how you implement “remember me”. Here’s how to implement a “remember me” feature:

DON’T

Seriously, you’re incompetent, I’m incompetent, almost everybody is. Use a proven library that’s been hammered by hackers and patched accordingly, not some code from a random blog post. In the case of ASP.NET, this feature is implemented by the platform out of the box. Read the documentation to understand where it’s appropriate to even use this feature. Don’t roll your own.

Also, use SSL at the very least on your login page, and on everything you serve to authenticated users. Even better, use it everywhere.

The third paragraph is quite scary as well: it’s a bunch of links to other, equally misguided posts by the author. I’ll resist the urge to review those… For now… Let’s look at the code…

The HTML is quite bad (inline styles, tables used for layout), but not especially dangerous, so let’s skip it and focus on the C#, which is both bad and dangerous. After a sigh of relief at the unexpected usage of sql parameters and the consequent absence of injection bugs, we are forced to face the horrible truth: the passwords are stored in plain text. They are not hashed, not salted. Plain text.

If the login and password are validated against the database, the cookie is sent to the user’s browser (in plain text, did I mention that?). The HttpOnly flag is, of course, not used.

The rest is not especially dangerous as far as I can tell, but just weird:

• Why even read the data reader? Its contents are not used.
• Why communicate with the user by injecting script into the page? And yes, of course, this script displays obnoxious alert boxes.
• Catching all exceptions? Just don’t do that. Remove the catch and leave the finally (or learn about “using”).

I’ll leave you with the most disturbing thing about this article:

And the comments… Oh boy.