thanks for your constructive comments, as I mentioned that "bad, bad, bad" code is out. no longer in the application... your comments on my current code tells me that this code is not bad, but I should check out tomcat's container managed logins... right? plus I would like to mention that I have client side form validations (js) to stop query busters.
________________________________ From: Christopher Schultz [mailto:ch...@christopherschultz.net] Sent: Thu 19-Aug-10 11:01 PM To: Tomcat Users List Subject: Re: [OT] Sessions mix-up on Tomcat 6.0.26 on Linux -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Yawar, I'm marking this as off-topic for /your/ request. I just have some comments for you. Take them or leave them. On 8/19/2010 11:53 AM, Yawar Saeed Khan/ITG/Karachi wrote: > Ok, let me share my source code with you... > > my index.jsp page has a html form which submits the form data to a servlet > called loginmanager. > > this is the code inside doPost function; > > try { > > userbean user = new userbean(); // usebean is a class the has > setter and getter functions for user attributes > > user.setUserId(request.getParameter("txt_userid")); > > user.setPassword(request.getParameter("txt_pass")); > > user = udac.login(user); //udac is a class that has data access > functions, login function takes user object and checks its existence in db > and sets isValid attribute for that user Not using Tomcat's container-managed login? Any particular reason why not? It's quite easy to configure and has the added benefit of being properly tested. > if (user.isValid()){ > > HttpSession session = request.getSession(true); > > session.setAttribute("user_id",user.getUserId()); > session.setAttribute("user_name",user.getName()); > session.setAttribute("role_id",user.getRole()); > session.setAttribute("role_desc", user.getRoleDesc()); > session.setAttribute("last_login", user.getLastLogin()); Why not session.setAttribute("user", user)? > response.sendRedirect("main.jsp"); //logged-in page That should be: response.sendRedirect(request.getContextPath() + response.encodeRedirectURL("/main.jsp")); > }else{ > > response.sendRedirect("index.jsp?user="+user.isValid()); > //revert back to login page That should be: response.sendRedirect(request.getContextPath() + response.encodeRedirectURL("/main.jsp") + "?user=" + java.net.URLEncoder.encode(user.isValid())); It always helps to format and encode things properly. > } > > } finally { > > out.close(); > } What is "out"? > Previously i had tried a simple way; my index.jsp file called itself on form > submit, below code was in index.jsp (no servlet etc); > > //after form is submitted > > String query = "SELECT a.USER_ID,a.NAME, a.BRANCH_CODE, a.PASSWORD, > a.LAST_LOGIN_DATE, a.ROLE_ID, b.ROLE_DESC FROM LOGIN_INFORMATION a, ROLES b > WHERE a.ACTIVE = 'A' AND a.ROLE_ID = b.ROLE_ID "; > > query = query + "AND LOWER(a.USER_ID) = LOWER('"+ > request.getParameter("txt_userid") + "') AND a.PASSWORD = '"+ epass +"'"; > > boolean hasdata=false; > > java.sql.ResultSet rs = connection.executeQuery(query); Wow: this is a SQL injection attack just waiting to happen. What happens if I submit the txt_userid request parameter as "') OR 1;" or, even better, "'); DELETE FROM LOGIN_INFORMATION;" or some other evil thing? I believe that certain JDBC drivers will not execute more than one query per executeQuery() call, but you can't really count on that. It's easy to use a PreparedStatement and just do it properly: poof! SQL injection attacks are a thing of the past (unless the driver is broken, but they test those things very well). Also, most SQL databases perform case-insensitive string comparisons, so your LOWER(a.USER_ID) = LOWER(...) can probably be simplified. Note that it also means you likely have case-insensitive passwords (though you haven't shown us what "epass" is -- is could have been hashed. > while(rs.next()) { > > hasdata=true; > > session.setAttribute("user_id",rs.getString("USER_ID")); > > session.setAttribute("user_name",rs.getString("NAME")); > > session.setAttribute("branch_code",rs.getString("BRANCH_CODE")); > > session.setAttribute("role_id",rs.getString("ROLE_ID")); > > session.setAttribute("role_desc",rs.getString("ROLE_DESC")); > > > session.setAttribute("last_login",rs.getString("LAST_LOGIN_DATE")); > > upsql = "UPDATE LOGIN_INFORMATION SET LAST_LOGIN_DATE = SYSDATE > WHERE USER_ID = '"+ rs.getString("USER_ID") +"'"; > > int up = connection.executeUpdate("UPDATE LOGIN_INFORMATION SET > LAST_LOGIN_DATE = SYSDATE WHERE USER_ID = '"+ rs.getString("USER_ID") +"'"); > > int audit_insrt = InsertAuditEntry("F001", (String) > session.getAttribute("user_id"), (String) > session.getAttribute("branch_code")); > > response.sendRedirect("main.jsp"); How many redirects do you end up sending? Hopefully, only one. But this code is bad, bad, bad. It makes me wonder what other nuggets can be found in your code. - -chris -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxtY30ACgkQ9CaO5/Lv0PA1pgCcDe1cNVlaqRNlWAbyQVybng4X OpUAn3ab9KDdsYvVGYzQmoeB871SgUqp =eEX2 -----END PGP SIGNATURE----- --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org This E-mail is confidential. It may also be legally privileged. If you are not the addressee you may not copy, forward, disclose or use any part of it. If you have received this message in error, please delete it and all copies from your system and notify the sender immediately by return E-mail. Internet communications cannot be guaranteed to be timely, secure, error or virus-free. MCB Bank does not accept liability for any errors or omissions.
--------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
