Struts 2 Security Update

A couple of weeks ago, a remote exploit was demonstrated for applications using Struts 2.0.8 and below. It's a scary one. Like System.exit(0) scary. In some ways I can't believe that it got this far because it's such a simple one.

Anyway, if you're using Struts 2 below verson 2.0.9, or if you're using WebWork below version 2.0.4, do yourself a favor and UPDATE your jars.

Easy way: just update your xwork jar file (download the full lib here)
Better way: update to Struts2.0.9

I know why a Struts 2 File Upload fails

Yesterday I began some work on using Struts2 to do a file upload. The way it works is pretty slick. At least when it works.

The problem was that when uploading files, I'd be surprised with null values. However, when pressing the back button and re-submitting the form, it would work fine. Odd behavior indeed, but I'm not alone. Other people have had the same problem uploading files with Struts 2.0.6.

I dug into it a little bit. To quite a deep level actually, and I found the bug. The good news is that it seems to be related specifically to Struts 2.0.6. There's a point in the Filter, which serves as the starting point of the whole framework, where the HttpServletRequest is supposed to be wrapped by the appropriate class for later data retrieval.

In the case of the mysterious broken file upload, the interceptor (FileUploadInterceptor) that handles populating the struts action with the values checks to see if the Request object is an instance of the MultiPartRequestWrapper. If it is, the interceptor attempts to pull out the multipart data; that is, the actual file bytes. If it's not, it does nothing. When the file uploads don't work it means that the Request has not been wrapped in the MultiPartRequestWrapper. Therefore the FileUploadFilter doesn't do anything, even though the data is actually there.

The reason why it doesn't get wrapped relates to the FilterDispatcher (org.apache.struts2.dispatcher package) and the prepareDispatcherAndWrapRequest method. In that method, there's a logic block which deals with the Dispatcher instance. When the instance is null, it sets the Dispatcher instance to one that will work. The peculiar thing about this block, is that the part of the method that does the request wrapping is also in that logic block. Therefore, the request doesn't get wrapped in the MultiPartRequestWrapper unless the Dispatcher instance is null. When the file uploads failed, this instance was NOT null, causing the Request to push through as the RequestFacade. The FileUploadInterceptor doesn't work with that data type.

The fix? Just put the code that does the wrapping outside of the logic block that deals with the dispatcher instance.

To the Struts team's credit, it looks like this bug has been fixed. The code in the repository after the 2.0.6 tag does the wrapping correctly.

Here's some code for those who care.

First, the code that comes with Struts 2.0.6

    protected HttpServletRequest prepareDispatcherAndWrapRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException {

    Dispatcher du = Dispatcher.getInstance();

    // Prepare and wrap the request if the cleanup filter hasn't already, cleanup filter should be
    // configured first before struts2 dispatcher filter, hence when its cleanup filter's turn,
    // static instance of Dispatcher should be null.
    if (du == null) {

        Dispatcher.setInstance(dispatcher);

        // prepare the request no matter what - this ensures that the proper character encoding
        // is used before invoking the mapper (see WW-9127)
        dispatcher.prepare(request, response);

        try {
                // Wrap request first, just in case it is multipart/form-data
                // parameters might not be accessible through before encoding (ww-1278)
                request = dispatcher.wrapRequest(request, getServletContext());
            } catch (IOException e) {
                String message = "Could not wrap servlet request with MultipartRequestWrapper!";
                LOG.error(message, e);
                throw new ServletException(message, e);
            }
    }
    else {
        dispatcher = du;
    }
    return request;
}

Code that works:

    protected HttpServletRequest prepareDispatcherAndWrapRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException {

     Dispatcher du = Dispatcher.getInstance();

     // Prepare and wrap the request if the cleanup filter hasn't already, cleanup filter should be
     // configured first before struts2 dispatcher filter, hence when its cleanup filter's turn,
     // static instance of Dispatcher should be null.
     if (du == null) {

         Dispatcher.setInstance(dispatcher);

         // prepare the request no matter what - this ensures that the proper character encoding
         // is used before invoking the mapper (see WW-9127)
         dispatcher.prepare(request, response);

     }
     else {
         dispatcher = du;
     }
      

        //Note that this wrapping code is where it should be
        try {
            // Wrap request first, just in case it is multipart/form-data
            // parameters might not be accessible through before encoding (ww-1278)
            request = dispatcher.wrapRequest(request, getServletContext());
        } catch (IOException e) {
            String message = "Could not wrap servlet request with MultipartRequestWrapper!";
            LOG.error(message, e);
            throw new ServletException(message, e);
        }
     return request;
 }