[Esapi-user] Fortify vs ESAPI 2.0

Jim Manico jim.manico at owasp.org
Thu Feb 3 00:11:13 EST 2011


This is 100% wrong thinking Chris, in my opinion. And this is a good debate, I mean no disrespect to you.

"JavaDoc about proper use" is not security. Building validation in IS security. We can even fix the contextual validation output error messages with solid input validation. (only allow context with A-Za-z)

We can do OS independent validation of file paths, we are doing it already in the Validator per my previous email.

There are whole classes of "safe numeric" libraries that handle Java's overflow problems which are common in the finance sector (these libs handle the problem you are describing below around Integer overflow).

We can design and build good security into most API's. We SHOULD design and build good security into at LEAST ESAPI's API's. If we cannot? Then all hope is lost.

- Jim


> So how do we write path validation that works on my Mac, my linux laptop, my
> windows 7 development box, it's cygwin environment, and running any webapp
> in the world? 
> 
> There is no context for the validation and while *maybe* it makes sense to
> validate against simple *blankie* rules like no '..' and no '|' in the
> passed in parameters, it still should be documented that the values here
> *need* to be validated before they are passed into the method.
> 
> What about the method:
> 
>  public static int addTwoNumbers( int a, int b ) {
>     return a+b;
>  }
> 
> Seems simple enough, but it is vulnerable to Integer Overflows! Should we
> write the method as:
> 
>  public static int addTwoNumbers( Number a, Number b ) {
>     if ( a > Integer.MAX_VALUE ) ||
>         ( b > Integer.MAX_VALUE ) ||
>         ( a < Integer.MIN_VALUE ) ||
>         ( b < Integer.MIN_VALUE ) ||
>         ( ( a+b ) > Integer.MAX_VALUE ) ||
>         ( ( ( a+b ) < a ) && a > 0 ) ||
>         ( ( ( a+b ) < b ) && b > 0 ) )
>         // ...
>             throw new ValidationException("YARRRRR, What kinda programmer
> arrrr ya?");
> 
>     return a+b;
>  }
> 
> My point here is that *every* method is vulnerable if it is not used
> correctly - and that is doubly true for utility methods. We may be a
> security library, but we can't predict every possible way that calling a
> method could go wrong and defend against that, so we educate the person
> calling the utility method by instead writing the above method as:
> 
>  /**
>    * Adds two integers together. It is possible to create an integer
>    * overflow resulting in broken math if the result of a+b is greater than
>    * the value of {@link Integer.MAX_VALUE} or less than the value of
>    * {@link Integer.MIN_VALUE)
>    * 
>    * @parm a The first integer value
>    * @parm b The second integer value
>    * @return The value of a+b
>    */
>  public static int addTwoNumbers( int a, int b ) {
>     return a+b;
>  }
> 
> I believe that we also always say that 'contextual input validation' is the
> right way to go, just like 'contextual output encoding' right?
> 
> That being said, in this particular case, I concede that adding a simple
> check for the no-dot-dot and no-pipe in the passed in parameters is probably
> not too difficult and elimates most of the obvious vulnerability of the
> code.
> 
> 
> On 2/2/11 9:42 PM, "Jim Manico" <jim.manico at owasp.org> wrote:
> 
>> I very strongly disagree, Chris. Utility functions like this should have
>> validation built in (like our file validation APIs). There is no excuse
>> to NOT add this validation in - especially for public utility functions.
>>
>> We are a security library after-all...
>>
>> McGraw was right all this time. Build Security In!
>>
>> - 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
>>>
>>>
>>>
>>
> 
> 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