tomcat-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: [OT] Sessions mix-up on Tomcat 6.0.26 on Linux
Date Thu, 19 Aug 2010 17:01:49 GMT
-----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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org


Mime
View raw message