[Esapi-dev] Help regarding issue 251

August Detlefsen augustd at codemagi.com
Tue Oct 7 00:18:09 UTC 2014


Actually, with the code change I proposed, the tests fail. I think this
actually is due to a flaw in the test methodology itself:

The test instantiates a DateFormat like this:

DateFormat format = SimpleDateFormat.getDateInstance();

SimpleDateFormat.getInstance() will return a lenient date format using
Java's default 'MEDIUM' pattern. When you format a date with the MEDIUM
pattern, it returns a String like this:

Oct 6, 2014

But because the formatter is lenient, it is stall able to parse full dates
like:

October 6, 2014

into a valid Java Date object.

In this case, the tests all fail because the input "September 11, 2001"
does not match the formatted parsed date, "Sep 11, 2001". I could easily
change the test methodology to use a fully-specified date formatter like:

DateFormat format = new SimpleDateFormat("MMMM dd, yyyy");

and all tests would pass, BUT, what is the expected operational use case of
ESAPI? Do we need to support lenient date formats? Or can developers be
expected to specify full date formats when using ESAPI?

Thanks,
August


On Sat, Oct 4, 2014 at 3:28 AM, Fabio Cerullo <fcerullo at owasp.org> wrote:

> 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> 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>
>>> 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.
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.owasp.org/pipermail/esapi-dev/attachments/20141006/1b4a5d5d/attachment.html>


More information about the Esapi-dev mailing list