[Esapi-user] Fortify vs ESAPI 2.0

Jim Manico jim.manico at owasp.org
Thu Feb 3 00:42:19 EST 2011


This code is public because someone cut and pasted a generic Base64 chuck of code from another project without vetting.

> What if I want to be able to write to a relative path that is a sibling orparent of my current path from my code?

Then we build you a new secure API for that use case if the current methods  do not satisfy your security need.

Java is highly vulnerable to path traversal attacks. Jeff wrote several very impressive fileIO validators that will stand the test of time. With a little modification we can extend the APIs in a secure fashion to fit your need. 

-Jim Manico
http://manico.net

On Feb 2, 2011, at 11:35 PM, Chris Schmidt <chris.schmidt at owasp.org> wrote:

> What if I want to be able to write to a relative path that is a sibling or
> parent of my current path from my code?
> 
> Now we have provided a broken API by adding this validation. You are
> battling 2 specifications here, and I think that causes some problems. I
> personally have used some of the ESAPI utility functions for things that had
> absolutely nadda to do with the webapp itself, just because they were
> available to use. So do we go out on the limb and say that ESAPI utilty
> functions should always assume that *all data* being handed to them is from
> an untrusted source?
> 
> I think that is really what all of my side of the debate boils down to -
> being a security API - should it be implied that the methods we provide will
> treat all data as untrusted data - when in fact, a good deal of the time -
> the code that calls utility methods like this will not in fact be calling it
> with untrusted data? (at least in any app that I wrote - perhaps that is
> just my way of thinking tho)
> 
> Then here is the real question - why is this method public? Why are we
> concerning ourself with exposing publicly utility methods of this nature
> when we should really be *only* exposing methods that are defined in the API
> itself, hence the purpose of the contract (interface)
> 
> Is this method public because it is being called internally? Perhaps this
> method is in the wrong place then? Is anything outside of the Base64 Codec
> calling this method? Why?
> 
> Again, this is only a single example - and I think we have already come to
> an agreement about what we need to do in this particular case (I am fine
> with either Kevin's or your solutions)
> 
> Another question tho, regarding Kevin's comment - why are we copy/pasting
> code from another library? Is the purpose to not drag in another dependency?
> Seems like a huge antipattern to me to work it this way, but mayhaps there
> was a good reason behind doing this?
> 
> 
> On 2/2/11 10:22 PM, "Jim Manico" <jim.manico at owasp.org> wrote:
> 
>> 
>>> 
>>> 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
>> 
> 
> 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