ESAPI.validator().getValidInput() returns misleading Exception

From Christop...@googlemail.com on July 18, 2012 12:45:47

Calling

ESAPI.validator().getValidInput("...", "%252%35252\u0036lt;", SafeString, 200, false)

with

Validator.SafeString=^[.\p{Alnum}\p{Space}]{0,1024}$

in my validation.properties throws an ValidationException instead of an IntrusionException because of the multiple and mixed encoding (Encoder.AllowMultipleEncoding and Encoder.AllowMixedEncoding are both set to false in the esapi.properties).
The expected default behavior of the getValidInput method is that it canonicalize the input and validate them after all. But when I had a look into the code I saw that it first validates the input than canonicalize it and than validate it once again and leads to the ValidationException during the first validation because % is no alphanumerical character. The api docs of getValidInput says "Input is canonicalized by default before validation. ". http://owasp-esapi-java.googlecode.com/svn/trunk_doc/latest/org/owasp/esapi/reference/DefaultValidator.html#getValidInput(java.lang.String,%20java.lang.String,%20java.lang.String,%20int,%20boolean)

Original issue: http://code.google.com/p/owasp-esapi-java/issues/detail?id=278

@kwwall

The OP is right:

/**
	 * {@inheritDoc}
	 */
	public String getValid( String context, String input ) throws ValidationException
	{
		String data = null;

		// checks on input itself

		// check for empty/null
		if(checkEmpty(context, input) == null)
			return null;

		if (validateInputAndCanonical)
		{
			//first validate pre-canonicalized data
			
			// check length
			checkLength(context, input);

			// check whitelist patterns
			checkWhitelist(context, input);

			// check blacklist patterns
			checkBlacklist(context, input);
			
			// canonicalize
			data = encoder.canonicalize( input );
			
		} else {
			
			//skip canonicalization
			data = input;			
		}

		// check for empty/null
		if(checkEmpty(context, data, input) == null)
			return null;

		// check length
		checkLength(context, data, input);

		// check whitelist patterns
		checkWhitelist(context, data, input);

		// check blacklist patterns
		checkBlacklist(context, data, input);

		// validation passed
		return data;
	}

My problem is that while I agree that we should always canonicalize before validating, I lack the history with this code to understand WHY we moved away from that behavior. Why do we check, canonicalize, and then check again? @kwwall @planetlevel ????

I might experiment by altering the code and seeing what unit tests blow up.

commit 0867375

Has comment

Validate the string both before and after canonicalization. Previously
it was only validated after. This also adds a
setValidateInputAndCanonical(boolean) method that, when passed false,
will make StringValidationRule behave as before.

To simplify this a fair bit was refactored as well.

This closes issue 54.

We no longer have access to issue #54.

I don't get it. What do we buy by regex parsing before and after canonicalization? One of the main functions of canonicalizing input is to make it safe to run against a regex since some input (like URLs) are actually complicated languages in themselves.

Hmm... this is a puzzler. I am trying to imagine why someone would have added this. Surely canonicalization before validation is a good idea. BUT, it can be dangerous to use the canonicalized form. Someone may have sent an encoded attack, which you then enabled by decoding it. So, perhaps the theory here is that while you should validate the canonicalized data, you should actually USE the original data. Which would argue for validating it also.

Personally, I'm not buying it. But at least it's a theory.

--J

@planetlevel

@planetlevel Do you know who schallee@darkmist.net is? If so, maybe we can ask. Probably won't remember though since this dates back to Nov 2009.

@xeno6696 Have you checked if there were any commits made to StringValidationRuleTest.java around that time? New or altered test cases might shed some light as to the rationale for these changes if we're lucky. I'm not good enough with git to do that so I'll leave that to you to check.

I can tell you by looking at it the answer is no.

The method in question was getValid() and while it defers to the methods shown here, that method itself was never tested. /sigh

Almost.... the method is called once in ValidatorTest on line 279.

assertEquals("Test.", ESAPI.validator().getRule("test").getValid("test", "Test. <script>alert(document.cookie)</script>"));
I experimented by commenting out the lines so that only canonicalize is called in getValid and we only get one unit test failure:

assertFalse(instance.isValidInput("test", "jeff\\WILLIAMS", "HTTPParameterValue", 100, false));

Which we can just flip our expectations here. Backslash is legal in an HTTP header. HEre's the BNF from RFC 7230:

field-name = token

token = 1*tchar

tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / 
        "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

Our regex for HTTPParameter value is here:

