[OWASP-ESAPI] Validator class - inadequate validation
Adam Boulton
adamboulton at gmail.com
Wed Jan 23 05:33:43 EST 2008
This conversation is a good one to get the brain warmed up in the morning!
Some excellent points put across here.
* public String getValidDataFromBrowser(String context, String type, String
input)
*
Yes! I didn't see that and I like this alot.
> If you can come up with a API level/RegEx hybrid, I think it will add to
the power of >the API.
Very valid point and I do agree it will improve the richness of the ESAPI
API. I think it must be approached with caution, due to the power and
fliexbility of regex we could obviously write methods all day which would
take arguments for a regex, some of which would become overly complex. For
example, do we start putting in methods which deal with the formatting of
data too? (I certainly wouldn't recommend it). I do like the idea of this
API/regex hybrid approach provided the exposed functions remain simple, as I
said it can easily get carried away.
The details of data length is a difficult one to deal with. Could this
possibly become part of the ESAPI properties file?
On Jan 22, 2008 8:56 PM, Jim Manico <jim at manico.net> wrote:
> 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 <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<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> 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> 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
>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
>
> _______________________________________________
> OWASP-ESAPI mailing list
> OWASP-ESAPI at lists.owasp.org
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
> ------------------------------
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
>
> OWASP-ESAPI mailing list
>
> OWASP-ESAPI at lists.owasp.org
>
> https://lists.owasp.org/mailman/listinfo/owasp-esapi
>
>
>
>
>
>
>
>
>
> ------------------------------
>
>
>
>
> _______________________________________________
>
> OWASP-ESAPI mailing list
>
> 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/20080123/4cb8edb7/attachment-0001.html
More information about the OWASP-ESAPI
mailing list