views:

378

answers:

4

The following is the piece of java code by using filters that shows the error page at every time if the username and password is also correct. Please help me, I don't have much knowledge on this concept.

String sql="select * from reg where username='"+user+"' and pass='"+pwd+"'";
rs=st.executeQuery(sql);
if(rs.next())
{
    chain.doFilter(request,response);
}
else
    sc.getRequestDispatcher("/error.html").forward(request,response);
+2  A: 

Use a prepared statement, your code is an open invitation for an SQL injection.

Connection con = getMyConnection();
        try {
                //no string concatenation, we use ? instead:
                PreparedStatement ps = con.prepareStatement("select * from reg where username=? and pass=?");
                try {
                        //actual value for parameters are set here:
                        ps.setString(1, user);
                        ps.setString(2, pwd);
                        ResultSet rs = ps.executeQuery();
                        if(rs.next()) {
                                chain.doFilter(request,response);
                        } else {
                                sc.getRequestDispatcher("/error.html").forward(request,response);
                        }

                } finally {
                        ps.close();
                }
        } finally {
                con.close();
        }

Now for your question, please check:

  • that the table and column names are the right one (don't you have a 'login' and a 'username' column?)
  • that the values are really correct (try the query in sqldevelopper for instance)
  • that it works with an ascii-7 password and username (it might be an encoding problem)
  • that the password column contains the real password and not a hash of it
Jerome
+1  A: 

First of all, you really should use parameterized queries for this. If the user enters '";DROP TABLE reg; as a username you will be in big trouble.

Also, are you sure the username and password are correct? How about capitalization?

Thomas Lötzer
A: 

So many things wrong with this... :-/

  1. The big one - SQL Injection. Don't write another line of SQL in your code until you understand it. And that's not an exaggeration for effect; code written without understanding of this is likely to give an attacker arbitrary access to your database.
  2. You shouldn't be able to issue a query like this because you should never store passwords in plaintext. Instead, you should calculate and store some kind of (preferably salted) hash of the password, and then hash the submitted password and compare this. See, for example, How to best store user login and password and Salting your password here on SO. This is especially bad given your SQL injection vulnerability.
  3. select * from reg is unnecessary given that you just want to know if a row exists. If you used select 1 instead the database wouldn't have to inspect the contents of the row and could serve the query results from just the index. If you expected there to be lots of rows, select 1 where exists ... would be faster as this would allow the DB to shortcircuit the query after finding at least one row.
  4. You're not closing the statement and result set in finally blocks. This means that they are not guaranteed to always be disposed (e.g. if there's a SQLException thrown) leading to resource and connection leaks.
Andrzej Doyle
+3  A: 

This is an extremely bad practice. You really shouldn't pass the username and password plain vanilla around through requests. Make use of sessions, in JSP/Servlet there you have the HttpSession for. There is really also no need to hit the DB again and again on every request. That's unnecessarily expensive. Just put User in session and use the Filter to check its presence on every request.

Start with a login.jsp:

<form action="login" method="post">
    <input type="text" name="username">
    <input type="password" name="password">
    <input type="submit"> ${error}
</form>

Then, create a LoginServlet which is mapped on url-pattern of /login and has the doPost() implemented as follows:

String username = request.getParameter("username");
String password = request.getParameter("password");
User user = userDAO.find(username, password);
if (user != null) {
    request.getSession().setAttribute("user", user); // Put user in session.
    response.sendRedirect("/secured/home.jsp"); // Go to some start page.
} else {
    request.setAttribute("error", "Unknown login, try again"); // Set error msg for ${error}
    request.getRequestDispatcher("/login.jsp").forward(request, response); // Go back to login page.
}

Then, create a LoginFilter which is mapped on url-pattern of /secured/* (you can choose your own however, e.g. /protected/*, /restricted/*, /users/*, etc, but this must at least cover all secured pages, you also need to put the JSP's in the appropriate folder in WebContent) and has the doFilter() implemented as follows:

if (((HttpServletRequest) request).getSession().getAttribute("user") != null) {
    chain.doFilter(request, response); // User is logged in, just continue request.
} else {
    ((HttpServletResponse) response).sendRedirect("/login.jsp"); // Not logged in, show login page. You can eventually show the error page instead.
}

That should be it. Hope this helps.

To get the idea how an UserDAO would look like, you may find this article useful. It also covers how to use PreparedStatement to save your webapp from SQL injections.

BalusC