New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StrictHttpFirewall allows "//path" "//path//path" patterns. #5044

Closed
hatanaka-akihiro opened this Issue Feb 27, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@hatanaka-akihiro
Copy link

hatanaka-akihiro commented Feb 27, 2018

Summary

StrictHttpFirewall checks non-normalized URL containing double slash (//).
However, I think this checking algorithm is ambiguous.

Actual Behavior

  • /path//path -> 500 error
  • //path/path -> OK
  • //path//path -> OK

Is this intended specification? or bug?

Expected Behavior

  • /path//path -> 500 error
  • //path/path -> 500 error ?
  • //path//path -> 500 error

Version

Spring Security 5.0.1

Comment

StrictHttpFirewall.isNormalized(String)

		if (path.indexOf("//") > 0) {
			return false;
		}

>=0 is right?
or lastIndexOf("//") ?

@rwinch

This comment has been minimized.

Copy link
Member

rwinch commented Mar 9, 2018

Thanks for the report! I have pushed a fix for this

@rwinch rwinch added this to the 5.1.0.M1 milestone Mar 9, 2018

@rwinch rwinch added Web Bug labels Mar 9, 2018

@rwinch rwinch self-assigned this Mar 9, 2018

@opncow

This comment has been minimized.

Copy link

opncow commented May 7, 2018

We are running into problems due to this change as the URL that gets checked in StrictHttpFirewall has an added double slash at the beginning that was not sent. The added slash seems to ultimately come from the Apache Coyote Request that is wrapped in the passed in HttpServletRequest (RequestFacade).

To make it clear: We are using embedded Tomcat in version 8.5.29 (the default in Spring Boot2.0.1.RELEASE). Let's say I'm sending the following GET request:

GET https://somedomain:8443/test-service/test

The following happens:

  • At some point, StrictHttpFirewall does its checks
  • It enters StrictHttpFirewall#isNormalized(HttpServletRequest request)
  • The HttpServletRequest implementation that is used is org.apache.catalina.connector.RequestFacade
  • RequestFacade wraps org.apache.coyote.Request (via org.apache.catalina.connector.Request)
  • The isNormalized method calls request.getRequestURI() which in the end gets answered via coyoteRequest.requestURI().toString()
  • For some reason, the result is //test instead of /test
  • isNormalized fails due to that since Spring Security 5.0.4.RELEASE which seems to have the change from this issue here

Any advice besides downgrading to Spring Security 5.0.3.RELEASE? Is there something wrong on our side?

@rwinch

This comment has been minimized.

Copy link
Member

rwinch commented May 7, 2018

@opncow Thanks for the feedback. This issue is closed so the best way to proceed is to create a new ticket so we can track the problem. Please provide details on how to reproduce in the issue. If we cannot reproduce the problem we cannot advise or provide a fix.

@opncow

This comment has been minimized.

Copy link

opncow commented May 7, 2018

@rwinch Okay, I thought you might simply reopen it. I will create a separate issue asap and provide a minimal example project there to reproduce it.

@rwinch

This comment has been minimized.

Copy link
Member

rwinch commented May 7, 2018

As an FYI we cannot reopen tickets once we have done a release because that would change the changelog for a release.

@opncow

This comment has been minimized.

Copy link

opncow commented May 17, 2018

Okay, to clear this one up: It actually was a misconfiguration of our NGINX reverse proxy... :-) Thanks for pointing us on it with this change and sorry for the noise!

@rwinch

This comment has been minimized.

Copy link
Member

rwinch commented May 17, 2018

@opncow Thanks for the follow up! I'm glad you got the problem resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment