[Esapi-dev] Help regarding issue 251

Kevin W. Wall kevin.w.wall at gmail.com
Sat Oct 4 02:04:07 UTC 2014


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


More information about the Esapi-dev mailing list