[Esapi-user] Fortify vs ESAPI 2.0

Jim Manico jim.manico at owasp.org
Thu Feb 3 00:22:51 EST 2011


> 
> 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");

That is why we have Validator.getValidDirectoryPath functions. Theses
are not RegEx based and stop path traversal attacks of this nature - in
any OS.

- Jim





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



More information about the Esapi-user mailing list