[Esapi-user] Fortify vs ESAPI 2.0

Jim Manico jim.manico at owasp.org
Wed Feb 2 23:59:28 EST 2011


Chris,

Again - I respectfully but forcibly insist that all utility functions have security built in. Here is a simple way to pull this off:

Change:

public static boolean decodeFileToFile( String infile, String outfile )

to

public static boolean decodeFileToFile( File inputDirectory, String inputFile, File outputDirectory, String outputFile ) {

And then call at least isValidFilename on each file, and then call getValidDirectoryPath on each file/directory pair.

This is a rather simple change and will not cause harm, even if it does deprecate the current function signature.

Why would we not want to do this, or something like it?

Triage of these issues is a pain, but again, we are a security library. Folks are depending on us to get this 100% right, or at least 99% right. We have made a LOT of critical mistakes around crypto, output encoding (AntiSamy and the CSS encoder), leaking of data (logging passwords and crypto tokens), etc. We CAN fix it, and we will, but until we do - lets keep calling it BETA. Its the honest thing to do. This is why I'm such a insane stickler about calling all of ESAPI BETA right now. Our "little mistakes" including performance are real issues. Comon, we are not even closing streams in some of the WAF code. That's really a coding 101 issue.

And I am trying to help. I'm paying my own way to the summit. I'm going to grind out some code with you and various other teams at the summit. We can fix this, we can make it a very high quality production library. We might need (as OWASP) to hire full time support to pull it off. But until we do, we are BETA regardless of what we label it.

- Jim



> Herein lies the problem with most static analysis - a complete lack of
> context.. This could potentially be an issue if we are ever passing
> untrusted data into this method, but this is more of a documentation issue
> that anything else I think. This is a standard utility method that *must
> never* be called without first validating the parameters. Short of that you
> are talking about adding another layer of path validation here and that is
> additional configuration as well as potential difficulty for OS specific
> validation functions. I think issues like this should be left well enough
> alone and the documentation should state that the parameters of this
> *utility method* are not validated and as such *must be validated prior to
> calling the method* according to the validation requirements of the calling
> context. 
> 
>> There are also several potential real findings (path manipulation in our
> Base64 encoder)
>>
>> public static boolean decodeFileToFile( String infile, String outfile )
>>     {
>>         boolean success = false;
>>         java.io.InputStream in = null;
>>         java.io.OutputStream out = null;
>>         try{
>>             in  = new Base64.InputStream(
>>                       new java.io.BufferedInputStream(
>>                       new java.io.FileInputStream( infile ) ),
>>                       Base64.DECODE );
>>             out = new java.io.BufferedOutputStream( new
>> java.io.FileOutputStream( outfile ) );
> 
> In other words if I call the method like this:
> 
>    String inFile = request.getParameter("inFile");
>    String outFile = request.getParameter("outFile");
>    if ( Base64.decodeFileToFile(inFile,outFile) {
>       // ...
>    }
> 
> Then, yes - there is a real security issue there. However, if I am calling
> it like:
> 
>    if ( Base64.decodeFileToFile(
>         ARBITRARY_FILE,
>         Constants.TMP_DIR + "valid-file-" + System.currentTimeMilliseconds()
> ) {
>         // ...
>     }
> 
> Then would you still consider that to be vulnerable code? Perhaps if the
> first argument wasn't being validated somehow, it could be possible to write
> portions of files out to a predictable location on the hard drive - but you
> are controlling that location, and you are keeping outside of the proverbial
> sandbox (hopefully) so that in order to get to that file, there would have
> to be some type of additional vulnerability (LFI) to really exploit the
> weakness. You show me the static analysis that can identify that type of
> problem and I will buy you an island.. :)
> 
> Maybe I am looking at this too critically, but this is just one of the
> "potential issues" reported - and really if I was too spend this much time
> (10-15 minutes) analyzing each *potential finding* (and it would likely be
> more like 30-60 minutes for a good analysis of each) we would not be
> releasing GA until 2046..
> 
> Don't get me wrong Jim - I think this kind of thing is at least valuable to
> know, and I haven't yet had a chance to actually look at the findings
> reported by Fortify, however, I think that due to the nature of the code -
> static analysis just really can't offer a super viable and accurate analysis
> of ESAPI. It is a library at the end of the day, and while there are likely
> things in the library code that have separate issues (like the System.out of
> the password) the vast majority of findings that are going to be reported
> just don't make sense since any method (if used incorrectly) could generally
> be exploited to do something wrong, somehow.
> 
> Like you said, we aren't perfect and we definitely don't write perfect code
> - the best thing we can do with findings like these is document the intended
> purpose and hope like hell that developers pay attention to our warnings and
> documents.
> 
> Wow - I am starting to write e-mails like Kevin; short novellas. :)
>  
> 
> On 2/2/11 8:34 PM, "Jim Manico" <jim.manico at owasp.org> wrote:
> 
> 
> Chris Schmidt
> ESAPI Project Manager (http://www.esapi.org)
> ESAPI4JS Project Owner (http://bit.ly/9hRTLH)
> Blog: http://yet-another-dev.blogspot.com
> 
> 
> 



More information about the Esapi-user mailing list