Summary

StrictHttpFirewall rejects certain patterns in URL paths that might lead to path traversal. This includes double-slashes (//) or any number of consecutive slashes for that matter. I agree that it's a bad idea to have consecutive slashes in URL paths. However, I don't consider them as critical as /../ segments. I'd like to suggest that rejecting consecutive slashes should be optional.

Actual Behavior

StrictHttpFirewall always rejects consecutive slashes.

Expected Behavior

StrictHttpFirewall should have a configuration property like allowConsecutiveSlashes. If that is set to true, StrictHttpFirewall should tolerate consecutive slashes. Naturally the default should be false, in order to prevent changes of out-of-the-box behavior.

Configuration

Not tested yet, but the new behavior could be configured like this:

<bean id="myHttpFirewall" class="org.springframework.security.web.firewall.StrictHttpFirewall">
    <property name="allowConsecutiveSlashes" value="true"/>
</bean>
<security:http-firewall ref="myHttpFirewall"/>

Version

Tested with master branch from git. Should also affect latest 5.0.x and 4.2.x.

Sample

Will provide a pull request...

Comment From: rwinch

@meeque Thanks for creating this ticket.

I agree that it's a bad idea to have consecutive slashes in URL paths. However, I don't consider them as critical as /../ segments. I'd like to suggest that rejecting consecutive slashes should be optional.

Can you please elaborate on

1) What is the use case for allowing //? 2) Why do you feel rejecting // is not as critical as /../? The attack accomplishes the same thing through only a slightly different means, so I'm not sure why you would consider one to be worse than the other.

Comment From: meeque