Validator.HTTPParameterValue=^[\\p{L}\\p{N}.\\-/+=_ !$*?@]{0,1000}$

I'm seeing no solid argument for leaving this method as it currently stands... it doesn't appear to break any other validator code. It IS tested indirectly by all of the calls to Validator.getValidInput()

I dunno. I'm for making ESAPI defaults more strict than what is allowed in all the various standards and grammars involved in web applications. I think in terms of safe-harbor best practice. Stick to these rules and it makes it more difficult to screw up. At least by default. I'm okay with asking folks to loosen their configuration if they have a specific need to do something crazy.

I agree with Jeff on this one. If they don't like the out-of-the-box ESAPI defaults they can always change them. Also, I must have looked at that BNF snippet at least a 1/2 dozen times and I am still not seeing '\' listed among the chars making up 'tchar'. Are you sure you copy/pasted the right one BNF snippet, or is Gmail just not rendering something right, or I am older and may sight is far worse than I imagined? (At this point, I'm not willing to rule any of them out. :)

…

-kevin -- Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall NSA: All your crypto bit are belong to us.

On Jul 21, 2017 12:15, "Jeff Williams" ***@***.***> wrote: I dunno. I'm for making ESAPI defaults *more* strict than what is allowed in all the various standards and grammars involved in web applications. I think in terms of safe-harbor best practice. Stick to these rules and it makes it more difficult to screw up. At least by default. I'm okay with asking folks to loosen their configuration if they have a specific need to do something crazy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#284 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3nmzIpByoOqnE2u_3bL9el-3i7h6Bmks5sQM6pgaJpZM4C6zC1> .

@kwwall

No @kwwall you're seeing it correctly. Backslash is legal, but in the unit test, it's the backslash that is getting rejected, or should I say, we're expecting the backslash to be rejected when it is in fact, legal.

Here's the relevant RFC snippet:

3.2.6. Field Value Components

Most HTTP header field values are defined using common syntax
components (token, quoted-string, and comment) separated by
whitespace or specific delimiting characters. Delimiters are chosen
from the set of US-ASCII visual characters not allowed in a token
(DQUOTE and "(),/:;<=>?@[]{}").
 token          = 1*tchar

 tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                / DIGIT / ALPHA
                ; any VCHAR, except delimiters

As far as "more secure" defaults, I'm going to push on this: How is validating the input before and after canonicalization more secure than canonicalizing it and validating it? Especially when everyone here agrees that best practice is always "canonicalize, then validate." Without a concrete case of "not doing this fails," we're appealing to FUD (Fear, Uncertainty, and Doubt) as opposed to measurable, actionable reasoning.

To make my case a bit more clear:

Validate before  |  Validate after | Validation Result
1                          1                       1
1                          0                       ?
0                          1                       ?  
0                          0                       0

Cases two and three are ambiguous to me. That's because it is only safe to perform regex matching against input that has already been reduced to its simplest form. It can actually be legitimate that a value would fail validation prior to being canonicalized, take the simple case of an HTML encoded string with a validation regex that punts ampersands. I have a harder time thinking of a case where case 2 would be legitimate, but since I can't think of a concrete reason, I'm leaving it "?"

@xeno6696

@xeno6696 You wrote:

No @kwwall you're seeing it correctly. Backslash is legal, but in the unit test, it's the backslash that is getting rejected, or should I say, we're expecting the backslash to be rejected when it is in fact, legal.

What I was referring to was this:

tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*"
/ "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
/ DIGIT / ALPHA
; any VCHAR, except delimiters

There is NO BACKSLASH mentioned there. (Do you see it here?) I went and read section 3.2.6 of RFC 7230. It does mention backslash, but as I read it, it is in a context that it generally is not allowed...

Delimiters are chosen
from the set of US-ASCII visual characters not allowed in a token
(DQUOTE and "(),/:;<=>?@[]{}").

