[Esapi-user] Fortify vs ESAPI 2.0

Chris Schmidt chris.schmidt at owasp.org
Thu Feb 3 00:01:51 EST 2011


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