@rwinch Tbh, the use case is just sloppy programing:(

Sometimes developers think they can create new URL paths by simply concatenating two or more path fragments. (Instead, they should be using proper URL resolution logic of course.) Anyway, they've learned that clients and servers will tolerate double-slashes, and treat them as a single slash. On the other hand, their URL will break, if a slash is omitted altogether. Thus, the like to add a trailing slash on the first part of the URL path and a leading slash on the second part. This bad habit is enforced by fact that one part of the URL path might come from application configuration, and developers don't have full control over it (but they are too lazy to implement appropriate checks).

Well, I'm dealing with such applications. We will certainly try to improve our code regarding URL path handling. However, due to the size of the applications it's not a trivial endeavor.

Comment From: meeque

@rwinch Regarding the criticality of //:

I think we have to consider two different aspects here:

  1. Preventing good old path traversal attacks: For this, it seems that only double-dots (..) are relevant. And only if they're surrounded by slashes, or come at the beginning of the path. This could lead to path traversal vulnerabilities in downstream components, e.g. when they try to map URL paths to directory paths, and don't do their own checks for path traversal. I don't see how // (or even /./ for that matter) could be harmful here. This would require some sort of non-standard path mapping, where these control sequences would navigate up the directory hierarchy. Well, I'm not saying no-one's doing that, but I haven't seen it in the wild yet.
  2. Interfering with spring security's path matching rules, e.g. in intercept-url configuration: This of course could lead to a authorization bypass, if there is a mismatch between how Spring Security matches the path vs. how application components do their path mapping. So I've looked into how Spring Security does pattern matching on URL paths. Afaik, it uses AntPathRequestMatcher by default. (And in our case, we don't override that.) That implementation will happily match any number of consecutive slashes in the URL wherever a single slash appears in the pattern. Thus, individual intercept-url patterns cannot be by-passed by adding more slashes to the URL path. Moreover intercept-url definitions are ordered and the first one that matches is applied. Therefore, stricter authorization constraints must be listed first and access cannot be weakened by additional slashes. (At least that seems to be the approach in our security configuration.)

Well, I may have overlooked something. So please correct me if I'm wrong! Otherwise, I don't think that allowConsecutiveSlashes would weaken StrictHttpFirewall more than any of the other configuration properties. And it might help some people to address backwards compatibility problems similar to the ones that I've encountered. If not, developers might get tempted to switch back to DefaultHttpFirewall, which is surely not a good idea?

Comment From: meeque

Just fyi, here's some tests that I wrote for AntPathRequestMatcher. They demonstrate how it will deal with consecutive slashes. I didn't add them to the PR, since I found they're slightly off-topic.

@Test
public void doubleWildcardMatchesDoubleSlashes() {
    AntPathRequestMatcher matcher = new AntPathRequestMatcher("/**", "POST");
    MockHttpServletRequest request = createRequest("/test//test");
 
    assertThat(matcher.matches(request)).isTrue();
}
 
@Test
public void wildcardsAndSlashMatchesDoubleSlashes() {
    AntPathRequestMatcher matcher = new AntPathRequestMatcher("/*/*", "POST");
    MockHttpServletRequest request = createRequest("/test//test");
 
    assertThat(matcher.matches(request)).isTrue();
}
 
@Test
public void literalAndSlashMatchesDoubleSlashes() {
    AntPathRequestMatcher matcher = new AntPathRequestMatcher("/test/test", "POST");
    MockHttpServletRequest request = createRequest("/test//test");
 
    assertThat(matcher.matches(request)).isTrue();
}

Comment From: rwinch

@meeque Thanks for the response.

Preventing good old path traversal attacks: ... Well, I'm not saying no-one's doing that, but I haven't seen it in the wild yet.

We have seen these types of exploits and that is why we are defending against them. This is one of the benefits of open source.

Interfering with spring security's path matching rules, e.g. in intercept-url configuration

If you have a security rule that is /admin/** -> ADMIN, then you would expect any URL starting with /admin/ to require the role ADMIN. A malicious URL could send a URL /public//admin/secret which would bypass the security constraint, but may be normalized by the container or servlet to be /admin/secret so the contents of /admin/secret is actually rendered. This depends on how the URL is normalized, but this has caused exploits in the past.

Another point is that there are lots of different methods that include the URL on HttpServletRequest. Some of the methods expose a raw URL, others are URL decoded and potentially normalized, etc. This ambiguity can cause security bypasses. Additional complexities are added when you account for different character encodings which can trick the container into decoding/normalizing the URL in an insecure way.

I can assure you that all of these protections have come from real attacks. Not all of them occur in every container, but as a security framework we aim to protect users from what they do not know.

If you like, you can leverage Spring Security's DefaultHttpFirewall by exposing it as a Bean. Rather than rejecting unnormalized URLs it will attempt to normalize them for the duration of Spring Security's Filter's and then reset them back to the way they were before. This is more lenient that StrictHttpFirewall, but can cause bypasses in some containers due to ambiguities listed above.

I'm closing this as won't fix as you can leverage DefaultHttpFirewall if you wish.

Comment From: meeque

Thanks for looking into it, @rwinch . I can understand your concerns about weakening such security controls. And I can see that the benefit of my proposed feature is questionable and may not warrant taking additional risks. So I understand the decision to reject this PR.

If you have a security rule that is /admin/** -> ADMIN, then you would expect any URL starting with /admin/ to require the role ADMIN. A malicious URL could send a URL /public//admin/secret which would bypass the security constraint, [...]

Well, as explained above, I think that the /admin/** rule would match the malicious URL from your example, too. Of course, this is a fragile assumption as it depends on the particular type of matcher that is used. I guess I'll have to do more research to verify it in my particular setup...

Regarding DefaultHttpFirewall: that would be the obvious solution of course. However, I was worried that it would make us even more vulnerable. After all, it is deprecated. And afaik, StrictHttpFirewall was introduced to address CVE-2018-119. Wouldn't the usage of DefaultHttpFirewall make us (and others) vulnerable to that again?

I know this is off-topic. But I struggled to find any tangible information on that particular vulnerability online. However, I noticed that there were no changes in DefaultHttpFirewall (or RequestWrapper) that would address CVE-2018-119. At least not since the time when the vulnerability became public. Do you have any advice on that?

Comment From: rwinch

DefaultHttpFirewall is better than removing security features in StrictHttpFirewall. StrictHttpFirewall was an agressive reaction to problems with trying to sanitize the URLs vs reject them. However, sanitation is better than ignoring the security restrictions all together.

I know this is off-topic. But I struggled to find any tangible information on that particular vulnerability online.

The real issue/changes were in Spring Framework. StrictHttpFirewall is something that viewed as necessary to provide defense in depth and to eliminate the types of attacks I have mentioned above.

PS: If you are using Spring MVC make sure you use MvcRequestMatcher instead of the ant matchers.

Comment From: meeque

@rwinch Thank you very much! I'll stick with DefaultHttpFirewall then. And I'll double-check our usage of matchers... Over and out:smile:

Comment From: lts-rad

For developers wondering about why it's important to be so strict, check out Orange Tsai's presentation from BlackHat:

https://i.blackhat.com/us-18/Wed-August-8/us-18-Orange-Tsai-Breaking-Parser-Logic-Take-Your-Path-Normalization-Off-And-Pop-0days-Out-2.pdf, it covers CVE-2018-1271 which Orange reported in Spring for Windows

Comment From: deepakab03

One usecase where the double forward slash is used is in Angular routing when there are multiple sub panels. For example the URL could be: http://localhost:8080/abcApp/app/(dbe-panel//xyz-panel) This request is then sometimes passed to the server side, for example when the user refreshes his page or bookmarks and directly opens up a URL, the server then has to just serve up the Angular index.html so that Angular can use the route to open the appropriate sub panels. However if the request itself is rejected then the server doesn't get the opportunity to serve up angular content.

It would be good if the StrictHttpFirewall can support such a use-case by allowing the use of a double fwd slash

Comment From: rwinch

This cannot be the default as it can allow path traversals which is one of the key features of the firewall.

As for Angular routing...I'm not that familiar with Angular routing, but I would expect that the routing is part of a anchor rather than the URL itself (that's how I've seen it in the past at least).

Comment From: coderroggie

@deepakab03 we were seeing similar issues with the double slash in keycloak and got around it by implementing a custom UrlSerializer in Angular 9. Keycloak collapses the double slash '//' (likely to avoid security issues above) but we were able to grep for the double slash and replace it while serializing and reverse the process when parsing. See https://digitalflask.com/blog/extend-angular-router/ for a similar implementation.

Comment From: madmaxmofo2020

The "Configuration" doesn't work. You get this error:

Bean property 'allowConsecutiveSlashes' is not writable or has an invalid setter method.

Comment From: rwinch

@madmaxmofo2020 that was a proposal from the submitter of the issue. The issue was rejected for the reasons described above. Read the thread for details on what your options are and why we rejected the request.