(Backslash is there, between the square brackets, 'NOT allowed in a token". It is also mentioned a little later on, in the context of quoting:

The backslash octet ("") can be used as a single-octet quoting
mechanism within quoted-string and comment constructs. Recipients
that process the value of a quoted-string MUST handle a quoted-pair
as if it were replaced by the octet following the backslash.

 quoted-pair    = "\" ( HTAB / SP / VCHAR / obs-text )

A sender SHOULD NOT generate a quoted-pair in a quoted-string except
where necessary to quote DQUOTE and backslash octets occurring within
that string. A sender SHOULD NOT generate a quoted-pair in a comment
except where necessary to quote parentheses ["(" and ")"] and
backslash octets occurring within that comment.

Regardless, I suspect when all this was originally written, RFC 2616 was still at least the predominant HTTP 1.1 standard and it very well could have been that the ESAPI regex was formulated after studying the specific mentioned HTTP request and response headers in that RFC with perhaps a few commonly observed oddball cases thrown in for good measure.

Regarding the "safe-harbor" that @planetlevel mentioned... perhaps things are the way they are been as a reaction to MSIE's over zealous double-encoding that resulted in a lot of vulnerabilities at the time when ESAPI was first being written. And I don't necessarily see Jeff's "safe-harbor" philosophy as FUD, but rather "defense-in-depth". Being "liberal in what you accept" is a good philosophy for interoperability, but IMO, a poor approach to software security. Sometimes it pays to err on the side of safety especially if it is something that can be tweaked via properties. (More on that later)

The other thing which is confusing me here (and note, I've not looked at that code in quite a while so I am more likely the one who is confused here), is that your discussion starts out mentioning the BNF for an HTTP header name, but then you reference the property Validator.HTTPParameterValue. is that property not for an HTTP header value? I ask because also have an Validator.HTTPParameterName property, so it seems perhaps to me that we are conflating issues here. I'm also curious where you pulled out that property value for Validator.HTTPParameterValue. I only see that in "src/test/resources/esapi/ESAPI.properties", which I think gets copied to "target/test-classes/esapi/ESAPI.properties", but the ESAPI.properties file that we tell people they should be using is the one in "configuration/esapi/ESAPI.properties", and that one is does allow '':

Validator.HTTPParameterValue=^[a-zA-Z0-9.\\-\\/+=@_ ]*$

(Although, again, that's for HTTP header values, not header names.)

Regardless, it's probably not a good thing that we are deploying one as the (so-called) production ESAPI.properties file and recommending people use that, but testing with one with different values.

Overall, even if we decide to change regex parsing before and after canonicalization, aren't we concerned that we are going to break something for someone? If we do it in a way that can be "fixed" by altering the regex or setting a property to revert to the old behavior, I guess I don't have any major objections to it, but I certainly would like to preserve backward compatibility if we don't have to jump through major hopes to do so. Otherwise, someone will soon be submitting a but that we've broke some obtuse corner-case for someone that they were relying on. Since you (@xeno6696) are the one who has taken on this GitHub issue, I will leave the ultimate decision up to you. (First rule of software development: You touched it last so you own it! :) Seriously, I trust your judgement.

What I was referring to was this:

tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*"
/ "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
/ DIGIT / ALPHA
; any VCHAR, except delimiters

Derp....

I have no excuse for this.

Regarding the rest, having worked on production systems I'm more inclined to leave things as they are and simply adjust the documentation. /sigh.

I will think long and hard before coming to a decision on this. It might go longer than the current release cycle.

Okay, after doing some digging, the issue brought up here is caused by a commit that should never have been. #54 clearly shows that the bug problem was a user with URLs that had HTML entities in it. (Which is now fixed by DefaultEncoder.getCanonicalizedURI. )

So... the code in StringValidationRule should never have been implemented and the original behavior--canonicalize then check white/blacklists should be restored.

Oh, FWIW, the "SafeSpace" regex denied the input discussed here, so even though it should have been canonicalized first (and wasn't) it was still rejected in this case.

Merge pull request #420 from xeno6696/develop

Navigation Menu

Repository navigation

ESAPI.validator().getValidInput() returns misleading Exception #284

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ESAPI.validator().getValidInput() returns misleading Exception #284

Description

Activity

xeno6696 commented on Jul 19, 2017

xeno6696 commented on Jul 19, 2017

xeno6696 commented on Jul 19, 2017

xeno6696 commented on Jul 19, 2017

planetlevel commented on Jul 20, 2017

kwwall commented on Jul 20, 2017

xeno6696 commented on Jul 20, 2017

xeno6696 commented on Jul 21, 2017

xeno6696 commented on Jul 21, 2017

xeno6696 commented on Jul 21, 2017

planetlevel commented on Jul 21, 2017

kwwall commented on Jul 21, 2017

xeno6696 commented on Jul 21, 2017

xeno6696 commented on Jul 21, 2017

kwwall commented on Jul 22, 2017

xeno6696 commented on Jul 22, 2017

xeno6696 commented on Aug 26, 2017

xeno6696 commented on Aug 26, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions