Skip to content

match redirect authority in http.redirect.allowed.uri check - #3624

Closed
rootvector2 wants to merge 1 commit into
apache:mainfrom
rootvector2:redirect-allowed-uri-authority
Closed

rootvector2 wants to merge 1 commit into
apache:mainfrom
rootvector2:redirect-allowed-uri-authority

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

HTTPConduit.checkAllowedRedirectUri enforces http.redirect.allowed.uri with a plain newURL.startsWith(allowedRedirectURI), so with the allowed value https://api.example.com a server can answer 302 with Location: https://api.example.com@evil.example/ (or https://api.example.com.evil.example/) and the conduit follows it, re-applying the configured AuthorizationPolicy so Basic credentials are sent to the other host. Found while reviewing the conduit redirect checks, and the new HTTPConduitAllowedRedirectUriTest reproduces it with two local servers. The prefix check now also requires the redirect URI's authority to equal the allowed URI's authority.


String allowedRedirectURI = (String)message.getContextualProperty(AUTO_REDIRECT_ALLOWED_URI);
if (allowedRedirectURI != null && !newURL.startsWith(allowedRedirectURI)) {
if (allowedRedirectURI != null && !isAllowedRedirectUri(newUri, newURL, allowedRedirectURI)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 properties in play here:

  • AUTO_REDIRECT_SAME_HOST_ONLY: this one only allow redirects to the same scheme/host and will address your concern I believe
  • AUTO_REDIRECT_ALLOWED_URI: freestyle prefix match (the user could make the prefix as strict as possible)

Hope it makes sense, thank you

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. With http.redirect.same.host.only set, the @evil.example and .evil.example redirects are already rejected, and a configured value with a trailing / (e.g. https://api.example.com/) closes it for http.redirect.allowed.uri too. So this is a config concern rather than a bug in the check. Happy to close this, or turn it into a short doc note on the property that the value is a raw prefix match, whichever you prefer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update docs shortly, seems like we never documented those, the pr could be closed, thanks @rootvector2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, closing this one. A note that http.redirect.allowed.uri is a raw prefix match, so the value should end with /, would cover the case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants