esapi-java-legacy

esapi-java-legacy copied to clipboard

From [email protected] on May 15, 2010 21:42:05

What steps will reproduce the problem? 1. Wrap a request that contains a % escape in the query string 2. Call getQueryString() 3. note that percent escapes have been unescaped What is the expected output? What do you see instead? This is debatable. The problem here is that unescaping % changes the meaning of the query string. In normal form encoding name1=value1&name2=value2, equals and ampersand must be escaped in names and values. However, the unescaping that getQueryString is performing would cause the values not to parse correctly. Please use labels and text to provide additional information. It needs to be decided what is best to do here. The purpose of this routine makes canonicalization and validation difficult at best because it bypasses the normal decoding into parameters that is done internally by the request. I have seen urls with other meta characters in the query string than those done for form encoding though I don't know what the specification says about such.

My current thought on the issue is that this routine is generally expected to return a raw value. As such it would probably be best just to validate that the characters contained therein are valid characters for a query string and not go farther. I believe this would just be URL chars without the '#' but I would need to test.

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

From [email protected] on May 15, 2010 18:44:22

It also seems like throwing a run exception would be better than returning an empty string when validation fails.

From [email protected] on November 02, 2010 00:50:09

Agreed - this should be resolved before 2.0GA.

Labels: Milestone-Release2.0

From [email protected] on November 18, 2010 18:37:56

Labels: Filter

From [email protected] on November 20, 2010 15:30:44

Labels: -Filter Component-Filter

From [email protected] on February 11, 2011 22:51:11

Jim: In Comment 2, what are you agreeing with? Comment #1, about throwing some sort of RuntimeException or the general description of this issue or both?

Cc: manico.james

@jeremiahjstacey this is where you should be committing your work against.

What do you want this to do? It's catching the ValidationException and swallowing it. I can either wrap it in a runtime exception and rethrow, or log it. If it's logged, it will not change the blank string returned to the client. If the logging route is chosen, I would appreciate confirmation that the blank string is desired. Otherwise, I think the only other option I have is to return the original Input reference.

I am of the opinion that this is an exception case, and a Runtime exception should be thrown, allowing the context to escape to the execution container. I intend to move in this direction, but it will be easy enough to re-route if needed.

There are other places in the SecurityWrapperRequest where similar ValidationExceptions are being caught. In some cases they appear to be logged, in others they are swallowed. Should the decision made for resolving this instance be applied to those other locations as well?

