[OWASP-ESAPI] Validator class - inadequate validation
Jim Manico
jim at manico.net
Tue Jan 22 15:56:56 EST 2008
The "context" addition for 1.1 is very useful, nice addition. I'll
surrender to regular expressions at the configuration level and will
give up on my Alpha/Numeric helper functions.
> The length option is more difficult. I can see that many fields will
have the same regex, except for the length. So as much as I want
one-and-only-one place to define validation rules, I can see the benefit
of saying a field is five numeric characters. I need to think on this
some more.
This is exactly the issue I was trying to address. If you can come up
with a API level/RegEx hybrid, I think it will add to the power of the
API. This is also very critical for Alpha or hybrid fields as well. Even
email type regular expressions should be protected in such a way, IMO so
a overly long but legal email address with the intention of trying to
force a stack trace fails gracefully.
Right on, great discussion Adam/Jeff,
Jim
> Certainly for the numbers I think this is a great idea. I'm pretty
> surprised that I didn't do it that way. I'm not sure about the Alpha
> convenience methods you suggest. Note that the API changed a bit in
> version 1.1....
>
>
>
> *public* String getValidDataFromBrowser(String context, String
> type, String input)
>
>
>
> The "context" is a string that indicates where the data came from --
> like the name of the request parameter. This makes the log entry very
> explicit. The "type" is the **name** of the regex in the
> ESAPI.properties file -- like "AlphaNumeric". This makes it very easy
> to define validators and access them from anywhere.
>
>
>
> So for your example below you would say:
>
>
>
> getValidDataFromBrowser("EmpName", "EmployeeName",
> request.getParameter( "EmpName" ));
>
>
>
> I'm not crazy about the extraAllowedCharacters option. I want to
> encourage people to specifically define the type of data they're
> accepting into their application.
>
>
>
> The length option is more difficult. I can see that many fields will
> have the same regex, except for the length. So as much as I want
> one-and-only-one place to define validation rules, I can see the
> benefit of saying a field is five numeric characters. I need to think
> on this some more.
>
>
>
> You can see a simplification in the current signature if we use the
> current request threadlocal - it could automatically get the parameter
> from the request for you. But that would make ESAPI more like a
> framework, which I'd like to avoid. I think having a set of good
> primitive security methods is the goal. Frameworks can layer
> simplifications on top if they like.
>
>
>
> I'm glad you guys are helping out with the Validator API. I spent
> much more of the design time on some of the more innovative APIs in ESAPI.
>
>
>
> --Jeff
>
>
>
> *From:* Jim Manico [mailto:jim at manico.net]
> *Sent:* Tuesday, January 22, 2008 2:24 PM
> *To:* jeff.williams at owasp.org
> *Cc:* 'Adam Boulton'; 'owasp-esapi'
> *Subject:* Re: [OWASP-ESAPI] Validator class - inadequate validation
>
>
>
> Jeff,
>
> What do you think about adding parameters for range checking? Should
> this logic be pushed to the regular expression?
>
> validator.getValidDataFromBrowserAlphaNumeric(String
> otherAllowedCharacters,* int maxNumCharacters,* boolean isRequired,
> String value)
> validator.getValidDataFromBrowserAlphaOnly(String
> otherAllowedCharacters, *int maxNumCharacters, *boolean isRequired,
> String value)
> validator.getValidInteger(*int minValue, int maxValue,* boolean
> isRequired, String value)
> validator.getValidFloat(*float minValue, float maxValue,* boolean
> isRequired, String value)
>
> Adam, with respect I submit that this is not making the API overly
> complex, but is a part of a basic whitelist validation strategy that I
> would use day-2-day in enterprise development.
>
> I also have no problem using regular expressions for complex
> validation like email addresses, but to limit a String by a few
> characters, it seems like overkill and unnecessary separation of
> duties. And I do nod to the other school of thought (ala Struts) that
> says that ALL form parameters should be pushed to the configuration
> layer via RegEx : but only if you have the resources to do so. :)
>
> - Jim
>
>
> Great discussion guys -- You all have got me thinking hard about how
> to support optional parameters in a way that's easy and intuitive for
> developers. Let's keep searching for a great API!
>
>
>
> Note that there is a method that does something related:
>
>
>
> isValidParameterSet( Set required, Set optional )
>
>
>
> The idea was that this would prevalidate a request to determine that
> ALL required parameters are present, and the ONLY other parameters are
> in the optional set. Any other parameters provided are probably an
> attack.
>
>
>
> Still, it feels like there's some room to improve the Validation API
> for optional parameters!
>
>
>
> > I would love to sit around a table and thrash all the ideas out!
>
>
>
> That would really be great. I'm so tired of "research" that's focused
> on some obscure bug in some product. Maybe we can plan an event
> someday if ESAPI really catches on.
>
>
>
> --Jeff
>
>
>
> *From:* owasp-esapi-bounces at lists.owasp.org
> <mailto:owasp-esapi-bounces at lists.owasp.org>
> [mailto:owasp-esapi-bounces at lists.owasp.org] *On Behalf Of *Adam Boulton
> *Sent:* Tuesday, January 22, 2008 6:03 AM
> *To:* owasp-esapi
> *Subject:* Re: [OWASP-ESAPI] Validator class - inadequate validation
>
>
>
> Hi Jim,
>
> I think we must be careful that the method signatures do not become
> too complex and perform too much as it loses it's highly cohesive
> characteristic (I am not dismissing your idea at all, I find your
> approach valid and I can see where you are coming from). In my
> personal opinion I see the methods as being overly complex, but I
> appreciate we are just throwing ideas around here. I appreciate what
> you are saying about the regex expression too but I am not sure we
> should be trying to compensate for a Developers lack of skills. If
> they don't know regex now would be a good time for them to learn ;)
> Just a quick question... what would address2 be assigned if the
> boolean was false or are you expecting a ValidationException?
>
> /String address2 = validator.getValidDataFromBrowser(".#", 50, false,
> request.getParameter("address2"));/
>
> As a new concept, to tailor for your suggestion, I am now thinking we
> could associate a new instance of Validator to the requirements of
> specific types of HTML forms (bare with me here, i'm still piecing the
> idea together in my mind). So sticking to your example of a US address
> maybe we could go along the lines of.....
>
> /String non_required_fields = {"address2"}; //Anonymous array
> containing fields which are allowed to be null (i suppose we should
> whitelist instead of blacklisting :s )
>
> Validator usAddress = Validator.getInstance (non_required_fields);
> //This Validator object specifically deals with a us address.
> Construct a Validator with the non-mandatory fields / null fields.
> address2 is not required for the US address./
>
> /String address1 = validator.getValidDataFromBrowser(".#", 50,
> request.getParameter("address1"));/
>
> /String address2 = validator.getValidDataFromBrowser(".#", 50,
> request.getParameter("address2"));
> String city =
> validator.getValidDataFromBrowserAlphaOnly("", 30,
> request.getParameter("city"));
> String state =
> validator.getValidDataFromBrowserAlphaOnly("", 12,
> request.getParameter("state"));
>
> /Note that the boolean is no longer required, as it can be determined
> based on the use of the overloaded factory method as the names of the
> nullable fields were passed. The logic would then need updating in
> getValidDataFromBrowser which would use this list to determine which
> fields can be null and act accordingly. I think this approach could
> tie nicely in with Spring. I would love to sit around a table and
> thrash all the ideas out!
>
> Cheers
>
> Adam
>
>
> /
>
>
> /
>
> On Jan 22, 2008 8:55 AM, Jim Manico <jim at manico.net
> <mailto:jim at manico.net>> wrote:
>
> Adam,
>
> The current API requires that I push the required condition all the
> way to the regular expression or add unnecessary code. Not to mention
> that it does not do range checking (both in terms of value and maximum
> characters) in an of itself - again I would need to push that to the
> regular expression/configuration layer.
>
> Let's take a simple dispatcher controller from a standard MVC J2EE
> architecture that is processing an HTML Form. This is the core of all
> enterprise applications.
>
> So my form, is say, a US address form with the fields:
>
> address1 (textfield)
> address2 (textfield)
> city (textfield)
> state (textfield)
> zip (textfield)
>
> Address1, city state and zip are required field. Address2 is optional.
>
> With the current Validator class, I would need to do the following.
>
> String address1 = validator.getValidDataFromBrowser(this,
> "address_field", request.getParameter("address1"));
> //example 1 of an optional field being pushed to the regEx
> String address2 = validator.getValidDataFromBrowser(this,
> "address_field_null_ok", request.getParameter("address1"));
> //or
> //example 2 of an optional field where I would need to check if the
> result is null first
> String address2 = request.getParameter("address1");
> if (address2==null) throw RequiredException("address1");
> address2 = validator.getValidDataFromBrowser(this, "address_field",
> request.getParameter("address1"));
>
> String city = validator.getValidDataFromBrowser(this,
> "city", request.getParameter("city"));
> String state = validator.getValidDataFromBrowser(this,
> "state", request.getParameter("state"));
> String zip = validator.getValidDataFromBrowser(this,
> "zip",request.getParameter("zipCode"));
>
> *What I would like to recommend is to add the option to do is more
> straightforward white list validation at the API level if the form of:
>
> validator.getValidDataFromBrowserAlphaNumeric(String
> otherAllowedCharacters, int maxNumCharacters, boolean isRequired,
> String value)
> validator.getValidDataFromBrowserAlphaOnly(String
> otherAllowedCharacters, int maxNumCharacters, boolean isRequired,
> String value)
> validator.getValidInteger(int minValue, int maxValue, boolean
> isRequired, String value)
> validator.getValidFloat(float minValue, float maxValue, boolean
> isRequired, String value)*
>
> String address1 = validator.getValidDataFromBrowser(".#", 50,
> true, request.getParameter("address1"));
> String address2 = validator.getValidDataFromBrowser(".#", 50,
> false, request.getParameter("address2"));
> String city =
> validator.getValidDataFromBrowserAlphaOnly("", 30, true,
> request.getParameter("city"));
> String state =
> validator.getValidDataFromBrowserAlphaOnly("", 12,
> request.getParameter("state"));
> //split zip into 2 fields that are integers only for US apps
> String zip1 = validator.getValidInteger(1, 99999, true,
> request.getParameter("zipCode1"));
> String zip2 = validator.getValidInteger(1, 9999, false,
> request.getParameter("zipCode2"));
>
> Most programmers that I know, even rather senior architects, do not
> speak regular expression. That's more of an artifact of the security
> industry. I would conjecture that if we want to make ESAPI easy for
> programmers, then perhaps give them an option to add security at the
> API level.
>
> It's also a lot less complex to create and audit.
>
> I'm not saying no to the current validation API, I'm only saying "Yes,
> and..."
>
> - Jim
>
> Hi Jim, can you expand on your reasoning behind why this should be
> an implementation on the part of the Validator API? I don't
> believe this should be tied in with the logic in Validator. If the
> fields which are being passed do not require processing (as they
> are non-required elements) then why are they being validated? i.e
> if they are non-required form elements why pass them to the
> Validator anyway? I may be missing your point here, so please
> elaborate for me.
>
> Cheers
>
> Adam
>
> On Jan 21, 2008 10:22 PM, Jim Manico < jim at manico.net
> <mailto:jim at manico.net>> wrote:
>
> In web development, it's *very* common where I need to accept a
> null from the request. (Non-required form elements).
>
> Please consider adding a
>
> "boolean required"
>
> To many of these API's.
>
> For example:
>
> /isValidNumber(String s, *boolean required*)/
>
> |/getValueDate(java.lang.String context,/|
>
> |/ java.lang.String input,/|
>
> |/ java.text.DateFormat format,/|
>
> |/ /|*/boolean required/*|/)/|
>
> etc
>
> - Jim
>
>
>
>
>
> Hi,
>
> The method signature:
> /
> boolean isValidNumber(String s)
> /
> which is located in the Validator class does not contain adequate
> validation. For example, it is possible to return /true/ if the
> value "NaN" or "Infinity" is passed as an argument. If we dig down
> the package tree we can see the source of this problem lies within
> FloatingDecimal object which contains the character arrays for
> these strings and will return /true /when these particular strings
> are passed, essentially the /Double.parseDouble(String s)/ is just
> a wrapper for /public static FloatingDecimal.readJavaFormatString(
> String in ) throws NumberFormatException /. I would argue that in
> this particular circumstance /isValidNumber(String s) /should
> return /false/ if "NaN" or "Infinity" is passed as an argument.
>
> As another note what is the purpose of having an instantiation of
> Validator? Why are the methods, such as /isValidNumber(String in)/
> not static? This seems very much like a helper class to me.
>
> Cheers
>
>
>
> ------------------------------------------------------------------------
>
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
>
> OWASP-ESAPI mailing list
>
> OWASP-ESAPI at lists.owasp.org <mailto:OWASP-ESAPI at lists.owasp.org>
>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
>
> _______________________________________________
> OWASP-ESAPI mailing list
> OWASP-ESAPI at lists.owasp.org <mailto:OWASP-ESAPI at lists.owasp.org>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
> ------------------------------------------------------------------------
>
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
>
> OWASP-ESAPI mailing list
>
> OWASP-ESAPI at lists.owasp.org <mailto:OWASP-ESAPI at lists.owasp.org>
>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
>
>
>
> ------------------------------------------------------------------------
>
>
>
>
> _______________________________________________
> OWASP-ESAPI mailing list
> OWASP-ESAPI at lists.owasp.org <mailto:OWASP-ESAPI at lists.owasp.org>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.owasp.org/pipermail/owasp-esapi/attachments/20080122/f8d34011/attachment-0001.html
More information about the OWASP-ESAPI
mailing list