11
\$\begingroup\$

I am building an ASP.NET MVC 5 application and, for reasons which are irrelevant at this point, I am attempting to build my own means of authenticating users. I'm still very new to programming, especially to this sort of thing, and I realize that what looks fine to me may be full of problems. Thus, I present my code, simplified for demonstration purposes, for review.

The Contact model:

public class Contact
{
    [Key]
    public int ContactID { get; set; }

    [Required]
    public string Email { get; set; }

    [Required]
    public string Password { get; set; }
}

The Login action:

[HttpPost]
public ActionResult Login(Contact contact)
{
    bool validEmail = db.Contacts.Any(x => x.Email == contact.Email);

    if (!validEmail)
    {
        return RedirectToAction("Login");
    }

    string password = db.Contacts.Where(x => x.Email == loginForm.Email)
                                 .Select(x => x.Password)
                                 .Single();

    bool passwordMatches = Crypto.VerifyHashedPassword(password, loginForm.Password);

    if (!passwordMatches)
    {
        return RedirectToAction("Login");
    }

    string authId = Guid.NewGuid().ToString();

    Session["AuthID"] = authId;

    var cookie = new HttpCookie("AuthID");
    cookie.Value = authId;
    Response.Cookies.Add(cookie);

    return RedirectToAction("Private");
}

Actions that require the user to be logged in will look like this:

public ActionResult Private()
{
    try
    {
        if (Request.Cookies["AuthID"].Value == Session["AuthID"].ToString())
        {
            return View();
        }
        else
        {
            return RedirectToAction("Index");
        }
    }
    catch
    {
        return RedirectToAction("Index");
    }
}

I feel like this is probably really bad. However, every "solution" I find seems overly complex and/or lacking any documentation that would allow me to make sense of it.

All I want is to make sure the user has a valid username and password before they place an order or update their account. Will my code do that securely?

\$\endgroup\$
  • 3
    \$\begingroup\$ I would only have 1 question for you : Why didn't you want to use the one that come with a new tutorial program. Rolling your own authentication system is one hell of a job. \$\endgroup\$ – Marc-Andre May 21 '14 at 19:25
  • 4
    \$\begingroup\$ Well, I don't really "know" that it's one hell of a job because it seems (to my inexperienced eyes) that what I've written works. Also, I felt I needed to write my own because I don't, at this point, understand MVC's default authentication processes at all. In addition, I have to use an existing table of user account data so, not understanding MVC's authentication, I end have ended up trying to do it myself. \$\endgroup\$ – Jason May 21 '14 at 19:39
  • \$\begingroup\$ It's normal, if you just started looking at authentication, but I can assure you that there is a lot to think. How do you store the password ? Are they salted ? How do you log-out, how do you keep them authenticated ? Is it secure ? It's good that you tried it yourself, but it's something hard to do well. \$\endgroup\$ – Marc-Andre May 21 '14 at 19:45
  • \$\begingroup\$ Don't you think the Login controller is doing too many things: connecting to the database, verifying the password, and creating the session? Can you think about 'S' from the SOLID principle? \$\endgroup\$ – user42957 May 22 '14 at 16:16
  • \$\begingroup\$ As I said in my initial post, my code is "simplified for demonstration purposes" so, yes, my controller could be improved, etc. My concern here is simply to find out if there are any security flaws with my very basic attempt at authenticating a user. \$\endgroup\$ – Jason May 22 '14 at 16:40
9
\$\begingroup\$

I fear that the credentials could be sniff by some attacker if they are listening to during the connection process. If https is not enabled, the password (if there is no protection) could be seen by any attacker during the HTTP requests.

This point is not directly related as to how you implemented your code, but it will go a long way as to how your code is safe.


string authId = Guid.NewGuid().ToString();  

I'm not a security expert, but using GUID for security purpose is something that I would recommend. It seems to be frown upon :

