Fix open redirect CVE-2019-13038

[awakenine]

Fix open redirect CVE-2019-13038
#35 (comment)

[olavmrk]

Hi,

first of all, sorry for taking so long before looking at this pull request.

I had a look at this pull request now. Overall, I am positive to the change, but I don't really trust the apr_uri_parse()-function anymore, since it has had so many problems.

I think we at least need to add more checks for what it returns.

My suggestion would be to allow two types of URLs: Absolute and relative to the root of the site.

For absolute URLs, we should require that scheme, hostname and path are set. We can allow port_str, query and fragment to be set. We should forbid forbid user and password.

For relative URLs, we should require path to be set, and allow query and fragment to be set. scheme, user, password, hostname and port_str should be forbidden.

In addition, for both absolute URLs and relative URLs, we should verify that path starts with a single /. I.e. "/" and "/example" are allowed, but // is forbidden.

[olavmrk]

Could you fix the indentation of the comment and closing curly bracket?

[awakenine]

Hi @olavmrk. Sure, it should be fixed now.

[awakenine]

apr_uri_parse()

I used this code to check apr_uri_parse() behavior, and then text to Apache foundation to discuss that.
Their answer was that URI parser works simply according to RFC (please see https://tools.ietf.org/html/rfc7230#section-2.7.1).
If you need URL parser, it will be better to use safe URL parser, or create auxiliary function for parsing URL with all required security checks.

[olavmrk]

If you need URL parser, it will be better to use safe URL parser, or create auxiliary function for parsing URL with all required security checks.

Yes, that is why the fix I imagined for the latest problem was to replace apr_uri_parse() with a really paranoid URL parser, though I realize I never wrote it out in my comment.

As I said in my comment, this is the third time that using apr_uri_parse() has given us a problem.

If we are not going to replace apr_uri_parse() here, I would rather be really paranoid in checking its output, which may reduce the probability of further open redirects being discovered.

[olavmrk]

Closing this pull request as part of archiving this project. See the announcement for details:

https://github.com/Uninett/mod_auth_mellon/blob/info/README.md