[Esapi-user] Fortify vs ESAPI 2.0

Chris Schmidt chris.schmidt at owasp.org
Thu Feb 3 00:20:22 EST 2011


Jim - I think perhaps you are misunderstanding me a little bit. I do think
that we should provide validation *where it makes sense* but I also think
that there needs to be a trade-off there. My integer overflow was a
over-simplified example of what I am saying.

As I stated before, I think that it is prudent and makes sense to have basic
validation in utility functions - sanity checks as you suggested. These are
simple things to add and make sense.

I think it is equally important to document that arguments containing
untrusted or unknown data should be *contextually validated* before being
passed into these utility functions to really knock the security out of the
park. 

Our current isValidFileName validation is only as good as the regexp - and
while I think that regexp is probably pretty solid, it isn't bulletproof in
every system - after all, '/etc/passwd' is indeed a valid path is it not -
likewise, so is new File("../../../../../../../etc/passwd");

:) 



On 2/2/11 10:11 PM, "Jim Manico" <jim.manico at owasp.org> wrote:

> 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
>> 
>> 
>> 
> 
> _______________________________________________
> Esapi-user mailing list
> Esapi-user at lists.owasp.org
> https://lists.owasp.org/mailman/listinfo/esapi-user

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