Skip to content
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.

Sign up for GitHub

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

Jump to bottom

It's likely redirects for sockets and regular hosts never behaved as expected #230

Closed
dimitribouniol opened this issue May 28, 2020 · 3 comments · Fixed by #259
Closed

It's likely redirects for sockets and regular hosts never behaved as expected #230

dimitribouniol opened this issue May 28, 2020 · 3 comments · Fixed by #259
Labels
kind/bug Feature doesn't work as expected.
Milestone
1.2.1

Comments

@dimitribouniol
Copy link
Contributor

While refactoring the unix domain socket implementation in #228, I noticed that redirects likely haven't been checking URL schemes correctly for some time now, or I'm not completely following the logic and intended use of RedirectHandler.

My assumption is that RedirectHandler.redirectTarget(status:, headers:) validates the incoming header, and returns a URL to redirect to if it passes all the necessary checks.

If I'm right, that means that the redirect logic checks the current request rather than the new URL to see if it can redirect to it - this likely never worked as designed due to the fact that it would always pass, since we have a request that is already valid, and we are checking it once again for validity...

It's also likely that this check never worked, but redirects to foreign URLs were still possible, since creating the Request later in the process would eventually fail, but there are no checks to ensure the Request hasn't been redirected from a host-based server to a socket based one, so something is still fishy in that regard.

If my assumption isn't misguided in some way, I suggest we refactor the check for the scheme as follows:
image

The new supportsRedirects(to:) method (refactored, since it is only being used in this scenario now):
image

Additional tests should clarify if this is working as designed or not.
@dimitribouniol dimitribouniol mentioned this issue May 28, 2020
Merged
@Lukasa
Copy link
Collaborator

Lukasa commented May 28, 2020

Yup, this looks like the correct diagnosis. It'd be really good to add a test for this to verify that the behaviour really is wrong, and then apply the fix.

@artemredkin artemredkin added kind/bug Feature doesn't work as expected. help wanted labels Jun 13, 2020
@artemredkin artemredkin added this to the 1.2.1 milestone Jun 13, 2020
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 13, 2020
@dimitribouniol
Added tests that ensure redirects to unix socket paths from a regular…
507d66d 
… HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 13, 2020
@dimitribouniol
Fixed an issue where redirects to socket path-based servers from any …
249b9f0 
…server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230
@dimitribouniol
Copy link
Contributor Author

@Lukasa I finally got around to writing tests for the above, which definitely do fail:
image

The second half (which currently passes) is to ensure the other scenarios (http(s)+unix to http(s)/http(s)+unix) all continue to work.
master...dimitribouniol:redirect-validation

Any other concerns with this approach, or should I open a PR with the fix?

@Lukasa
Copy link
Collaborator

Lukasa commented Jun 15, 2020

No concerns from me, this approach looks reasonable.

dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 15, 2020
@dimitribouniol
Added tests that ensure redirects to unix socket paths from a regular…
fce2c9f 
… HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 15, 2020
@dimitribouniol
Fixed an issue where redirects to socket path-based servers from any …
3810060 
…server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230
@dimitribouniol dimitribouniol mentioned this issue Jun 15, 2020
Merged
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 16, 2020
@dimitribouniol
Added tests that ensure redirects to unix socket paths from a regular…
ee07ddf 
… HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 16, 2020
@dimitribouniol
Fixed an issue where redirects to socket path-based servers from any …
6c87c34 
…server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 17, 2020
@dimitribouniol
Added tests that ensure redirects to unix socket paths from a regular…
e8dc0de 
… HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 17, 2020
@dimitribouniol
Fixed an issue where redirects to socket path-based servers from any …
79190aa 
…server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 20, 2020
@dimitribouniol
Added tests that ensure redirects to unix socket paths from a regular…
71d7f7f 
… HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.
dimitribouniol added a commit to dimitribouniol/async-http-client that referenced this issue Jun 20, 2020
@dimitribouniol
Fixed an issue where redirects to socket path-based servers from any …
9db6474 
…server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230
artemredkin added a commit that referenced this issue Jun 22, 2020
@dimitribouniol @artemredkin
Fixed an issue where redirects to socket path-based servers from any …
5c7a317 
…server was always allowed (#259)

* Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See #230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.

* Fixed an issue where redirects to socket path-based servers from any server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes #230

Co-authored-by: Artem Redkin <[email protected]>
artemredkin added a commit to artemredkin/async-http-client that referenced this issue Jun 23, 2020
@dimitribouniol @artemredkin
Fixed an issue where redirects to socket path-based servers from any …
233f18b 
…server was always allowed (swift-server#259)

* Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed.

Motivation:

Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230.

Modifications:

- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.

Result:

Failing tests that show that the existing redirect checks were inadequate.

* Fixed an issue where redirects to socket path-based servers from any server was always allowed.

Motivation:

An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.

Modifications:

- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.

Result:

Closes swift-server#230

Co-authored-by: Artem Redkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Milestone
1.2.1
3 participants
@dimitribouniol @artemredkin @Lukasa