GUIDs were not designed to be a part of any security system; use of them in a security system is an "off label" use and is therefore a very bad idea. I can't see much at the moment that would break the mechanism.

You should take this advice as-is, and do some more research on it. I would not take a comment on a question as a good reference, but it would make me wonder if it's in fact a good idea.


There is a lot of documentation to help you develop a secure authentication system,but this great question on SO would cover it for most of the important things.


The authentication mechanism looks fine to me. Storing a cookie and check the value in the Session is a good way to assure that user is authenticated.

The only drawback is if someone hijack the session and the cookie, he don't need to login with credentials, since the token will assure him that the system see him as an authenticated user. There is not much in your code that you could do to prevent that.

What you will miss most by rolling your own mechanism is some "perk" from using the .Net implementation. From what I've previously done at school and search, you'll miss the [Authorize][3] properties where your code could look :

[Authorize]
public ActionResult Private()
{
    return View();

}

Which would hide all the cookie related code. What that could help you is, if for a specific reason you can't use cookies you won't need to change all the place where you were verifying the identity of your user. This is just for code readability and maintenance, this is not related to security.

\$\endgroup\$
  • \$\begingroup\$ See edit to my original post/code regarding the passwords. As for the GUID, I found a solution here (stackoverflow.com/questions/1402696/…) that seems to be better. These things having been covered, I'm still not seeing why my code isn't sufficient. It appears (to me) to keep the user from accessing protected pages unless they have a cookie that contains the correct and extremely difficult to duplicate value. The only catch I can think of is if somehow someone stole that cookie. Otherwise, I can't see what's wrong with it. \$\endgroup\$ – Jason May 22 '14 at 12:32
  • \$\begingroup\$ @Jason Part of the bigger picture is how you store your password. The reality is that if you don't store password correctly, if you don't have secure mechanism to assure the protection of the account this won't work. If somehow I can access you db and see clear password, I can connect with every account you have on your site and don't even care about stealing a cookie. \$\endgroup\$ – Marc-Andre May 22 '14 at 12:42
  • \$\begingroup\$ @Jason security.stackexchange.com/questions/41447/… \$\endgroup\$ – Marc-Andre May 22 '14 at 12:45
  • \$\begingroup\$ I apologize for not making my treatment of the passwords more clear: I am indeed hashing the passwords. I am using Crypto.HashPassword(registrationForm.Password) during user registration. \$\endgroup\$ – Jason May 22 '14 at 12:49
  • \$\begingroup\$ @Jason No no, I'm sorry I haven't seen that line in the edit. You know that's a good start! I'll edit my answer later, right now, your implementation looks "good" but I want to add some point that are not for security purpose, but I have a meeting. \$\endgroup\$ – Marc-Andre May 22 '14 at 12:53
7
\$\begingroup\$

You can create a custom authorization class inheriting ActionFilterAttribute

public class AuthorizationFilter : ActionFilterAttribute
{
  public override void OnActionExecuting(ActionExecutingContex   filterContext)
  {
  //Write you authentication logic here . you can use Request headers,cookies ,..etc 
  }

}

Then decorate your action method with [AuthorizationFilter]

[AuthorizationFilter]
public ActionResult GetAllEmployee()
{
  //Action method logic
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This is actually what I ended up doing not too long after I made my original post. It has worked great for me so far. \$\endgroup\$ – Jason Jul 14 '15 at 12:26
2
\$\begingroup\$

One addition to what has been said:

It's not enough to make sure that the login is done using https, as you don't want the authentication cookie to be submitted during any non-secure requests, as it could be intercepted by anyone listening to network traffic.

So, you also need both of the following conditions to be true:

  1. The authentication cookie needs to be marked as secure, so that it is only submitted on HTTPS requests.
  2. All actions that require authentication need to be available only using HTTPS.

In practice, if you have HTTPS enforced for everything across the entire site (and any other sub-domains that someone might set up), then point 1 could be avoided, but there's no reason to do that.

\$\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.