8.04.2009

Synchronizing the HttpSession

This is something that I have heard a great deal of debate over the last 2 years about. The servlet spec was somewhat recently amended to clarify that there is no guarantee that multiple calls to HttpServletRequest.getSession() or HttpServletRequest.getSession(boolean) will return the same object. This holds especially true in containers that return a facade object that wraps around the actual HttpSession object that you are working with, like Tomcat.

Why would you want to synchronize a session anyway?
The answer is pretty simple actually. Consider the following theoretical block of code:

public class WithdrawFundsServlet extends HttpServlet {

@Override
protected void doPost(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
User u = ESAPI.authenticator().getCurrentUser();
String withdrawAmt = request.getParameter("withdrawAmt");
float amt;
Account acct = session.getAttribute("acct_"+u.getAccount());
try
{
amt = Float.parseFloat(withdrawAmt);
}
catch ( Throwable t )
{
ESAPI.log().info( Logger.SECURITY_FAILURE, "Non-Numeric value passed as Withdraw Amount");
try {
ESAPI.httpUtilities().sendForward(request, response, "/error" );
} catch (AccessControlException ignored) { }
}

// Calling Withdraw will queue a check to be printed and mailed to the customer.
AccountFacade.withdraw( acct, amt );

try
{
ESAPI.httpUtilities().sendForward(request, response, "/success" );
}
catch (AccessControlException ignored) { }

return;
}
}


Now there are a couple things that I will point out that I am sure you will notice if you are paying attention. The first is that yes, this example is using the ESAPI. Call it a shameless plug :). The second is that I am ignoring AccessControlExceptions. This is purely to keep this example scenario short and to the point, and in any production code, you would never want to do this. There would also be some validation code in there as well.

Aside from those things, it looks innocent enough right? Well let's consider this for a second with a scenario.

Joe needs to have a check cut to him from his account at SomeBodiesBank. So he gets online and hits the form for the above servlet. Joe is not that savvy of a computer user, and like most novice internet users will do, he has the tendency to double-click on everything. He fills out the form to withdraw $500 from his account and double-clicks the submit button. So somewhere on the backend, we'll say in the AccountFacade.withdraw method, the software validates that Joe has enough money to cover the check, it discovers he has $750 in his Checking account so everything looks good. But wait a minute, Joe double-clicked remember?

Do you know what happens when you double click the submit button on a form? Well, 2 requests get submitted one after the other. Hmmmmmm.. So now I have 2 requests entering this method at the exact same time, both requests check Joe's balance and discover that he has $750 in his account, so they both queue up a request to print a check for the requested amount. There's only one problem, these are cashiers checks, the bank has withdrawn $1000 dollars (or in some circumstances, maybe only withdrew the original $500 from his account) but Joe ended up with $1000 in cashiers checks!

The checks show up in the mail, and Joe being the responsible individual he is, reports this to the bank. The bank will likely write this off as an anomoly and the bug will remain until one day when Joe is down on his luck and remembers the bug. He finds a program called JMeter and submits 1000 requests to the servlet as fast as he can for $1000 withdrawals. When his 1,000,000 in cashiers checks arrive, he promptly leaves the country and disappears in the backwoods of New Zealand never to be heard from again.

So the moral of the story is that this problem could have been easily avoided simply by adding thread-safety measures to the code. Granted the example cited is extreme and the consequence of the example even more extreme, but I can promise you that something similar to the situation has already happened and even moreso I can guarantee that something similar will happen again.

So, with this knowledge, what is the correct means to add thread safety around manipulating the session. It's quite simple even.


final Object lock = request.getSession().getId().intern();
synchronized(lock) {
AccountFacade.withdraw( acct, amt );
}


Would do the trick in this simple example.

It's important when using synchronization to always lock on immutable objects. It is also important to use the same lock when locking in multiple places where you are working with the same data. Thread-safety is an entire subject on it's own that is will beyond the scope of this blog posting, so I will cut to the chase here.

This is incorrect, and not guaranteed:

synchronized (request.getSession()) {
// do stuff
}


While this method is proven and works:

synchronized (request.getSession().getId().intern()) {
// do stuff
}


Some interesting stats to close out with:

Google Code Search Results found approximately 4000 uses of different ways to say 'synchronized(session)'

The scary part is this was only the first 5 ways I came up with to search for it.

10 comments:

  1. Nice article. I think as we focus on application security issues the area of concurrency often gets over looked. Many time because it’s hard to spot as you rightly said.

    The more developers know of these issues during the creation process, the more they can proactively address the issues before the code is out the door.

    -Michael

    ReplyDelete
  2. Thanks! This concept of viewing application development from the viewpoint of application security is something that I think is important to start pushing. There are plenty of little things that can be ingrained into the developers mind (me being one of them) that will keep your applications more secure at the low-level and will make the job of the appsec guy better by allowing him to focus on appsec issues directly rather than appsec issues caused by incorrect coding practices.

    ReplyDelete
  3. Fantastic, it so great to come across a solution provided by someone who actually knows what he talks about - many thanks!

    ReplyDelete
  4. Of course for this example you would want some kind of database with optimistic / pessimistic row level locking, right?

    ReplyDelete
  5. Most modern relational databases come with some type of row locking mechanism out of the box to ensure that a read will wait for a concurrent write and visa versa (depending on the locking paradigm) so the short answer to your question is yes. However, it is important to note here that this isn't just an issue of concurrency, but also access control - it is also important to integrate access control decisions into data calls where you are dealing with sensitive data that may be subject to concurrency issues. Adding logic to ensure that a user can only modify data that he has access to and even adding timing thresholds (User A can modify this data once per minute) can go a long way in addressing threats that depend on concurrency issues.

    ReplyDelete
  6. interning can lead to a lot of permgen bloat can't it?

    ReplyDelete
  7. I have never heard of this happening and I have been doing this in some pretty heavy traffic applications for a while now. Definately worth checking into tho - I'll shoot off an e-mail to a couple colegues and see what they think. Since Strings are flyweight factoried, I wouuld think this would be a non-issue tho.

    ReplyDelete
  8. The problem with using the intern'd session ID is that the set of intern'd strings will slowly grow over the lifetime of the application. Since these session IDs will not be valid after the session itself has expired, this is a form of memory leak. After all, you cannot un-intern a string. If you have an app that deals with hundreds of thousands of sessions a day, this technique will not scale, and may quickly get you into trouble.

    Does anyone have experience implementing this technique on such a system?

    ReplyDelete
    Replies
    1. Interned Strings are garbage collected as long as there are no references to them.
      See Myth(buster) #3:
      http://www.codeinstructions.com/2009/01/busting-javalangstringintern-myths.html

      Delete
  9. getSession().getId().intern() ... pretty brilliant. Thanks!

    ReplyDelete