DefaultEncoder / getCanonicalizedURI returns mix encoding for HTML special characters

[krog78]

Hi,

DefaultEncoder / getCanonicalizedURI returns mix encoding for HTML special characters in query string (and does not seem to canonicalize the parameter value despite the fact it is mentionned):

esapi-java-legacy/src/main/java/org/owasp/esapi/reference/DefaultEncoder.java

Line 573 in 2136292

//In the case of a uri query, we need to break up and canonicalize the internal parts of the query.

And the canonicalize is applied to scheme, host, port and also UriSegment.SCHEMSPECIFICPART, is it really relevant?

Thanks,
Regards,
Sylvain

[kwwall]

Okay - @xeno6696 - This looks like code you added as part of addressing GitHub issue #394; could you please take a look at it?

Also, @krog78 - Did you have a specific test case in mind and what your expectations for your test case were so we could include it in a future JUnit test? Thanks.

[xeno6696]

There's not enough data here for me to offer a diagnosis. What's the input URL that's giving you trouble? @krog78 we have to start there or there's no conversation to be had.

The method is designed to allow Mixed or Multiple encoding depending on what you have set in your ESAPI.properties file, I would check there first.

As written the line to canonicalize is begins like this:

       for(UriSegment seg: set){
            String value = canonicalize(parseMap.get(seg), allowMultiple, allowMixed);

This iterates over the entire set, queries, if present, included, and they will get canonicalized on this line. Unless of course, you've configured allowMixed and allowMultiple contrary to your intentions. I DO see that I didn't mention in the documentation that those parameters come from ESAPI.properties and will update the docs accordingly.

Further, all of the regression tests were updated in ESAPI to use getCanonicalizedURI, not saying that we couldn't have missed something, but having walked through the code here, I'm not seeing anything obviously wrong.

As for the question:

And the canonicalize is applied to scheme, host, port and also UriSegment.SCHEMSPECIFICPART, is it really relevant?

Would have been nice to have you on the code review back when this was created, I DID NOT think deeply about each URL segment, so this is a bug. Part of what this method was designed to perform was to avoid the general bad practice of using regex to validate URLs when URLs have their own BNF grammar. Validating against that segment breaks our intent, so this is definitely a bug. Separate from your original issue however, which we still have to determine.

I'm currently running a regression having taken that piece out. Please get back to us with the input and usage, and properties file configurations so we can make a determination.

[xeno6696]

And the canonicalize is applied to scheme, host, port and also UriSegment.SCHEMSPECIFICPART, is it really relevant?

Forgot to answer the other half of this question. Yes, scheme/host are relevant. ht<script>alert(1);%20</script>tp is a possible attack vector. Port looks pedantic until you remember that the goal here is to reconstruct a SAFE version of the URL, and you're running into premature optimization if you're worried about canonicalizing it. As per original behavior, DefaultEncoder.canonicalize("http://the.port.would.get.canonicalized.like.this.anyway:9443"); so at the bare minimum I was being hypersensitive to preserve as much of the original logic as possible. I could be convinced to change it, but at worst it's a very very minor performance hit, and I'm against premature optimization.

[xeno6696]

Actually, the more I reflect on this, How are you getting a mixed/multiple encoding exception? The only OTHER way I can see it happening is if you’re applying the OLD canonicalize method to the output from getCanonicalizedURI(URI)

[xeno6696]

So to make that more plain language:

Problem 1: allowMixed and/or allowMultiple are set to false in ESAPI.properties, and in one of the various segments (I think you said queries) you have mixed/multi encoding in a given parameter value that is escaping detection. In this configuration, this is expected behavior.

Problem 2: You take the String given to you by Encoder.getCanonicalizedURI(URI) and are passing that string to a subsequent call to Validator.getValid() which simply reintroduces the behavior of the old canonicalize method where if you have a URL and a query that happens to collide with an HTML entity ?foo=bar&para2=lala will incorrectly fire a mixed/multi encoding exception by "detecting" &para as an HTML entity. I guess I have a documentation failure here, but if you want to check if a URL is valid, I built a convenience method Validator.isValidURI() that uses the default regex to canonicalize and return true/false if the URI is valid.

[xeno6696]

Did a deep dive with a test case, can confirm that URL query strings are indeed canonicalized twice, once on line 541 as demonstrated above, and again in the splitQuery method on lines 618-632.

I would have expected the line on 541 to result in false positive on the URL query but after running several sets of new unit tests to try and tease out the indicated behavior, I'm not striking gold.

I did however find a bug but we're failing secure. With the following input:

we get an unexpected URL back, it's multiple encoding and I can validate that we do throw an exception properly, I haven't tested mixed encoding yet but I have noticed that I didn't have unit tests done to explicitly ensure we catch and throw those exceptions. So that's a good find.

The query string that has the multiple encoding returns "null" so the attack against the URL parameter is thwarted.

I'll leave this open until midweek barring an example parameter, but if that doesn't happen this is a "can't reproduce."

[xeno6696]

Scratch that, it's not a bug, though I still need those unit tests. getCanoncializedURI(URI) assumes that the programmer is knowledgeable enough with the programming idioms used in ESAPI to catch Mixed/Multi exceptions, which since they're runtime exceptions, could escape if you're not expecting them. It's the old design biting us in the ass.

In hindsight, I should never have exposed that method, we should only have exposed the convenience methods on the Validator interface.

[kwwall]

@xeno6696 - if you decide this is a bug or need to make a code or Javadoc change, please turn this discussion into an issue. That will get it into our next release notes. Thanks.

…

-kevin

[xeno6696]

There will already be a minor code change. The question about scheme-specific part was valid, but the way the logic was written we never would have processed that part and it wasn't used to construct the final URL anyway. Removing that resulted in zero regression failures (not that THAT necessarily means anything.) Completeness is impossible but I try anyway. I'll make this an issue. There's already some documentation updates on my fork.

[krog78]

Hi,
sorry for the response delay,

We effectively have both parameters set to false:

Encoder.AllowMultipleEncoding=false
Encoder.AllowMixedEncoding=false

The URL we are using is of kind /webapp/ux/home?d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q%3D%3D&newsess=false&roleid=DP010101/0007&origin=ourprogram, this is a relative URL but I think the problem also occurs with full URL. The following warnings are written:

22-Jan-2024 10:03:28.231 AVERTISSEMENT [http-nio-8080-exec-8] org.owasp.esapi.logging.java.JavaLogLevelHandlers.log [SECURITY FAILURE Anonymous:58505@unknown -> 0:0:0:0:0:0:0:1:8080/eTemptation/Encoder] Mixed encoding (2x) detected in /webapp/ux/home?d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q%3D%3D&newsess=false&roleid=DP010101/0007&origin=ourprogram
22-Jan-2024 10:03:52.919 AVERTISSEMENT [http-nio-8080-exec-8] org.owasp.esapi.logging.java.JavaLogLevelHandlers.log [SECURITY FAILURE Anonymous:58505@unknown -> 0:0:0:0:0:0:0:1:8080/eTemptation/Encoder] Mixed encoding (2x) detected in d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q%3D%3D&newsess=false&roleid=DP010101/0007&origin=ourprogram

The warning is produced when seg = SCHEMSPECIFICPART and on seg = QUERY because of line

esapi-java-legacy/src/main/java/org/owasp/esapi/reference/DefaultEncoder.java

Line 571 in 2136292

String value = canonicalize(parseMap.get(seg), allowMultiple, allowMixed);

(the full line is canonicalized).

Note: the canonicalize parameters of the function are restrictMultiple and restrictMixed but we are passing allowMultiple and allowMixed is it normal?

The first HTMLEntityCodec decodes the string as:

/webapp/ux/home?d=1705914006565&status=login&ticket=1705914653964_thWhiiFp_VESwCkQ-Rq0TU0LZWVKuRxpSUmOzIMsZNCcUIiYGMXX_Q%3D%3D≠wsess=false&roleid=DP010101/0007∨igin=ourprogram

&or has been interpreted as HTML special char (is it normal? I made a test with Chrome, Firefox and Edge with the following code and none is interpreted the special character : Art and Copy).

How should we validate such URLs (containaing HTML special chars) ?

Thanks,
Regards,
Sylvain

[xeno6696]

I will investigate the input.

Likely the schemespecific part was throwing the exceptions as I suggested somewhere in that mess above here. I strongly suspect THAT was the culprit. I will put further responses though over in issue #824

[xeno6696]

No worries. Found issue, no need to migrate anything.

[xeno6696]

Going to close this discussion, please forward any future comments to #824