This post goes through how we fixed CSRF (cross site request forgery) in JForum, issues encountered and approach.  It is useful reading for anyone who needs to protect against CSRF on their website.

Background

Stock JForum has a number of security vulnerabilities.  We’ve fixed a lot of the XSS ones.  We hadn’t fixed CSRF as of early January 2013.  (It is fixed now – don’t bother trying.)  We had captchas enabled for creating ids.  And I think our working theory was that other CSRF problems would be annoying, but easy to undo.

On January 14th, we learned there was going to be an announcement of XSS and CSRF vulnerabilities  on January 16th (later changed to February – High-Tech Bridge asked me to post this before their announcement.)  I did a code review and learned that someone could exploit a CSRF attack to cause all sorts of damage including deleting the forums – which would require a database restore and likely lost posts from the duration.  Eek.  Nothing like an urgent need to have a release.

What is CSRF

Cross Site Request Forgery is a security attack where the attacker tries to do something in your name.  For example, if you open an email (with images enabled) or go to another website in a tab of your browser while being logged into your bank in another tab, the bad guy could try to move money between accounts for you.  Not something you want.  In a forum, the bad guy could post spam or ads in your name.  Or even delete the forum.  OWASP has an excellent overview of CSRF along with suggestions of “fixes” that do and don’t work.

Analysis

The first thing I did was write code (AllJForumActions.java) to output the names of all actions in JForum that can be accessed from a browser.  To see how much analysis was required.   In stock JForum, the 112 such method names are listed here..  In JavaRanch/CodeRanch forked JForum, there were 191.  (we’ve added a lot of functionality)

I then classified them in a property file by whether they were safe (read only) or needed protection.  I wrote a unit test to check this property file:

• has all action names represented
• doesn’t have any action names more than once
• only lists valid case.

Choosing a CSRF filter

Ok.  Analysis done.  Now on to coding. CSRF is a common problem.  I’d like to use an existing tested filter and just adapt it to JForum.

1) Google code CSRF Filter (StatelessCookieFilter)

From website: this one says it is better than the OWASP one by being simpler and using cookies rather than session (memory) usage.

Analysis

• It is simpler, I’ll give them that.
• I’m not terribly concerned with the memory implication.  It’s just one token per user.  We store other things in the session in memory.
• Disadvantage: the code requires you to manual add the CSRF token to all POST forms.  That’s time consuming and error prone.
• HUGE disadvantage: the filter assumes that only POST methods need to be protected.  This is only true if you strictly enforce not accepting GET requests for any requests that change state.  While I agree all code should be that way, it isn’t in the real world.  Many frameworks (including JForum) treat get and post requests the same by directing them to the same code.  If I was using this code, I’d need to change it avoid the check for POST.  (albeit not a big deal since it is less than 100 lines long).  We’d need to protect both get and post actions and exclude the expected GET URLs from the filter.  This checks the expected POST actions regardless of their actual submit type.

3) OWASP filter

Well documented.   Supports many types of checking.  Sets up Javascript DOM injection of token so don’t have to hand edit each page.  It supports a choice of only injecting the token into forms or whether to include src/href links as well.  This is great as I know that each and every update to the database isn’t in a form.  I’m also confident the developers know what they are doing because the documentation says:

 However, most JavaEE web applications respond in the exact same manner to HTTP requests and their associated parameters regardless of the HTTP method. The risk associated with not protecting GET requests in this situation is perceived greater than the risk of exposing the token in protected GET requests. As a result, the default value of this attribute is set to true. Developers that are confident their server-side state changing controllers will only respond to POST requests (i.e. discarding GET requests) are strongly encouraged to disable this property.

Comparing this to the google code which only checks POST, it is a very easy decision which one to go with.  The one that actually secures you against CSRF in the face of code that isn’t perfectly written.  OWASP CSRF Guard!

Starting out with OWASP CSRF Guard Version 3

