[OWASP-ESAPI] Validator class - inadequate validation
Jeff Williams
jeff.williams at owasp.org
Tue Jan 22 14:50:27 EST 2008
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] 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/20080122/3b3aced5/attachment-0001.html
More information about the OWASP-ESAPI
mailing list