The filter seems to corrupt certain files

[julienfromentc]

Hello,

After testing the latest filter, we found some case where when we upload certain files, the files that gets saved is "corrupted".

You can find an offending file here: https://github.com/julienfromentc/struts1filter-issue/raw/master/PDF.PDF, in this case the resulting PDF will be blank page.

Update:
It seems the issue happens with the following characters:

• 81
• 8D
• 8F
• 90
• 9D

Those characters are undefined in Windows1252 (http://en.wikipedia.org/wiki/Windows-1252). After the string conversion, those chars are transformed to '?'. So if we could convert the read the inputstream in an other encoding, the issue could be fixed

[pwntester]

Sorry for the delay.

I tried settting the request encoding to UTF-8 before reading the inputStream: request.setCharacterEncoding("utf-8");

I also added the original encoding to the InputStreamReader:
String characterEncoding = this.getCharacterEncoding();
if (characterEncoding == null ) {
bufferedReader = new BufferedReader(new InputStreamReader(inputStream));
}
else {
bufferedReader = new BufferedReader(new InputStreamReader(inputStream, characterEncoding));
}

But I still get some bytes converted. Note that those bytes are read that way by inputStream. You can get the bytes from body and print them and you will see that the bytes after %PDF-1.2% are efbfbdef ... instead of e2e3cfe30d0a...

@rgielen any ideas?

[julienfromentc]

@pwntester @rgielen Did you guys had any chances to look at this issue a bit?

[pwntester]

nothing new since last update. @rgielen can you please look at it? Any idea if Struts is finally going to release a new version using the secure BeansUtil version??

[rajeshmanoharann]

In below PR, which is raised to struts repo, has change in RequestUtils which checks request.getParameterNames alone. But this filter checks input stream also. Is it really needed to check input stream?

apache/struts1#1

[pwntester]

Yep, it is since payload can be placed in a multipart post request

[pwntester]

I think using a fixed version of Struts 1 is a much better option than using the filter since unfortunately its not as stable as we would like it to be. And actually applying a patch should not be that difficult. The official patch uses a new version of BeanUtils that basically applies the regular expression. It should be similar to:
https://gist.github.com/ykare/1fb2d5d11e16cb0003c3#file-2-beanutils-patch-struts1-0-2

But instead of patching BeanUtils (used by Struts), you can directly patch Struts library, like this pull request proposes:
https://github.com/apache/struts1/pull/1/files

Another alternative is to download a struts version that comes with the patch. Fedora has applied it:
http://koji.fedoraproject.org/koji/buildinfo?buildID=552377

[jimnz111]

I'm quite late in chiming in on this issue. We had to implement the filter temporarily to pass security requirements. I too came across this issue for multipart form data. Tracked issue back to the conversion of the InputStream which is byte[] array to a char[] array. You cannot convert binary data to char and back again and hope it won't change! The fix for this is to instead read the full byte[] stream and store that, whilst still doing conversion of body to char array to check for malicious strings. The subsequent call in getInputStream() should return the original byte[] array instead of body.

Side note that the 1.0.1-SNAPSHOT also doesn't work. It appears that the removal of the call to super.getParameterNames() in constructor causes the filter to fail! I haven't debugged through to see why its needed, however leaving it in keeps everything working

[ilatypov]

Thank you @jimnz111 for noticing the input swallowing in the 1.0.1-SNAPSHOT filter and analyzing the cause.

Using Alberto Fernández's maintenance branch of Struts 1 which included the fix in ActionServlet and the fix in RequestUtils protected against the RCE ".class." attack.

The mass assignment of business-sensitive fields (IDOR) needs addressing by a white-listing technique such as defining dedicated Data Transfer Objects.

P.S. A hack in doFilter (after undoing the Jan 22, 2015 change) can preserve binary uploads at the expense of corrupting non-UTF8 text inputs.

        request.setCharacterEncoding("UTF-8");
        response.setCharacterEncoding("UTF-8");