The documentation was clear and self explanatory so let’s get into the problems I encountered:

1. Minor problem: the OWASP CSRF Guard source code from github isn’t the same with the download offered on owasp.  It was an easy Ant build from the github version.  I rebuilt because I needed to call a method that existed in the github version but not the download/packaged one
2. Major problem: JForum doesn’t consistently use URL names for actions.  There are a lot of Ajax calls that pass the “url name” as a parameter so I can’t filter these out through the OWASP API.    And the “unprotectedPages” logic in the OWASP filter is deep within the component in a final class.  Very secure, but I can’t extend it so must find another approach.

See part 1 for how we got here  and part 2 for how we changed the OWASP filter.

Code cleanup and problems

There is some poorly written code in JForum that CSRF now prevents from working.  In these cases, I needed to clean up our code.  For example:

1. Links/anchors shouldn’t be used to update state.  They should only be used for gets.  The OWASP filter is realistic enough to recognize links are in fact used for updates in the real world and accomodates them.  It doesn’t accommodate code like

   onclick="document.location = '${contextPath}/${jforumMainServletMapping}/${moduleName}/...';" />

   In the cases this was used for actions that required CSRF protection, I needed to refactor to use a form.  In a few cases, I created a one to one form.  On others where there were a lot of actions or it wasn’t conducive, I used a common form

   <!-- CSRF token messes up submission if forum id isn't in URL -->
   
   function submitActionForForum(actionVerb, forumId) {
   
   var action = document.actionForm.action;
   
   action = action.replace("ACTION", actionVerb);
   
   action = action.replace("FORUM_ID", forumId);
   
   document.actionForm.action = action;
   
   document.actionForm.submit();
   
   }
   
   </script>
   
   <#-- general purpose form for the submit buttons on the screen; see JS for ACTION and FORUM_ID values -->
   
   <form method="post" name="actionForm" action="${contextPath}/${jforumMainServletMapping}/${moduleName}/ACTION/FORUM_ID">
   
   </form>
   
   ...
   
   onClick="submitActionForForum('up', ${forum.id})"
   

2. Some pages (like the first one I happened to test) didn’t have </head> so dynamically adding the JavaScript which sets the CSRF token wasn’t working.  I wrote a unit test to identify such files and ensure we don’t create any more.  We also added the missing HTML headers.  Most of them were on the admin pages.

Note: all code is sanitized to remove references to javaranch/coderanch.

The story continues in part 4 – deciding not to use JavaScript for the solution.

While implementing CSRF for JForum, I needed to extend the OWASP solution.  Let me tell you, they don’t make it easy to extend.  Lots of final.  Here’s what I did – linked to code on github.

To read about the original problem or why I choose the OWASP filter, see part 1.

Extending the OWASP solution

