[Esapi-dev] Help regarding issue 251

Fabio Cerullo fcerullo at owasp.org
Sat Oct 4 10:28:07 UTC 2014


I belleve August has a point. We cannot handle all date formats that could
be represented out there, but we definitely need to make sure the ones we
accept are valid.

Fabio

On Saturday, October 4, 2014, August Detlefsen <augustd at codemagi.com> wrote:

> If you want to be able to handle non-date information in your date
> parsing, you can specify string literals in SimpleDateFormat by using
> single quotes. You just need to create a specific formatter to handle cases
> like "Today is October 3, 2014". The format String would look something
> like this:
>
> "'Today is' MMMM d, yyyy"
>
> So if someone needed to, they could definitely handle such odd cases with
> ESAPI. But IMHO the default should be to strictly validate dates to conform
> to the specified format.
>
> -August
> On Oct 3, 2014 7:04 PM, "Kevin W. Wall" <kevin.w.wall at gmail.com
> <javascript:_e(%7B%7D,'cvml','kevin.w.wall at gmail.com');>> wrote:
>
>> Pardon me for jumping in late to this discussion as well as
>> for top-posting. However, I wanted you to know that I asked
>> Nalin and 2 others he is working with to try to get some community
>> consensus on feedback about what the correct behavior should
>> be.
>>
>> The 4 of us had a discussion in a G+ Hangout last Monday
>> and some of the things that I asked them to explore was whether
>> or not these isValidDate() methods should do 'strict' checking or
>> 'loose' checking and to ask how important is keeping backward
>> compatibility with the current version.
>>
>> Let me explain. I do not think this is a black or white issue, but
>> perhaps I am misunderstanding.
>>
>> The JUnit test case that was given in issue # 251 was this:
>>
>>
>>
>> assertFalse(instance.isValidDate("datetest4", "September 11, 2001'
>> union select * from another_table where user_id like '%", format,
>> false));
>>
>> According to Andrew Gronosky, who submitted this bug, this assertion
>> currently fails. His expectation and almost everyone else's intuition
>> is that
>>    "September 11, 2001' union select * from another_table where user_id
>> like '%"
>>
>> Should not be recognized as a valid date, but it is according to the
>> current
>> ESAPI code base (or at least that was the case back in Nov 2011).
>>
>> In the remainder of the issue, Andrew also writes:
>>
>>     I believe I have traced the root cause to
>>     org.owasp.esapi.reference.validation.DateValidationRule.java line 97:
>>
>>     return format.parse(canonical);
>>
>>     According to the JavaDoc for DateFormat.parse at
>>
>> http://download.oracle.com/javase/6/docs/api/java/text/SimpleDateFormat.html#parse%28java.lang.String,%20java.text.ParsePosition%29
>> ,
>>     the parse method does not necessarily use up all characters to the
>> end of
>>     the string. Thus the current implementation apparently reads to the
>> end of
>>     the valid date and returns true, without checking whether additional
>>     characters are present.
>>
>>
>> So if that conjecture is true, and I have no reason to believe that it is
>> not,
>> then if we were to test user input for a valid date and rather than
>> placing
>> the malicious payload at the end, it was placed at the beginning and some
>> valid date was placed last, then SimpleDateFormat.parse() would consume
>> all of the input and still return true. E.g., consider the input string
>>
>>     /bin/rm -fr /; echo The day you lose your files is: October 3, 2014
>>
>> So my question to Nalin and the others was do allow things like this to
>> pass or not? If not, what about user input like
>>
>>    "Today is October 3, 2014"
>>
>> If we want 'strict' checking, then we would *only* match valid dates
>> and nothing more. If we wish to allow 'loose' checking, we would
>> accept things like "Today is October 3, 2014".  But as I noted to
>> Nalin and the others, if we wish to allow 'loose' checking, then
>> how do we decide what we should reject? I don't think we easily
>> can. We obviously cannot simply say no SQL statements as there
>> would potentially be lots of other injection attacks possible here
>> depending on the context of where the input is ultimately used
>> and ESAPI does not have that insight.
>>
>> On the other hand, as I pointed out to them, if we start doing only
>> 'strict' checking (which would be my personal preference), we will,
>> by definition break backward compatibility with previous 2.x versions
>> and by doing so, we thereby could break someone's code even
>> with legitimate input.  Personally, I don't have any issues with breaking
>> code which is likely to be dubious in the first place, but on the other
>> hand, I am not a USER of ESAPI, I am a developer of it. I have only
>> used ESAPI in JUnit test cases and that's about it, but I know that in
>> some companies ESAPI is being used enterprise wide so I thought
>> that we should try to gather a community consensus as to what the
>> correct behavior should be. Perhaps we even go as far as replace
>> the existing isValidDate() methods with some new methods and
>> deprecate the old methods.
>>
>> I don't know and I'm not sure that a community of developers (and
>> specifically ESAPI developers) should try to be answering this with
>> out input from the ESAPI *user* community, so I am also Cc'ing
>> the ESAPI user's group. (Yeah, cross-posting sucks, but I think
>> most of us involved in ESAPI development are subscribed to
>> both mailing lists.)
>>
>> Of course as a developer and security practitioner I want whatever
>> default that we come up with to be secure, but I am also aware that
>> it is vastly important that we should LISTEN to our user community
>> and not simply assume that we know better and know what they
>> want.
>>
>> Oh, and one last thing, I certainly think it would be nice if
>>    public boolean isValidDate(String context, String input, DateFormat
>> format, boolean allowNull) throws IntrusionException
>> simply called
>>    return isValidDate(context, input, format, allowNull, null)
>>
>> and we have a catch block in the latter isValidDate() that has:
>>          } catch( ValidationException e ) {
>>              if ( errors != null ) errors.addError(context, e);
>>              return false
>>          }
>>
>> instead of implementing the guts of isValidDate() twice. It makes
>> it a lot easier to maintain that kind of code as you don't have to
>> fix things in two places. I realize that it's not presently written
>> that way so this is not a knock against August for fixing it the
>> way he did, but while we are addressing this issue, it would
>> be good to re-factor at least that section of code. (And frankly,
>> I totally am not getting how this is supposed to even potentially
>> throw an IntrusionException, but that's a discussion best left for
>> another day.
>>
>> Best regards,
>> -kevin
>>
>>
>> On Thu, Oct 2, 2014 at 6:24 PM, August Detlefsen <augustd at codemagi.com
>> <javascript:_e(%7B%7D,'cvml','augustd at codemagi.com');>> wrote:
>> > Better yet would be to use the two-argument version of
>> DateFormat.parse()
>> > and pass in a ParsePosition Object. When parse() returns, you check
>> that the
>> > index of the ParsePosition is actually the end of the input String. If
>> it is
>> > not, then isValidDate() returns false.
>> >
>> > This should be faster than parsing, formatting the resulting Date, and
>> > comparing them.
>> >
>> > -August
>> >
>> > On Thu, Oct 2, 2014 at 3:09 PM, August Detlefsen <augustd at codemagi.com
>> <javascript:_e(%7B%7D,'cvml','augustd at codemagi.com');>>
>> > wrote:
>> >>
>> >> Developers might see that isValidDate() returns true and then take the
>> >> original string input and use that in subsequent operations instead of
>> an
>> >> actual Date object. isValidDate() needs to be fixed to return false if
>> the
>> >> date includes extra characters, regardless of what SimpleDateFormat
>> does.
>> >>
>> >> Attached is a proposed patch.
>> >>
>> >> -August
>> >>
>> >> On Thu, Oct 2, 2014 at 2:26 PM, Jim Manico <jim.manico at owasp.org
>> <javascript:_e(%7B%7D,'cvml','jim.manico at owasp.org');>> wrote:
>> >>>
>> >>> Well it sure is a significant bug. So how to fix?
>> >>>
>> >>> So what if you first take the string and parse it to a Date, and then
>> >>> take the same Date and format it back to a String? Assuming the format
>> >>> does not include the erroneous characters, you might be able to fail
>> >>> on validation if the original and formatted Date string do not match.
>> >>>
>> >>> This is how I'd first take it on.
>> >>>
>> >>> Maybe look for an Apache date class that is more strict?
>> >>>
>> >>> Thanks for looking at this.
>> >>>
>> >>> --
>> >>> Jim Manico
>> >>> @Manicode
>> >>> (808) 652-3805
>> >>>
>> >>> > On Oct 2, 2014, at 1:07 PM, Jim Manico <jim.manico at owasp.org
>> <javascript:_e(%7B%7D,'cvml','jim.manico at owasp.org');>> wrote:
>> >>> >
>> >>> > You do not stop injection at the input validation layer, I do not
>> >>> > think this is a good bug.
>> >>> >
>> >>> > --
>> >>> > Jim Manico
>> >>> > @Manicode
>> >>> > (808) 652-3805
>> >>> >
>> >>> >> On Oct 2, 2014, at 10:53 AM, Nalin Goel <naling1994 at gmail.com
>> <javascript:_e(%7B%7D,'cvml','naling1994 at gmail.com');>> wrote:
>> >>> >>
>> >>> >> Hi guys,
>> >>> >>
>> >>> >> I am new to open-source and would like to work with owasp-esapi.
>> >>> >>
>> >>> >> I did some research on issue 251(IsValidDate not recognizing
>> inection
>> >>> >> attacks) and would appreciate guidance as well as feedback as to
>> what our
>> >>> >> inputs might be.
>> >>> >>
>> >>> >> Any help on getting me started is appreciated .
>> >>> >> _______________________________________________
>> >>> >> Esapi-dev mailing list
>> >>> >> Esapi-dev at lists.owasp.org
>> <javascript:_e(%7B%7D,'cvml','Esapi-dev at lists.owasp.org');>
>> >>> >> https://lists.owasp.org/mailman/listinfo/esapi-dev
>> >>> _______________________________________________
>> >>> Esapi-dev mailing list
>> >>> Esapi-dev at lists.owasp.org
>> <javascript:_e(%7B%7D,'cvml','Esapi-dev at lists.owasp.org');>
>> >>> https://lists.owasp.org/mailman/listinfo/esapi-dev
>> >>
>> >>
>> >
>> >
>> > _______________________________________________
>> > Esapi-dev mailing list
>> > Esapi-dev at lists.owasp.org
>> <javascript:_e(%7B%7D,'cvml','Esapi-dev at lists.owasp.org');>
>> > https://lists.owasp.org/mailman/listinfo/esapi-dev
>> >
>>
>>
>>
>> --
>> Blog: http://off-the-wall-security.blogspot.com/
>> NSA: All your crypto bit are belong to us.
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-dev/attachments/20141004/d58bbdfe/attachment.html>


More information about the Esapi-dev mailing list