@jeremiahjstacey IF the comments '// already logged' are incorrect, then I think at an absolute minimum we should add logging here (I'm going to suggest at '.warning' level to match the others, but I could be persuaded otherwise). I am hesitant to make the DEFAULT behavior to either rethrow a ValidationException since this would require signature changes on the methods involved in terms of their throws claus(es) which [in theory at least] should require a bump in the 'major' version number). And even if we were to throw some sort of RuntimeException (perhaps a new custom ESAPI subclass?), I'm still somewhat against that. That could cause even more subtle code breakage since the compiler would not catch it. But if whatever code that is using SecurityWrapperRequest was not already catching something like RuntimeException or a superclass, it could mysteriously 'break' code at runtime, which is almost worse than breaking it at compile time. And if we 'break' code either at compile time or runtime, that only leaves 2 possible outs...one is to roll back to a previous version of ESAPI and the other is to fork the ESAPI code and fix your own local copy to do what you want, neither of which in my mind are good options.

My concern is that while I think we all feel this is a "bug" it may have accidentally become a "feature" and developers are relying on this behavior. Change the behavior and at best we break someone's regression tests. At worst, we break someone's production code.

So as much as I hate do do this, I think the only way to handle this is to only throw an exception if and only if some new ESAPI property is set to true. (I'm not sure what to call it; maybe a boolean like 'Validation.MultiencodingBugWorkaround, and if it is set to 'true' then throw some sort of RuntimeException (or custom subclass) with a suitable exception message. (Maybe name it something like ValidationRuntimeException??) Note that I'd be okay with setting this property to 'true' in our ESAPI.properties file and just commenting it appropriately there and in the Javadoc.

And, yes, IF you think that this same problem exists and some sort of RuntimeException really should be thrown where ValidationException is currently silently being swallowed, then by all means do a similar thing for them.

Just my $.02, -kevin

@kwwall I had been going down those paths as well. I completely agree with your concerns about altering production behavior. I believe that the most consistent course of action, and the least damaging, is to log all caught exceptions in this class, as some of the instances are already doing.

This may result in some content being logged twice, yes; however, from the context and scope of the SecurityWrapperRequest implementation, it should not/does not matter. If the exception is handled elsewhere, then it should be handled and not allowed to escape, and a reasonable return value should be provided back to the caller. If a public API (like ESAPI.validator()) throws an exception, that exception can really only be handled by the caller from the context it was invoked. Assuming that the exception was dealt with in another method in the call stack is unsafe, as external implementations will change without the downstream contexts.

I would suggest that duplicate logging events from this change be filed as individual bugs as they crop up, and each code path should be individually investigated. Trusting handling from any class-external sources is, in my opinion, a bug waiting to happen.

---

In summary, I'm altering my course to now apply consistent logging in the SecurityWrapperRequest rather than rethrowing a RuntimeException.

The question still remains though, what is a valid return? Is a blank string expected to be returned if the original input cannot be processed? This seems to be the case in all instances of the class, but this issue appears to be implying that it's undesired.

@kwwall @xeno6696 I have confused contexts around this conversation.

My aforementioned suggestions for logging and changing the return are not addressing the primary concern of this task, which is that the decode is working as expected.

The problem is, that when the PercentCodec successfully decodes the input, the decoded string is no longer compliant with the expectations of the HttpServletRequest query string usage context. So the "intended destructive nature" of the Validator.getValidInput method is actually undesired in this situation.

With that understanding (which I now believe to be accurate), I believe the correct solution is to have the SecurityWrapperRequest.getQueryString return the original queryString, so long as the validator is able to process that content successfully. In this case, I am defining success as having the call to Validator.getValidInput returning a Not Null & Not Empty value.

This can be achieved by modifying the return statement of the function:

 public String getQueryString() {
        String query = getHttpServletRequest().getQueryString();
        String clean = "";
        SecurityConfiguration sc = ESAPI.securityConfiguration();
        try {
            clean = ESAPI.validator().getValidInput("HTTP query string: " + query, query, "HTTPQueryString", sc.getIntProp("HttpUtilities.URILENGTH"), true);
        } catch (ValidationException e) {
            e.printStackTrace();
            // already logged
        }
        /*
         * GIT Issue #135 As long as the original query can be cleaned it's safe -- return the original value since the
         * clean value is now decoded.
         **/
        return clean == null || clean.isEmpty() ? clean : query;
        //return clean;
    }

This change fixes one of the two commented-out tests. testGetQueryStringPercentEquals will work with this modification; however, it breaks another test.

testGetQueryStringPercent now fails. Since we're now returning the original String, the assertion now fails. It's easy enough to update it to change the assertion, but I'm not sure if it's the right thing to do.

	public void testGetQueryStringPercent()
	{
		MockHttpServletRequest req = new MockHttpServletRequest();
		SecurityWrapperRequest wrappedReq;

		req.setQueryString("a=%62");
		wrappedReq = new SecurityWrapperRequest(req);
		assertEquals("a=b",wrappedReq.getQueryString());
		//GITHUB #135  -- Is this the right thing to do?
		//assertEquals("a=%62",wrappedReq.getQueryString());
	}

---

testGetQueryStringPercentAmpersand still fails. I believe that this test has an incorrect environment expectation. The value is being correctly decoded by the DefaultEncoder, but the decoded value is not compliant with the system's configured HttpHeaderRequest whitelist regex. As such, the ValidationException is thrown and a blank string is returned to the caller.

I've outlined the workflow below:

Given INPUT = "a=%26b" GIVE INPUT_TYPE = "HTTPQueryString" GIVEN INPUT_TYPE_WHITELIST_PATTERN = "^([a-zA-Z0-9_-]{1,32}=[\p{L}\p{N}.-/+=_ !$?@%]&?)*$" GIVEN CODECS = "HTMLEntityCodec, PercentCodec, JavaScriptCodec" GIVEN PERCENT_CODEC_DECODE = "a=&b"

1. Client makes request to SecurityWrapperRequest.getQueryString
2. SecurityWrapperRequest extracts INPUT from the HttpServletRequest
3. SecurityWrapperRequest attempts to clean INPUT AS INPUT_TYPE through ESAPI.validator().getValidInput(...)
4. DefaultValidator creates a new StringValidationRule for INPUT_TYPE.
5. DefaultValidator derives the INPUT_TYPE_WHITELIST_PATTERN from ESAPI.securityConfiguration().getValidationPattern(INPUT_TYPE)
6. DefaultValidator assigns INPUT_TYPE_WHITELIST_PATTERN to StringValidationRule
7. DefaultValidator delegates the validation request to StringValidationRule.getValid.
8. [PASS] StringValidationRule asserts INPUT is consistent with expected WHITELIST PATTERN
9. StringValidationRule requests the canonicalized form of INPUT from ESAPI.encoder().canoncalize(INPUT
10. HTMLEntityCodec does NOT decode INPUT.
11. PercentCodec decodes INPUT to PERCENT_CODEC_DECODE
12. JavaScriptCodec does NOT decode PERCENT_CODEC_DECODE
13. HTMLEntityCodec does NOT decode PERCENT_CODEC_DECODE.
14. PercentCodec does NOT decode PERCENT_CODEC_DECODE.
15. JavaScriptCodec does NOT decode PERCENT_CODEC_DECODE
16. Encoder determines INPUT is clean, and returns PERCENT_CODEC_DECODE to StringValidationRule
17. [FAIL] StringValidationRule asserts PERCENT_CODEC_DECODE is consistent with expected WHITELIST PATTERN - ValidationException Thrown
18. SecurityWrapperRequest swallows the exception (catches it, performs no action)
19. SecurityWrapperRequest returns an empty String to the client.

I believe that this is just a bad test expectation, and the test should be updated to reflect the whitelist configuration settings.

	public void testGetQueryStringPercentAmpersand()
	{
		MockHttpServletRequest req = new MockHttpServletRequest();
		SecurityWrapperRequest wrappedReq;
		
		req.setQueryString("a=%26b");
		wrappedReq = new SecurityWrapperRequest(req);
		//assertEquals("a=%26b",wrappedReq.getQueryString());
		//GIT ISSUE 135 -- Decoded 'a&b' does not pass default whitelist reqs.  Expect Blank.
		assertEquals("",wrappedReq.getQueryString());
	}

---

• The content suggested here allows all tests to pass. The only open concern I have is whether there is ANY case that we would want the SecurityWrapperRequest to return the decoded QueryString reference? That feels odd to me, but I don't believe I have the experience in the arena to make that call. Input would be appreciated.
• The logging modifications mentioned in previous comments are not required to resolve this issue. I think that should be moved to another task for later investigation.

I'm out of practice with git. I had not merged with the current head when I was doing my testing. There is a change to the property HTTPHeaderValue from the source I was testing with and the current develop tip. The current value is:

Validator.HTTPHeaderValue=^[a-zA-Z0-9()\\-=\\*\\.\\?;,+\\/:&_ ]*$

Whereas my tests above were using this setting from an older version:

Validator.HTTPHeaderValue=^([a-zA-Z0-9_\-]{1,32}=[\p{L}\p{N}.\-/+=_ !$*?@%]*&?)*$

This changes the outcome of the tests.

testGetQueryStringPercentAmpersand now does not violate the whitelist settings, so the original expected return of "a=%26b" is correct.

testGetQueryStringPercent still requires the update (as listed in my previous comment) to expect the encoded string of "a%62"

---

testGetQueryStringPercentNUL previously expected an empty string, but the updates allow the NUL characters to pass validation and whitelist concerns. "a=%00" is the matching return.

I'm not sure this is the desired response.

These changes are pushed to my fork https://github.com/jeremiahjstacey/esapi-java-legacy

First, my bad for not responding earlier. I was working on non-ESAPI related things that were taking up a lot of time. (Gasp!) @jeremiahjstacey Anyhow, I'm somewhat confused by your comment that a difference in the value for Validator.HTTPHeaderValue would make a difference in the SafeRequestTest.testGetQueryStringPercentNUL() test case. That is using SecurityWrapperRequest.getQueryString(), not SecurityWrapperRequest.getHeader() nor any of the related methods that ought to be dealing with HTTP headers. Query strings and header values should be treated independently of each other so if there is some mysterious relationship between the implementation of the two such that query strings somehow depend on Validator.HTTPHeaderValue, then IMO, that is just wrong. So is that what you are saying--that they are interrelated--or am I just misunderstanding something. (I certainly hope it is the later.)

@planetlevel Jeff, would appreciate if you could take a read through this thread and give us your $.02 on what is intended / anticipated by these test results here. Thanks.

@kwwall You are right, and the HTTPHeaderValue is not making a difference. I must have munged my context while I was going through debug. In an earlier post I even outline the "type" parameter as HTTPQueryString in the workflow. I'm not sure how I mixed that up, my apologies, and thank you for pointing out the inconsistency. The property being used is

Validator.HTTPQueryString=^([a-zA-Z0-9_\\-]{1,32}=[\\p{L}\\p{N}.\\-/+=_ !$?@%]&?)*$

Null is being returned to the test.

I'm not sure if it's related, but the StringValidationRule.getValid implementation has changed as well with modifications to resolve issue #284.

https://github.com/ESAPI/esapi-java-legacy/commit/a9849f1421617b5087685daeb7239015bd753930#diff-1952223d7dbaf29bdc540c82d1034891

I think it may have been the whitelist check of the canonicalized 'data' value that had been causing the test to fail as originally expected.

When I add the line checkWhitelist(context,data,input);

I see the originally expected empty string return, and can see that the whitelist check does throw the ValidationException, which is swallowed by the SecurityRequestWrapper.getQueryString implementation.

@jeremiahjstacey Yep; I think those changes to StringValidationRule is what caused these tests to fail. Just to double-check, I put back an older version (one that I had from the work I was doing on BitBucket) and compiled it and ran the tests and all tests passed.

Of course, that doesn't allow us to escape quite that easily. We are still back to the question that kicked off this (then Google Code) issue, which is "what unescaping (if any) should SecurityRequestWrapper.getQueryString() be doing here?". I think there are a few related questions too, such as if we think that getQueryString() should return the 'raw' query string, do we want to add a method (something like getDecodedQueryString() that would do the unescaping of % encoding and other possible encodings? And if we think that that should be the default behavior, what is the best approach to keep this backward compatible with previous ESAPI releases so we don't break anyone's existing production code should they update to a new version of ESAPi that changes the default behavior of SecurityRequestWrapper.getQueryString()?

I'm fine trusting the 2 of you to decide on this and come up with a solution, just keep me in the loop. And I appreciate your hard work on this from both of you.

If I'm interpreting the issue correctly, the legacy implementation breaks default expected behaviors of the javax api. Since the SecurityRequestWrapper is an extension of javax.servlet.http.HttpServletRequestWrapper and implements javax.servlet.http.HttpServletRequest, we are changing the industry standard expected behavior by altering the default return value from the delegate HttpServletRequest implementation.

https://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest.html#getQueryString()

Returns: a String containing the query string or null if the URL contains no query string. The value is not decoded by the container.

I interpret this documentation to state that the method will return the literal query string content from the URL, and not an interpreted format of that element.

I believe that this was the intent, and impact, of my commit.

@xeno6696 looking for your input.

---

With regards to the test: What do we actually want to test?

This unit test is actually running integration-level validation, and has an explicit reliance on configuration.

At a unit level, I believe a more explicit test to assert data flow would be helpful. By that, what I intend is to use Mock implementations to assert that when SecurityWrapperRequest.getQueryString() is invoked, that the ESAPI validator is called. Not only is it called, but the exact parameters we are expecting are provided. The validator should be a mock that is verified post-invocation. There should be 2 primary tests.

1. HappyPath behavior. All parameters to the Validator are asserted, invocation is checked, return value is a not-null, not-empty return. Test asserts that the return of the getQueryString matches the original query string from the servlet request.
2. Exception case. Validator is mocked to throw an exception. Assert that the Validator method was called, with expected parameters. Test asserts that the return of the getQueryString is a blank string.

That insulates this test from the environmental test conditions and at least asserts that the workflow is functional consistently.

If we are required to test the specific configurations, such as 'The default Validator.HTTPQueryString property allows null values', then I think that should be addressed under a separate effort, with a specific set of tests that validate that property configuration in isolation.

Adding additional protection on top of the Servlet Spec is exactly what ESAPI is designed to do. It's a bit like TCP adding sequencing on top of IP. You can build insecure and secure applications on top of the Servlet Spec. ESAPI, and particularly the filter, is a way to add some security capabilities on top. Specifically, the behavior of getQueryString is overridden such that (from the Javadoc):

"Returns the query string from the HttpServletRequest after canonicalizing and filtering out any dangerous characters."

In this case, the idea is that applications built on top of ESAPI take advantage of the fact that input from the querystring is fully decoded (canonicalized). That's probably a really good idea for the vast majority of applications. I hope we can agree that all forms of double and multiple encoding are probably not a good idea to accept. But there may be some applications that rely on receiving single encoded input. If so, then the filter will break them.

@planetlevel I believe I understand your point. Based on the documentation, the behavior of the SecurityRequestWrapper is doing exactly what it is intended to do.

If that's true, is this issue really a bug at all?

---

More what I mean to say is that the original HttpServletRequest already returns the single encoded input. If that's the behavior that's required then the SecurityRequestWrapper just shouldn't be used? If the user is in an environment where they require the canonicalized input, then use this implementation. Single encoded input was never the design of this implementation. If the end state we're getting to is that we need to provide another implementation of an esapi-wrapped HttpServletRequest that validates safe input and returns a single-encoded representation, then that would be a new feature to the library altogether?

Well, I think the question is how one should pass an & via the querystring without it causing another parameter to be introduced. For example if my name is O&Brian for example... ?name=O&Brian will get parsed like name=O and Brian="". This wouldn't affect request.getParameter(), only custom parsers. I haven't seen too many apps that parse their own querystring, but it's certainly possible.

So it's sort of a tradeoff. Do we want to protect the ability of people to write custom querystring parsers and accept the risk of forcing them to do their own decoding. This opens the door to a variety of multiple, mixed, and nested encoding attacks. Or, do we decode everything and make it impossible to pass in & and = characters.

A middle ground might be to allow only one level of percent encoding. And we could decode everything except & and = .

@jeremiahjstacey You wrote:

@planetlevel I believe I understand your point. Based on the documentation, the behavior of the SecurityRequestWrapper is doing exactly what it is intended to do.

If that's true, is this issue really a bug at all?

If you recall, we began this discussion on a private email thread and it was about a the test case testGetQueryStringPercentNUL() in src/test/java/org/owasp/esapi/filters/SafeRequestTest.java failing.

I wasn't convinced that it was a bug in the test or a bug in SecurityRequestWrapper.getQueryString(), but I do know--and I think we're all in agreement here--that we can't have a test regularly failing in an official ESAPI release. That just is piss poor practice. So either we fix the code or fix or remove the test. But we're not to go release a new ESAPI version on my watch where the test consistently fails.

IIRC, I think how it got associated here is that in the code there was a commented chunk of code that referenced (Google Code issue) 125 which is the same as this issue. And so, here we are.

One thing that I brought up with @planetlevel in a private email was that I am concerned that if we want to do canonicalization than perhaps we need more complete tests. The reason for this is that we are only testing default ESAPI configurations, yet we allow (and perhaps expect?) development teams to 'tune' the ESAPI.properties to their own needs. So what if someone changes the order of (say) Encoder.DefaultCodecList=HTMLEntityCodec,PercentCodec,JavaScriptCodec or leave out or or more codecs or say the Validator.HTTPQueryString property? Should we at least not have some tests to make sure that something sane happens. Granted such scenarios are difficult to test, but how do we know for certain that ESAPI doesn't do something crazy unsafe such as accepting ANY input if a developer were to change the order of Encoder.DefaultCodecList or even (egads!) remove all the codes from the list?

Wait a sec... let's not go crazy. The canonicalization contract is that when it's complete, there are no encoded characters left in the input. You might get a slightly different result if the order of the decoders is different, but only if you allow multiple/mixed/nested encoding (which is a bad idea IMO). If we only allow single encoding, then order doesn't matter.

But in any case, step 2 is to validate against the Validator.HTTPQueryString property. So whatever gets accepted has to pass that criteria. This is exactly why we shouldn't allow the return of raw results. That means you just validated the canonicalized string and returned the raw string. We should only return canonical, validated results.

I appreciate both of your input with this. I am trying to grasp what the best outcome for this effort is.

With regards to the canonicalization of the return value of SecurityWrapperRequest.getQueryString -- I agree that my initial proposed change is incorrect. Based on input from @planetlevel the method was originally doing exactly what it was designed to do. It should not be modified. This bug is oriented around that method and that defined and expected behavior (based on my understanding of the description).

I believe it's safe to say that at this point we all agree that this bug is not a bug. The code works as designed. Along that line, my commit are changes that should not be merged.

I am comfortable with that resolution, and apologize for my side-track down that path.

---

The second topic is the behavior of the unit test for SecurityWrapperRequest.getQueryString in SafeRequestTest. The tests had been commented out as non-functional, and now that they've been re-activated may fail.

Focusing on that, it is my belief that this test is trying to test too much at once. I've tried multiple times to write up something that describes what I mean by this, but I'm just not quite able to make the point I am looking for. So instead of trying to describe what I think should happen to resolve this issue, I'd like to show you my opinion.

I've created a branch in my fork named 'jstacey_135', and in that I've shifted the test implementation to be smaller units, focused on where there is minimal impact from anything outside the unit. I would appreciate it if you could find the time to look through these changesets. I believe this will not only fix the current test issue, but also prevent the arbitrary breaks in the future; all while still providing the same coverage across the system.

The major difference is that I have purposefully excluded the environmental conditions from these implementations.

I'm not sure what the best way to offer this is, so here are the links. If there is a better way to present this please let me know and I will do my best to facilitate.

https://github.com/jeremiahjstacey/esapi-java-legacy/commit/2c199e5e84ebee32e1147440c8ef9b72ecf8e198

https://github.com/jeremiahjstacey/esapi-java-legacy/commit/f006b5308eb54e2ba316a4b55e24cc6430fdcfed

https://github.com/jeremiahjstacey/esapi-java-legacy/commit/5f91df0a6a454bc5df04e2e64918ebba59f4512a

https://github.com/jeremiahjstacey/esapi-java-legacy/commit/f5a190ff8a709bf712783cac12280c4f786c2624

https://github.com/jeremiahjstacey/esapi-java-legacy/commit/6f60045cdd20e7b32cb2814734e58eb2c23e8ffe

@jeremiahjstacey You wrote "... and apologize for my side-track down that path". Please, no apology is necessary. If anyone should apologize it should be all those people who regularly use ESAPI by just sit back and criticize it for low development activity. If some of those people were stepping up and helping like you are, we wouldn't be in this predicament.

I did a very cursory approach to your changes, but I want to take a more detailed look tomorrow. I will then either comment here or on your individual commits to your branch.

Thanks again for all the effort you've put into this. And note that even when you make mistakes, I'm okay with that. That's how we all learn and if we are not making any mistakes, it just means that we are not trying hard enough.

-kevin

“And note that even when you make mistakes, I'm okay with that. That's how we all learn and if we are not making any mistakes, it just means that we are not trying hard enough.”

This!! On Sat, Jan 20, 2018 at 16:07 Kevin W. Wall [email protected] wrote:

@jeremiahjstacey https://github.com/jeremiahjstacey You wrote "... and apologize for my side-track down that path". Please, no apology is necessary. If anyone should apologize it should be all those people who regularly use ESAPI by just sit back and criticize it for low development activity. If some of those people were stepping up and helping like you are, we wouldn't be in this predicament.

I did a very cursory approach to your changes, but I want to take a more detailed look tomorrow. I will then either comment here or on your individual commits to your branch.

Thanks again for all the effort you've put into this. And note that even when you make mistakes, I'm okay with that. That's how we all learn and if we are not making any mistakes, it just means that we are not trying hard enough.

-kevin

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/135#issuecomment-359209330, or mute the thread https://github.com/notifications/unsubscribe-auth/AJEAQSOxHzBqNT11FIuaZOgroIJ38-6Pks5tMnG-gaJpZM4C6wjw .