1. CsrfFilter.java – The OWASP filter is final so had to copy the logic in my own.  I added logic to get the action method name/
2. CsrfHttpServletRequestWrapper.java – Since I’m using actions instead of URLs, I need to make the request look as if it the actions are URLs.  A short simple class.
3. CsrfListener.java – OWASP assumes you are using URLs and you can enumerate them in the property file.  I have action names and there a lot of pages to allow as unprotected – without wildcard patterns to help, this is unwieldy.  The OWASP listener isn’t final but the logic I needed to extend was in the middle of a method.  So I copied their listener adding a method that reads the csrf.properties and creates “org.owasp.csrfguard.unprotected.” properties for all lines that weren’t set to “AddToken.”
4. Redirect.java – the OWASP Redirect class works just fine if you always have the same context root.  We have a different one when testing locally vs on the server.  And since the path is in a property file, it isn’t easy to change.  Of course the OWASP class was final so I had to copy logic rather than extending it.  My Redirect class gets the server and context root from the request and adds it to the relative path in the OWAP property file.
5. AddJavaScriptServletFilter.java – Adds the OWASP JavaScript to each page after </head> unless it is a download or a user isn’t logged in.  Since logged in users can’t do anything CSRF exploitable, they don’t need the token.  This also makes Google happier because googlebot doesn’t see the extra parameter confusing the URLs.
6. AjaxHelper.java – The OWASP filter adds “OWASP CSRFGuard Project” to the X-Requested-With header for AJAX calls.  You can customize it to not include that, but I wasn’t sure what would happen if I left it blank.  It was easy enough to change JForum to use startsWith instead of equals() to make it more flexible.
7. Owasp.CsrfGuard.js (attempt 1)
   1.  The JavaScript provided by OWASP has a bug.  It goes through the properties in document.all from top to bottom.  As it finds forms, it adds hidden input fields.  So far so good.  The problem is that when you have a lot of forums, some of them get bumped past the original end of the document.  So the loop never sees them and the CSRF token is never added.  The solution is to go through the document from bottom to top.  I’ve submitted a fix to OWASP for this.  What did the trick for understanding the problem was adding console.log(“length: ” +  document.all.length);  at the beginning and end.  It grows by roughly 100.   The bug fix was changing in injectTokens()

      for(var i=len -1; i>=0; i--) {

      to

      // BUG FIX: if go through in ascending order, the indexes change as you add hidden elements
      
      // when have 100 forms, this moves some of the forms past the original document.all size
      
      for(var i=len -1; i>=0; i--) {

   2. IE doesn’t like

      var action = form.getAttribute("action");

      when you have a form field named action.  (If at all possible, DON’T DO THAT!!!).  Since JForum has 100’s of such reference, changing it at this point isn’t an option.  IE helpfully returns some object.  It’s not an array/map or list of any kind that I can tell.  After getting hopelessly stuck on this, I asked Bear Bibeault for help.  He noted the answer was in “Secrets of the JavaScript Ninja” a book he wrote that I had recently read.  And it was.  Solution

      // hack to test if action is a string since IE returns [object] when action in form and as hidden field
      
      // if not a string, assume it is our action and add token for now
      
      var action = form.getAttributeNode("action").nodeValue;

   3. The OWASP code contains this innocent looking code.

      var hidden = document.createElement("input");
      
      hidden.setAttribute("type", "hidden");
      
      hidden.setAttribute("name", tokenName);
      
      hidden.setAttribute("value", (pageTokens[uri] != null ? pageTokens[uri] : tokenValue));

      . It works fine in all browsers except IE. Want to know what IE does? That’s right.  It generates a hidden field with the correct token value and *no* token name.  I found this problem solution online and changed the code to

      
      var hidden;
      
      try {
      
      hidden = document.createElement('<input type="hidden" name="' + tokenName + '" />');
      
      } catch(e) {
      
      hidden = document.createElement("input");
      
      hidden.type = "hidden";
      
      hidden.name = tokenName;
      
      }
      
      hidden.value = (pageTokens[uri] != null ? pageTokens[uri] : tokenValue);
      
      

   4. My last IE change was an easy one.  Easy in that it was a problem I had seen before.

      var location = element.getAttribute(attr);

      changed into

      //var location = element.getAttribute(attr);
      
      // hack - getting same error as on action - don't know why but hack to move forward
      
      var attr = element.getAttributeNode(attr);
      
      var location = null;
      
      if ( attr != null) {
      
      location = attr.nodeValue;
      
      }
      

   5. Then I encountered an issue with the event handling framework in IE>  I didn’t even look at this.  All IE debugging was done between 9pm and midnight January 28th-30th after we all thought CSRF was fine from being in production the previous Sunday.  The only computer I had access to that ran IE is from 2002 and barely runs.  I was tired of IE and didn’t want to throw any more time down that rabbit hole.
8. Owasp.CsrfGuard.js (attempt 2) – After encountering numerous IE problems, I would up merging the code from OWASP CSRF Guard version 2 and 3 to make it less dependent on the browser.

Finally, we get to look at the JForum specific parts in part 3.